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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from Jul 26, 2017

Conversation

Projects
None yet
9 participants
@Koc
Contributor

Koc commented Nov 11, 2016

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!

@Koc

This comment has been minimized.

Show comment
Hide comment
@Koc

Koc Nov 12, 2016

Contributor

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

Contributor

Koc commented Nov 12, 2016

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

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

@Koc

This comment has been minimized.

Show comment
Hide comment
@Koc

Koc Dec 26, 2016

Contributor

can anybody review and merge?

Contributor

Koc commented Dec 26, 2016

can anybody review and merge?

@HeahDude

This comment has been minimized.

Show comment
Hide comment
@HeahDude

HeahDude Dec 26, 2016

Member

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

Status: Reviewed

Member

HeahDude commented Dec 26, 2016

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

Status: Reviewed

@xabbuh

This comment has been minimized.

Show comment
Hide comment
@xabbuh

xabbuh Feb 8, 2017

Member

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?

Member

xabbuh commented Feb 8, 2017

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

This comment has been minimized.

Show comment
Hide comment
@Koc

Koc Feb 8, 2017

Contributor

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

Contributor

Koc commented Feb 8, 2017

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

@xabbuh

This comment has been minimized.

Show comment
Hide comment
@xabbuh

xabbuh Feb 8, 2017

Member

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

Member

xabbuh commented Feb 8, 2017

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

@murilolobato

This comment has been minimized.

Show comment
Hide comment
@murilolobato

murilolobato Mar 17, 2017

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

murilolobato commented Mar 17, 2017

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

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Mar 22, 2017

Member

@Koc What about @xabbuh suggestion for the name?

Member

fabpot commented Mar 22, 2017

@Koc What about @xabbuh suggestion for the name?

@Koc

This comment has been minimized.

Show comment
Hide comment
@Koc

Koc Mar 23, 2017

Contributor

@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.

Contributor

Koc commented Mar 23, 2017

@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.

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

@yceruto

This comment has been minimized.

Show comment
Hide comment
@yceruto

yceruto Mar 23, 2017

Contributor

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.

Contributor

yceruto commented Mar 23, 2017

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

This comment has been minimized.

Show comment
Hide comment
@murilolobato

murilolobato Mar 23, 2017

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

murilolobato commented Mar 23, 2017

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

@Koc

This comment has been minimized.

Show comment
Hide comment
@Koc

Koc Mar 23, 2017

Contributor

@yceruto interesting idea, will implement it tomorrow

Contributor

Koc commented Mar 23, 2017

@yceruto interesting idea, will implement it tomorrow

@Koc Koc changed the title from [Form] Added entry_filter option to collection type. to [Form] Allow pass filter callback to delete_empty option. Mar 24, 2017

@yceruto

This comment has been minimized.

Show comment
Hide comment
@yceruto

yceruto Jun 4, 2017

Contributor

Should be rebased to 3.4 branch?

Contributor

yceruto commented Jun 4, 2017

Should be rebased to 3.4 branch?

@Koc Koc changed the base branch from master to 3.4 Jul 23, 2017

@Koc

This comment has been minimized.

Show comment
Hide comment
@Koc

Koc Jul 23, 2017

Contributor

@yceruto done
@ogizanagi please review

Contributor

Koc commented Jul 23, 2017

@yceruto done
@ogizanagi please review

@Koc

This comment has been minimized.

Show comment
Hide comment
@Koc

Koc Jul 23, 2017

Contributor

@ogizanagi updated again :), now all green

Contributor

Koc commented Jul 23, 2017

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

This comment has been minimized.

@yceruto

yceruto Jul 23, 2017

Contributor

I guess $entryFilter can be removed?

@yceruto

yceruto Jul 23, 2017

Contributor

I guess $entryFilter can be removed?

This comment has been minimized.

@Koc

Koc Jul 24, 2017

Contributor

Why? This variable uses some lines bottom

@Koc

Koc Jul 24, 2017

Contributor

Why? This variable uses some lines bottom

This comment has been minimized.

@yceruto

yceruto Jul 24, 2017

Contributor

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

@yceruto

yceruto Jul 24, 2017

Contributor

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

This comment has been minimized.

@Koc

Koc Jul 24, 2017

Contributor

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

@Koc

Koc Jul 24, 2017

Contributor

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

This comment has been minimized.

@ogizanagi

ogizanagi Jul 24, 2017

Member

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

@ogizanagi

ogizanagi Jul 24, 2017

Member

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

This comment has been minimized.

@yceruto

yceruto Jul 24, 2017

Contributor

Yep, my bad ;)

@yceruto

yceruto Jul 24, 2017

Contributor

Yep, my bad ;)

@xabbuh

xabbuh approved these changes Jul 25, 2017

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Jul 26, 2017

Member

Thank you @Koc.

Member

fabpot commented Jul 26, 2017

Thank you @Koc.

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

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details
@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Jul 26, 2017

Member

@Koc Can you submit a pull request to update the docs? Thank you

Member

fabpot commented Jul 26, 2017

@Koc Can you submit a pull request to update the docs? Thank you

fabpot added a commit that referenced this pull request Jul 26, 2017

feature #20496 [Form] Allow pass filter callback to delete_empty opti…
…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.

This was referenced Oct 18, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment