Skip to content
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] filter entity choicelist after hydration #14072

Open
DavidBadura opened this issue Mar 26, 2015 · 21 comments

Comments

Projects
None yet
8 participants
@DavidBadura
Copy link
Contributor

commented Mar 26, 2015

Is it possible to filter the choicelist after querying the entities?
I know that you can filter it with the query_builder option but in some case you can't filter in this way.
For example if you use "array" as column definition in your entity.

I find only bad way to resolve this problem:

$builder->add(
    'contact',
    'entity',
    array(
        'query_builder' => function (EntityRepository $repository) {
            return $repository->createQueryBuilder('c')
                ->where('c.roles LIKE :role')->setParameter('role', '%ROLE_CONTACT%');
        },
        'label'         => 'Contact',
        'class'         => 'AppBundle\Entity\User',
        'required'      => false
    )
)

Or you must implement your own loader.

I think it would be nice and easy to use if you have an option to filter the data after hydration.
Like this:

$builder->add(
    'contact',
    'entity',
    array(
        'choice_filter' => function ($user) {
            return $user->hasRole('ROLE_CONTACT');
        },
        'label'         => 'Contact',
        'class'         => 'AppBundle\Entity\User',
        'required'      => false
    )
)

As with #14050 choice_filter can be a callback or property path. And i think it should be add in ChoiceType so you can filter every choicelist.

I know you should use query_builder to filter the data in the database but in some case you don't have a choice.

@Tobion

This comment has been minimized.

Copy link
Member

commented Mar 26, 2015

Looks like an edge case that you can do in a custom type or with workaround like attr callable setting display:none. 👎

@tolry

This comment has been minimized.

Copy link

commented Mar 27, 2015

👍

I do not think of this as an edge case. If you want to filter entities using domain logic, which is implemented in php, this would be really useful. You could e.g. try and duplicate this logic in SQL, but then you would end up with duplicate code that isn't even easily detected as such.

In the example above, the problem with SQL filtering is, that the user roles are stored as an array in one column, therefore exact matching gets tricky. This is not that an uncommon thing to do.

@larsborn

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2015

+1 and totally agreeing with @tolry

@DavidBadura

This comment has been minimized.

Copy link
Contributor Author

commented Jun 17, 2015

this needs the form label 😉

@Tobion Tobion added the Form label Jun 17, 2015

@webmozart webmozart added the Feature label Jun 22, 2015

@webmozart

This comment has been minimized.

Copy link
Contributor

commented Jun 22, 2015

If this is implemented, this should probably be implemented in DoctrineChoiceLoader. We need to take care that the optimizations in loadValuesForChoices() and loadChoicesForValues() don't apply if a filter is set.

@HeahDude

This comment has been minimized.

Copy link
Member

commented Feb 12, 2016

A big 👍 for this !

This could be easily implemented globally in ChoiceType and used in all its inheritance, if the filter was applied when building the choice list.

The filter option could be a property path or a callable returning a boolean (false would reject the choice), so we could have something like :

