[Form] Fixed: "date", "time" and "datetime" fields can be initialized with \DateTime objects #3290
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…defined in parent types In turn, FormType::getParent() does not see default options anymore.
…zed with \DateTime objects
|
|
||
| public function getParent() | ||
| { | ||
| return isset($options['widget']) && 'single_text' === $options['widget'] ? 'text' : 'choice'; |
There was a problem hiding this comment.
Add array $options to method signature (same for before block), for sake of clarity.
|
@fabpot Ready to merge. |
| { | ||
| // Throws an exception if "data_class" option is not explicitely set | ||
| // to null in the type | ||
| $this->factory->create('time', new \DateTime()); |
There was a problem hiding this comment.
a test without any assertion would be marked as failing if running PHPUnit in strict mode IIRC
There was a problem hiding this comment.
Not too nice, but perhaps ..
$exception = false;
try {
$this->factory->create('time', new \DateTime());
} catch (\Exception $e) {
$exception = true;
}
$this->assertTrue($exception);
There was a problem hiding this comment.
As long as we don't use PHPUnit strict mode per default, I don't see the point in overcomplicating the test that way.
There was a problem hiding this comment.
@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)
There was a problem hiding this comment.
Would you mind to open a thread on the ML?
There was a problem hiding this comment.
@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;
}
There was a problem hiding this comment.
@bschussek Nah, I don't want to be the evangelist behind this proposition :)
There was a problem hiding this comment.
You can use fail() instead of àssertTrue(false).
There was a problem hiding this comment.
@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);|
@fabpot Ready to merge |
fabpot
added a commit
that referenced
this pull request
Feb 10, 2012
Commits ------- 22c8f80 [Form] Fixed issues mentioned in the PR comments 3b1b570 [Form] Fixed: The "date", "time" and "datetime" types can be initialized with \DateTime objects 88ef52d [Form] Improved FormType::getDefaultOptions() to see default options defined in parent types b9facfc [Form] Removed undefined variables in exception constructor Discussion ---------- [Form] Fixed: "date", "time" and "datetime" fields can be initialized with \DateTime objects Bug fix: yes Feature addition: yes Backwards compatibility break: **yes** Symfony2 tests pass: yes Fixes the following tickets: #3288 Todo: -  Fixed exception that was thrown when doing: $builder->add('createdAt', 'date', array('data' => new \DateTime())); On a side note, the options passed to `FieldType::getDefaultOptions` now always also contain the default options of any parent types. This is necessary if you want to be independent of how `getDefaultOptions` is implemented in the parent type and still rely on the already defined values. As a result, `FieldType::getParent` doesn't see default options anymore. This shouldn't be a big problem, because this method only relies on options in few cases. If it does, options now need to be checked for existence with `isset` before being used (BC break). --------------------------------------------------------------------------- by bschussek at 2012-02-09T16:14:46Z @fabpot Ready to merge. --------------------------------------------------------------------------- by bschussek at 2012-02-10T12:15:04Z @fabpot Ready to merge
fabpot
added a commit
that referenced
this pull request
Feb 11, 2012
Commits ------- dbaddbb [Form] Allow empty choices array for ChoiceType Discussion ---------- [Form] Allow empty choices array for ChoiceType Bug fix: yes Feature addition: no Backwards compatibility break: no Symfony2 tests pass: yes The documentation about ChoiceType says, that default for 'choices' is array(). This is not allowed because an empty array evaluates to false: if (!$options['choice_list'] && !$options['choices']) { Use case: choices are empty and items are added dynamically. --------------------------------------------------------------------------- by chmielot at 2012-02-07T21:03:03Z Sorry, I messed up with the tickets. Didn't know a pull request opens a ticket. --------------------------------------------------------------------------- by bschussek at 2012-02-08T08:23:29Z This ticket depends on #3290 for being merged. Apart from this, a test case is missing. Add this to ChoiceTypeTest: // #3298 public function testInitializeWithEmptyChoices() { $this->factory->createNamed('choice', 'name', null, array( 'choices' => array(), )); } --------------------------------------------------------------------------- by craue at 2012-02-10T20:32:44Z @bschussek: Why does it depend on #3290? I have issues with the `!$options['choices']` check even without #3290 applied. --------------------------------------------------------------------------- by chmielot at 2012-02-10T21:24:28Z Ok, I updated the branch with the test case and craue's suggestion on explicitly checking valid values. --------------------------------------------------------------------------- by chmielot at 2012-02-11T09:07:05Z Should be fine now. --------------------------------------------------------------------------- by bschussek at 2012-02-11T09:15:10Z Good. I see that you added a check for \Traversable too - can you add a test for this too? It should be fine to pass an empty \ArrayObject(). --------------------------------------------------------------------------- by craue at 2012-02-11T10:05:36Z @bschussek: But even if there's passed something different than an array, the c'tor of `SimpleChoiceList` (which is called in line 45) is type hinted with `array` and won't allow passing a `Traversable` anyway, right? --------------------------------------------------------------------------- by bschussek at 2012-02-11T10:16:36Z @craue Yes. But in EntityType the choices option is reused and passed to EntityChoiceList, which extends ChoiceList and accepts any array or Traversable. You're right though that it's not possible to add a test covering this in ChoiceTypeTest, and in EntityTypeTest this is already covered. @fabpot Ready to merge.
This was referenced Apr 5, 2012
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Bug fix: yes
Feature addition: yes
Backwards compatibility break: yes
Symfony2 tests pass: yes
Fixes the following tickets: #3288
Todo: -
Fixed exception that was thrown when doing:
On a side note, the options passed to
FieldType::getDefaultOptionsnow always also contain the default options of any parent types. This is necessary if you want to be independent of howgetDefaultOptionsis implemented in the parent type and still rely on the already defined values.As a result,
FieldType::getParentdoesn't see default options anymore. This shouldn't be a big problem, because this method only relies on options in few cases. If it does, options now need to be checked for existence withissetbefore being used (BC break).