Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[Form] Allow pass filter callback to delete_empty option.#20496
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.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I like this, thanks for your proposal!
| $isNew = !isset($previousData[$name]); | ||
| // $isNew can only be true if allowAdd is true, so we don't | ||
| // need to check allowAdd again | ||
| if ($child->isEmpty() && ($isNew || $this->allowDelete)) { | ||
| if (($child->isEmpty() || (null !== $isEmptyCallback && $isEmptyCallback($child->getData()))) && ($isNew || $this->allowDelete)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I would add a variable above to ease readability:
/* @var $child FormInterface */$isNew = !isset($previousData[$name]);$isEmpty =$child->isEmpty() || (null !==$isEmptyCallback &&$isEmptyCallback($child->getData());// $isNew can only be true if allowAdd is true, so we don't// need to check allowAdd againif ($isEmpty && ($isNew ||$this->allowDelete)) {
| @@ -97,6 +98,7 @@ public function configureOptions(OptionsResolver $resolver) | |||
| 'entry_type' => __NAMESPACE__.'\TextType', | |||
| 'entry_options' => array(), | |||
| 'delete_empty' => false, | |||
| 'is_empty_callback' => null, | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Please add:
$resolver->setAllowedTypes('is_empty_callback',array('null','callable'));
d542c63 toc3e6ff5CompareKoc commentedNov 12, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@HeahDude done. Also I've changed the test for better description of this new feature. |
can anybody review and merge? |
Some may have some concern about the naming of this option, for me it's a 👍 Status: Reviewed |
I agree with the implementation, but the option name doesn't sound good. For example, we don't check if the whole collection is empty, but only check for each submitted entry if it should be ignored. Can we think of a better name? |
give some examples of a better name, I'll rename it |
What about something like |
murilolobato commentedMar 17, 2017
If |
d9703d6 tocbd5707Compare@fabpot done, except I prefer Test failure is unrelated. |
yceruto commentedMar 23, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
IMHO, the option should be the same In case of compound entries with 'delete_empty' =>function (Author$author =null) {returnnull ===$author ||empty($author->getFirstName());}, In case of compound entries without 'delete_empty' =>function ($author) {returnempty($author['firstName']);}, Thus, we avoid problems to use a new option |
murilolobato commentedMar 23, 2017
When the property gets renamed, I can add the test I've made in#22014 |
| $isNew = !isset($previousData[$name]); | ||
| $needRemove = $child->isEmpty() || (null !== $entryFilter && !$entryFilter($child->getData())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It is about whether the child form/data is empty or not, right?, so the previous nameisEmpty sounds better IMO ;)
691cc34 to4c0011eCompare| $isNew = !isset($previousData[$name]); | ||
| $isEmpty = $child->isEmpty() || true === $entryFilter($child->getData()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The$entryFilter and the empty closure function looks a bit overkill, can be simplified?
$isEmpty = $child->isEmpty() || (is_callable($this->deleteEmpty) && true === $this->deleteEmpty($child->getData());There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
changed
| )); | ||
| $this->assertTrue($form->has('0')); | ||
| $this->assertFalse($form->has('1')); | ||
| $this->assertEquals(array('s_first', 's_last'), $form[0]->getData()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
fixed
| foreach ($form as $name => $child) { | ||
| /* @var $child FormInterface */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This change should not be done in master.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
why not? it improves autocomplete
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Because this should target 2.7 :)
Should be rebased to 3.4 branch? |
@yceruto done |
| foreach ($form as $name => $child) { | ||
| /* @var $child FormInterface */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I agree this should go on a lower branch. Also, It'd rather be:
+ /** @var FormInterface $child */ foreach ($form as $name => $child) {- /* @var $child FormInterface */
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
ok, will open PR for that. Where can I find information about standards for inline@var phpdoc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
See PSR-5:https://github.com/php-fig/fig-standards/blob/master/proposed/phpdoc.md#3-definitions
About the foreach, though:
This Standard does not cover this specific instance as a foreach statement is not considered to be a "Structural Element" but a Control Flow statement.
but still:
It is RECOMMENDED to precede a "Structural Element" with a DocBlock where it is defined and not with each usage.
| $isNew = !isset($previousData[$name]); | ||
| $isEmpty = $child->isEmpty() || (is_callable($entryFilter) && true === $entryFilter($child->getData())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
If the callback was set, shouldn't it prevail over (or actually replace entirely) theisEmpty implementation?
- $isEmpty = $child->isEmpty() || (is_callable($entryFilter) && true === $entryFilter($child->getData()));+ $isEmpty = is_callable($entryFilter) ? $entryFilter($child->getData()) : $child->isEmpty();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
imho not. It will force us to duplicate standard checks inside callables
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm not convinced you need those standard checks to be run anyway. For instance, you'll already have to check for the data not being null inside your callable before further checks.
@ogizanagi updated again :), now all green |
| @@ -148,14 +148,15 @@ public function onSubmit(FormEvent $event) | |||
| throw new UnexpectedTypeException($data, 'array or (\Traversable and \ArrayAccess)'); | |||
| } | |||
| if ($this->deleteEmpty) { | |||
| $previousData = $event->getForm()->getData(); | |||
| if ($entryFilter = $this->deleteEmpty) { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I guess$entryFilter can be removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Why? This variable uses some lines bottom
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yep, but why not use$this->deleteEmpty directly? IMHO$this->deleteEmpty($child->getData()) is quite easy to understand too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
because$this->deleteEmpty() will call undefined methodResizeFormListener::deleteEmpty()
ogizanagiJul 24, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
and($this->deleteEmpty)($child->getData()) is only doable in PHP 7.0+
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yep, my bad ;)
Thank you@Koc. |
@Koc Can you submit a pull request to update the docs? Thank you |
…on. (Koc)This PR was merged into the 3.4 branch.Discussion----------[Form] Allow pass filter callback to delete_empty option.| Q | A| ------------- | ---| Branch? | master| Bug fix? | yes| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#13601,#13940,#22008,#22014| License | MIT| Doc PR | coming soonCommits-------8630abe [Form] Allow pass filter callback to delete_empty option.
Uh oh!
There was an error while loading.Please reload this page.