// assuming we already have loaded choices for another purpice
$builder->add('choice_field', ChoiceType::class, array(
    'choices' => $loadedChoices,
    'filter_choices' => function ($choice) {
        return 'condition' === $choice->getCondition() || 0 === $choice->getSomeState();
    },
    // or
    'filter_choices' => 'condition',
    ...
);

and in ChoiceType :

private function createChoiceList(array $options)
{
    if (null !== $options['choice_loader']) {
        return $this->choiceListFactory->createListFromLoader(
            $options['choice_loader'],
            $options['choice_value']
        );
    }

    // Harden against NULL values (like in EntityType and ModelType)
    $choices = null !== $options['choices'] ? $options['choices'] : array();

+   if (!empty($choices) && null !== $options['filter_choices']) {
+      return $this->filterChoices($choices, $options['filter_choices'], $options['choice_value']);
+   }
+
    return $this->choiceListFactory->createListFromChoices($choices, $options['choice_value']);
}

+ /** @var callable|string $filter */
+ private function filterChoices(array $choices, $filter, $value)
+ {
+     if (is_string($filter) && !is_callable($filter)) {
+         $propertyPath = new PropertyPath($filter);
+         $accessor = new PropertyAccessor;
+         $filterCall = function ($choice) use ($accessor, $propertyPath) {
+             if (is_object($choice) || is_array($choice)) {
+                 return $accessor->getValue($choice, $propertyPath);
+             }
+         };
+     } elseif (is_callable($filter) {
+         $filterCall = $filter;
+     }
+
+     if (isset($filterCall)) {
+         $filteredChoices = array();
+         foreach($choices as $choice) {
+             if (call_user_func($filterCall, $choice)) {
+                 $filterChoices[] = $choice;
+             }
+         }
+     }
+
+     return $this->choiceListFactory->createListFromChoices(
+         (isset($filteredChoices) ? $filteredChoices : $choices),
+         $value
+     );
+ }

Filtering choices makes sense to me only if they are already loaded (in case of entity one should use a ChoiceType with loaded entities as value for choices options).
Using a custom choice_loader or EntityType which does use a custom loader should implement their own filter logic, could be overridden in that last case by query_builder option anyway.

Example of use case, in a controller you get some Event entities do some stuff with them and then want to add them as choices in a form to assign some manager to it but only if the event happens in the current week, code could look like :

// assuming you have a variable $events which holds all the entities
$builder = $this->createForm(PlanningType::class, new Planning)
        ->add('managers', EntityType::class, array('class' => Manager::class))
        ->add('events', ChoiceType::class, array(
            'choices' => $events,
            'filter_choices' => function ($event) {
                return new DateTime('next week') > $event->getLaunchDate();
            },
            // ...

However it could be easier to use a filter as well for manager for example with a property path 'isAvailableThisWeek', as some filter condition could not be acheive by a custom query builder since it may rely on dynamic properties and methods of a class which may not be persisted in database.

But this last idea could not be resolved as above because in ChoiceType::createChoiceList() there is a return on the execution of the loader itself returning a choice list so it cannot be managed by the filterChoices method which requires an initial array of choices not an instanciated choice list.

I guess it would be complex to tweak default choice loaders (including doctrine choice loader) so they can behave depending on this filter option.

@webmozart do you have any lead to engage this on choice loaders ?

@xabbuh

This comment has been minimized.

Copy link
Member

commented Feb 17, 2016

@HeahDude Afaik this proposal contradicts the cache performance improvements (you cannot cache the choices if the filter is some dynamic) that have been done recently, doesn't it?

@HeahDude

This comment has been minimized.

Copy link
Member

commented Feb 18, 2016

@xabbuh I may be wrong but my example uses pre-loaded choices on generic ChoiceType to narrow the array before creating the hash for the view, it does not impact choices loaded by a choice_loader as the DoctrineChoiceLoader where choice_listis returned earlier.

That's why I'm asking if someone has a guess about how to engage it on choice_loader as the original post is about an usage with EntityType. It may require to change the signature of ChoiceListFactory::createListFromLoader() to pass it the filter, doesn't it ?

@HeahDude

This comment has been minimized.

Copy link
Member

commented Feb 18, 2016

I can try to send a PR in the next days to discuss this implementation if you think it makes sense, but changing the signature is a BC break, isn't it ?

@xabbuh

This comment has been minimized.

Copy link
Member

commented Feb 28, 2016

@HeahDude Yeah, I think having a PR to discuss this is a good idea.

@HeahDude

This comment has been minimized.

Copy link
Member

commented Feb 28, 2016

@xabbuh Actually, I'm working on a global way to get it in CollectionType too ;)
But I may start there and handle ChoiceType later, there's a huge PR on its way...
Is the feature frozen state of 3.1 planned for the 1st march ?

@webmozart

This comment has been minimized.

Copy link
Contributor

commented Feb 29, 2016

@HeahDude Thanks for working on this :) Could you push your WIP PR so that we can double-check the approach and make sure you don't invest efforts into a solution that might turn out to be unacceptable for us?

@HeahDude

This comment has been minimized.

Copy link
Member

commented Feb 29, 2016

Hi @webmozart, I'm actually working on a CollectionList refactoring based on your approach of ChoiceType in #14050.

The plan is to include filter_collection or filter_entries in CollectionType in the process and if the check pass, we can add it to ChoiceType too.

I'll send a PR in the next hours based on your comment #13116 (comment)

You can see a preliminary work (that will be pushed later) with separated commits at my branch https://github.com/HeahDude/symfony/tree/feature-collection_type-refactoring

And a target prototype implementation at my other branch https://github.com/HeahDude/symfony/tree/feature-reindex-collection_type

I've also made a proto-doc as a gist here

Your feedback would be very appreciated, especially if we could get this in 3.1 :)

@webmozart

This comment has been minimized.

Copy link
Contributor

commented Feb 29, 2016

After quickly looking at this, I think this should be solved by adding a choice_filter option (name consistent with the other choice_* options) to DoctrineType. That option would enable a ChoiceLoader decorator that is wrapped around the choice loader before storing it in the cache. This way, even filtered choice lists would be properly cached.

I would not add the choice_filter option to ChoiceType, since one can simply use array_filter() before passing an array to the choices option.

@HeahDude

This comment has been minimized.

Copy link
Member

commented Feb 29, 2016

Maybe it could allow to rely on pre set filters in some from type classes using this option instead of managing it in controllers when the same ChoiceList may be cached and reused. What do you think ?

@webmozart

This comment has been minimized.

Copy link
Contributor

commented Feb 29, 2016

@HeahDude Can you give an example where that might be useful?

@HeahDude

This comment has been minimized.

Copy link
Member

commented Feb 29, 2016

Basic blog, an admin wants to create an article manage a list of articles, he needs many ChoiceType fields with the same entity collection but the choices available in each field depend on the states or the subclasses of the entities/choices.

@HeahDude

This comment has been minimized.

Copy link
Member

commented Mar 1, 2016

Considering #17968 and #17609, I propose to use two patterns for options in types managing nested types like ChoiceType or CollectionType :
item_passive or active_group which gives choice_*nested_option or handle_option*_choices.

For CollectionType we get entry_*nested or handle_entries or maybe handle_collection.

Some proposed options could be added to both types like filter_* and empty_*.

I think we better should cache the most complete collections and apply some filter option after in order to reuse loaded lists many times depending on dynamic forms and options for both ChoiceType and CollectionType.

To preserve BC we could add a filter via a setter.

I humbly suppose the use cases where you want to cache a more narrowed list might be edge cases (managed with loaders ?)

@webmozart

This comment has been minimized.

Copy link
Contributor

commented Mar 1, 2016

Reusing the same choice list and filtering it elsewhere violates the separation of concerns in the component. From the perspective of a choice field, the choice list contains all choices that are displayed and selectable. Any filtering, caching etc. should be done while loading the choice list. How to do that, what to cache etc. is of course a different topic. But it should not be done outside the ChoiceListFactory.

@HeahDude

This comment has been minimized.

Copy link
Member

commented Mar 1, 2016

Yes, understood.

My idea was to bind the logic the factory not to the form "lists" like ChoiceList.

It will be easier to talk about it on a WIP as you suggested :)

@yceruto

This comment has been minimized.

Copy link
Member

commented Sep 26, 2018

See another attempt to achieve it #28607

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.