[Form] Allow pass filter callback to delete_empty option. #20496

Koc · 2016-11-11T22:31:53Z

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 soon

HeahDude

I like this, thanks for your proposal!

HeahDude · 2016-11-11T23:41:25Z

src/Symfony/Component/Form/Extension/Core/EventListener/ResizeFormListener.php

                $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)) {


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 again if ($isEmpty && ($isNew || $this->allowDelete)) {

HeahDude · 2016-11-11T23:43:47Z

src/Symfony/Component/Form/Extension/Core/Type/CollectionType.php

@@ -97,6 +98,7 @@ public function configureOptions(OptionsResolver $resolver)
            'entry_type' => __NAMESPACE__.'\TextType',
            'entry_options' => array(),
            'delete_empty' => false,
+            'is_empty_callback' => null,


Please add:

$resolver->setAllowedTypes('is_empty_callback', array('null', 'callable'));

Koc · 2016-11-12T10:55:42Z

@HeahDude done. Also I've changed the test for better description of this new feature.

Koc · 2016-12-26T10:50:15Z

can anybody review and merge?

HeahDude · 2016-12-26T11:04:05Z

Some may have some concern about the naming of this option, for me it's a 👍

Status: Reviewed

xabbuh · 2017-02-08T11:21:42Z

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?

Koc · 2017-02-08T11:29:05Z

give some examples of a better name, I'll rename it

xabbuh · 2017-02-08T17:01:05Z

What about something like filter_entry as we are basically filter our collection entries inside the callback?

murilolobato · 2017-03-17T03:25:20Z

If is_empty_callback is a option, I would rename it to just is_empty.

fabpot · 2017-03-22T17:57:45Z

@Koc What about @xabbuh suggestion for the name?

Koc · 2017-03-23T11:26:25Z

@fabpot done, except I prefer entry_filter to be consistent with other options like entry_type/entry_options. Callable should return true for leave entry in collection.

Test failure is unrelated.

yceruto · 2017-03-23T16:01:32Z

IMHO, the option should be the same delete_empty, accepting bool or callable. Use true to delete empty (non-compound) entries, and use a callable function to delete empty (compound with or without data_class) entries like Author(null) or array('firstName' => '').

In case of compound entries with data_class:

'delete_empty' => function (Author $author = null) {
    return null === $author || empty($author->getFirstName());
},

In case of compound entries without data_class:

'delete_empty' => function ($author) {
    return empty($author['firstName']);
},

Thus, we avoid problems to use a new option entry_filter (learning curve!!) which will depend of delete_empty = true also.

murilolobato · 2017-03-23T16:14:29Z

When the property gets renamed, I can add the test I've made in #22014

yceruto · 2017-03-23T16:28:17Z

src/Symfony/Component/Form/Extension/Core/EventListener/ResizeFormListener.php

                $isNew = !isset($previousData[$name]);
+                $needRemove = $child->isEmpty() || (null !== $entryFilter && !$entryFilter($child->getData()));


It is about whether the child form/data is empty or not, right?, so the previous name isEmpty sounds better IMO ;)

yceruto · 2017-03-24T12:34:45Z

src/Symfony/Component/Form/Extension/Core/EventListener/ResizeFormListener.php

                $isNew = !isset($previousData[$name]);
+                $isEmpty = $child->isEmpty() || true === $entryFilter($child->getData());


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());

yceruto · 2017-03-24T12:56:34Z

src/Symfony/Component/Form/Tests/Extension/Core/Type/CollectionTypeTest.php

+        ));
+        $this->assertTrue($form->has('0'));
+        $this->assertFalse($form->has('1'));
+        $this->assertEquals(array('s_first', 's_last'), $form[0]->getData());


Missing keys https://travis-ci.org/symfony/symfony/jobs/214628660#L3215

HeahDude · 2017-04-09T13:06:31Z

src/Symfony/Component/Form/Extension/Core/EventListener/ResizeFormListener.php

            foreach ($form as $name => $child) {
+                /* @var $child FormInterface */


This change should not be done in master.

why not? it improves autocomplete

Because this should target 2.7 :)

yceruto · 2017-06-04T20:25:48Z

Should be rebased to 3.4 branch?

Koc · 2017-07-23T00:09:31Z

@yceruto done
@ogizanagi please review

ogizanagi · 2017-07-23T08:50:52Z

src/Symfony/Component/Form/Extension/Core/EventListener/ResizeFormListener.php

            foreach ($form as $name => $child) {
+                /* @var $child FormInterface */


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 */

ok, will open PR for that. Where can I find information about standards for inline @var phpdoc?

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.

ogizanagi · 2017-07-23T08:56:19Z

src/Symfony/Component/Form/Extension/Core/EventListener/ResizeFormListener.php

                $isNew = !isset($previousData[$name]);
+                $isEmpty = $child->isEmpty() || (is_callable($entryFilter) && true === $entryFilter($child->getData()));


If the callback was set, shouldn't it prevail over (or actually replace entirely) the isEmpty implementation?

- $isEmpty = $child->isEmpty() || (is_callable($entryFilter) && true === $entryFilter($child->getData())); + $isEmpty = is_callable($entryFilter) ? $entryFilter($child->getData()) : $child->isEmpty();

imho not. It will force us to duplicate standard checks inside callables

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.

Koc · 2017-07-23T10:09:11Z

@ogizanagi updated again :), now all green

yceruto · 2017-07-23T23:24:08Z

src/Symfony/Component/Form/Extension/Core/EventListener/ResizeFormListener.php

