Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

[Form] Fixed: "date", "time" and "datetime" fields can be initialized with \DateTime objects #3290

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Feb 10, 2012
Merged

[Form] Fixed: "date", "time" and "datetime" fields can be initialized with \DateTime objects #3290

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b9facfc
[Form] Removed undefined variables in exception constructor
webmozart Feb 7, 2012
88ef52d
[Form] Improved FormType::getDefaultOptions() to see default options …
webmozart Feb 7, 2012
3b1b570
[Form] Fixed: The "date", "time" and "datetime" types can be initiali…
webmozart Feb 7, 2012
22c8f80
[Form] Fixed issues mentioned in the PR comments
webmozart Feb 9, 2012
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
[Form] Fixed issues mentioned in the PR comments
  • Loading branch information
@webmozart
webmozart committed Feb 9, 2012
commit 22c8f8087cd7eb6179de1c4a473a76a339c16005
4 changes: 2 additions & 2 deletions 4 UPGRADE-2.1.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,14 +237,14 @@ UPGRADE FROM 2.0 to 2.1

Before:

public function getParent()
public function getParent(array $options)
{
return 'single_text' === $options['widget'] ? 'text' : 'choice';
}

After:

public function getParent()
public function getParent(array $options)
{
return isset($options['widget']) && 'single_text' === $options['widget'] ? 'text' : 'choice';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add array $options to method signature (same for before block), for sake of clarity.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

}
2 changes: 1 addition & 1 deletion 2 ...ymfony/Component/Form/Extension/Core/DataTransformer/ChoicesToBooleanArrayTransformer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ public function reverseTransform($values)
}

if (count($unknown) > 0) {
throw new TransformationFailedException('The choices "' . implode('", "', $unknown) . '" where not found');
throw new TransformationFailedException('The choices "' . implode('", "', $unknown) . '" were not found');
}

return $result;
Expand Down
2 changes: 1 addition & 1 deletion 2 tests/Symfony/Tests/Component/Form/Extension/Core/Type/DateTimeTypeTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,6 +264,6 @@ public function testInitializeWithDateTime()
{
// Throws an exception if "data_class" option is not explicitely set
// to null in the type
$form = $this->factory->create('datetime', new \DateTime());
$this->factory->create('datetime', new \DateTime());
}
}
2 changes: 1 addition & 1 deletion 2 tests/Symfony/Tests/Component/Form/Extension/Core/Type/DateTypeTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -536,6 +536,6 @@ public function testInitializeWithDateTime()
{
// Throws an exception if "data_class" option is not explicitely set
// to null in the type
$form = $this->factory->create('date', new \DateTime());
$this->factory->create('date', new \DateTime());
}
}
2 changes: 1 addition & 1 deletion 2 tests/Symfony/Tests/Component/Form/Extension/Core/Type/TimeTypeTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -407,6 +407,6 @@ public function testInitializeWithDateTime()
{
// Throws an exception if "data_class" option is not explicitely set
// to null in the type
$form = $this->factory->create('time', new \DateTime());
$this->factory->create('time', new \DateTime());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a test without any assertion would be marked as failing if running PHPUnit in strict mode IIRC

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestions?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not too nice, but perhaps ..

$exception = false;
try {
    $this->factory->create('time', new \DateTime());
} catch (\Exception $e) {
    $exception = true;
}
$this->assertTrue($exception);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As long as we don't use PHPUnit strict mode per default, I don't see the point in overcomplicating the test that way.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@helmer your way is wrong as it will make it impossible to see why the test really failed (you will hide the error and replace it with an expectation failure)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you mind to open a thread on the ML?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stof True (although majority of tests I've seen throw out "asserting that true is false has failed", and you need to dive into code anyhow) .. let me throw another ugly solution:

try {
    $this->factory->create('time', new \DateTime());
    $this->assertTrue(false);
} catch (\Exception $e) {
    $this->assertTrue(true);
    throw $e;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bschussek Nah, I don't want to be the evangelist behind this proposition :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use fail() instead of àssertTrue(false).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@helmer the difference here is that getting an exception in this place is an error, not a test failure. So the exception should not be catched.

And btw, your snippet is wrong as it will always fail. You could go this way but it should be

$this->factory->create('time', new \DateTime());
$this->assertTrue(true);

}
}
Morty Proxy This is a proxified and sanitized view of the page, visit original site.