@@ -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) {


I guess $entryFilter can be removed?

Why? This variable uses some lines bottom

Yep, but why not use $this->deleteEmpty directly? IMHO $this->deleteEmpty($child->getData()) is quite easy to understand too.

because $this->deleteEmpty() will call undefined method ResizeFormListener::deleteEmpty()

and ($this->deleteEmpty)($child->getData()) is only doable in PHP 7.0+

Yep, my bad ;)

fabpot · 2017-07-26T06:57:33Z

Thank you @Koc.

fabpot · 2017-07-26T06:57:48Z

@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 soon Commits ------- 8630abe [Form] Allow pass filter callback to delete_empty option.

carsonbot added Status: Needs Review Form Bug Feature labels Nov 11, 2016

HeahDude suggested changes Nov 11, 2016

View reviewed changes

Koc force-pushed the form-is-empty-callback branch 3 times, most recently from d542c63 to c3e6ff5 Compare November 12, 2016 10:50

nicolas-grekas added this to the 3.x milestone Dec 6, 2016

carsonbot added Status: Reviewed and removed Status: Needs Review labels Dec 26, 2016

HeahDude mentioned this pull request Mar 16, 2017

[Form] delete_empty don't work for embedded forms without data_class #22008

Closed

Koc force-pushed the form-is-empty-callback branch 4 times, most recently from d9703d6 to cbd5707 Compare March 23, 2017 11:14

Koc force-pushed the form-is-empty-callback branch from cbd5707 to 57a2aa5 Compare March 23, 2017 12:54

Koc changed the title ~~[Form] Added is_empty_callback option to collection type.~~ [Form] Added entry_filter option to collection type. Mar 23, 2017

yceruto reviewed Mar 23, 2017

View reviewed changes

Koc force-pushed the form-is-empty-callback branch 3 times, most recently from 691cc34 to 4c0011e Compare March 24, 2017 12:31

yceruto reviewed Mar 24, 2017

View reviewed changes

HeahDude reviewed Apr 9, 2017

View reviewed changes

Koc force-pushed the form-is-empty-callback branch from 4c0011e to 3b7f11a Compare April 9, 2017 21:18

Koc force-pushed the form-is-empty-callback branch from 3b7f11a to ff51ad8 Compare July 23, 2017 00:00

Koc changed the base branch from master to 3.4 July 23, 2017 00:00

Koc force-pushed the form-is-empty-callback branch from ff51ad8 to 8b49f00 Compare July 23, 2017 00:03

ogizanagi reviewed Jul 23, 2017

View reviewed changes

Koc force-pushed the form-is-empty-callback branch from 8b49f00 to e697559 Compare July 23, 2017 09:51

[Form] Allow pass filter callback to delete_empty option.

8630abe

Koc force-pushed the form-is-empty-callback branch from e697559 to 8630abe Compare July 23, 2017 09:57

ogizanagi approved these changes Jul 23, 2017

View reviewed changes

yceruto reviewed Jul 23, 2017

View reviewed changes

yceruto approved these changes Jul 24, 2017

View reviewed changes

xabbuh approved these changes Jul 25, 2017

View reviewed changes

fabpot merged commit 8630abe into symfony:3.4 Jul 26, 2017

HeahDude mentioned this pull request Jul 31, 2017

Document "delete_empty" CollectionType option as callable symfony/symfony-docs#8244

Closed

This was referenced Oct 18, 2017

Release v3.4.0-BETA1 #24609

Merged

Release v4.0.0-BETA1 #24612

Merged

yceruto mentioned this pull request Jul 30, 2018

[Form] delete_empty & checkbox #13747

Closed

Koc deleted the form-is-empty-callback branch July 22, 2019 12:08

		$isNew = !isset($previousData[$name]);
		$needRemove = $child->isEmpty() \|\| (null !== $entryFilter && !$entryFilter($child->getData()));

		$isNew = !isset($previousData[$name]);
		$isEmpty = $child->isEmpty() \|\| true === $entryFilter($child->getData());

		foreach ($form as $name => $child) {
		/* @var $child FormInterface */

		$isNew = !isset($previousData[$name]);
		$isEmpty = $child->isEmpty() \|\| (is_callable($entryFilter) && true === $entryFilter($child->getData()));

[Form] Allow pass filter callback to delete_empty option. #20496

[Form] Allow pass filter callback to delete_empty option. #20496

Conversation

Koc commented Nov 11, 2016 • edited Loading

HeahDude left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Koc commented Nov 12, 2016 • edited Loading

Koc commented Dec 26, 2016

HeahDude commented Dec 26, 2016

xabbuh commented Feb 8, 2017

Koc commented Feb 8, 2017

xabbuh commented Feb 8, 2017

murilolobato commented Mar 17, 2017

fabpot commented Mar 22, 2017

Koc commented Mar 23, 2017

yceruto commented Mar 23, 2017 • edited Loading

murilolobato commented Mar 23, 2017

yceruto Mar 23, 2017 • edited Loading

Choose a reason for hiding this comment

yceruto Mar 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yceruto commented Jun 4, 2017

Koc commented Jul 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Koc commented Jul 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ogizanagi Jul 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot commented Jul 26, 2017

fabpot commented Jul 26, 2017

Koc commented Nov 11, 2016 •

edited

Loading

Koc commented Nov 12, 2016 •

edited

Loading

yceruto commented Mar 23, 2017 •

edited

Loading

yceruto Mar 23, 2017 •

edited

Loading

yceruto Mar 24, 2017 •

edited

Loading

ogizanagi Jul 24, 2017 •

edited

Loading