Add SQLFilter parameter in the configuration for Doctrine ORM

[yoannch]

Hello there,

In doctrine 2.2, a new feature was added : sql filters made by @asm89 => doctrine/orm#210.

But the doctrinebundle seems not to enable this in the configuration, so I'd like to add it.

Maybe we could add it as follow, in the config.yml :

doctrine:

    orm:
        filters:
            my_first_filter:
                class: 'Fully\Qualified\Class\Name'
                enabled: true
            my_second_filter:
                class: 'Fully\Qualified\Class\Name2'
                enabled: true

and so modify the Configuration and DoctrineExtension classes to enable this?

What do you think about this?

[armetiz]

Good proposition.
enabled booleanNode should have "true" as default value.

[beberlei]

I think filters should be disabled by default, otherwise the proposition is good. You can implement this and send a PR.

Whats your take @asm89 ?

[Burgov]

exactly how could you force the filter to be abled using the container builder?

right now i'm trying this in a compiler pass, which i could port to this bundle once I get it working

        $container->getDefinition('doctrine.orm.default_configuration')
            ->addMethodCall('addFilter', array('filtername', 'classname'))
        ;
        $container->getDefinition('doctrine.orm.default_entity_manager')
            ->addMethodCall('getFilters', array())
            ->addMethodCall('enable', array('filtername'))
        ;

but that doesn't result in the desired code in the container class

        $instance->getFilters();
        $instance->enable('filtername');

rather than

        $instance->getFilters()->enable('filtername');

[asm89]

Looks fine imo. I agree with @beberlei that filters should be disabled by default, because I think there are a lot of use cases where filters need additional parameters in order to work properly.

[yoannch]

I agree with @beberlei and @asm89 : this boolean should have a false default value.

@Burgov I'm investigating right now on how to do that too, if you have any suggestions or ideas, feel free to mail me.

[Burgov]

I couldn't think of a way without changing Doctrine ORM code, for example a third optional boolean argument to addFilter, or an en-/disableFilter method on the entitymanager...

[stof]

@Burgov I can look at it at the end of the week if you have issues implementing it

[Burgov]

@stof that'd be great. I'd write a PR if I knew how to do this. Unfortunately I'll be out of the country next week, so I won't be able to make any PR's soon

[yoannch]

@stof did you have some time to explore this issues? I'm still blocking on the same issue as @Burgov ... if you have any tips, you're welcome!

[stof]

I had no time at all for the Symfony2 community this weekend.

[yoannch]

I found a solution! I implement it now, make tests and send you the PR as all tests will pass.

I just have a question about the configuration.

I first made a proposal as follow :

doctrine:
    ...
    orm:
        filters:
            my_first_filter:
                class: 'Fully\Qualified\Class\Name'
                enabled: true
            my_second_filter:
                class: 'Fully\Qualified\Class\Name2'
                enabled: true

I think that will work for the default entity manager, but if someone has different entity managers, he may want to have different filters for each of his entity managers right?

So I was thinking of the following conf :

doctrine:
    ...
    orm:
        filters:
            my_first_filter:
                class: 'Fully\Qualified\Class\Name'
                enabled: true
            my_second_filter:
                class: 'Fully\Qualified\Class\Name2'
                enabled: true
        entity_managers:
            em1:
                filters:
                    my_em1_filter:
                        class: 'Fully\Qualified\Class\Name'
                        enable: true
            em2:
                filters:
                    my_first_em2_filter:
                        class: 'Fully\Qualified\Class\Name'
                        enabled: true
                    my_scnd_em2_filter:
                        class: 'Fully\Qualified\Class\Name'
                        enabled: true

or do you think the following conf will be better :

doctrine:
    ...
    orm:
        entity_managers:
            default:
                filters:
                    my_first_filter:
                        class: 'Fully\Qualified\Class\Name'
                        enabled: true
                    my_second_filter:
                        class: 'Fully\Qualified\Class\Name2'
                        enabled: true
            em1:
                filters:
                    my_em1_filter:
                        class: 'Fully\Qualified\Class\Name'
                        enable: true
            em2:
                filters:
                    my_first_em2_filter:
                        class: 'Fully\Qualified\Class\Name'
                        enabled: true
                    my_scnd_em2_filter:
                        class: 'Fully\Qualified\Class\Name'
                        enabled: true

?

[stof]

it should be done at the em level, like for DQL functions

[stof]

so your last example (and the first one will correspond to the short syntax which is rewritten)

[yoannch]

Ok, thanks for your advice. So I implement it to handle both the last example and the short syntax one (the first I gave).

[stof]

No, you implement the last example and you add the needed key in the list of rewritten keys for the short syntax. Look at how it is handled

[stof]

@beberlei @asm89 does it make sense to give the possibility to enable a sql filter through the config ? It would assume that the sql filter can work without any parameters.

[asm89]

Yes. For example a SoftDeleteFilter could query for and <alias>.deleted = 0. There is no need to configure the parameters.

[stof]

I finally took some time (over my sleeping time obviously) to implement it

[Burgov]

Very nice! Need to remember these configurator classes, I can imagine it'll come in handy more often.

[stof]

@Burgov in most cases, they are overkill. The best way to see this is that I never saw a use case for them until yesterday

[julesbou]

@stof Thanks for your time, for my part i've always had a big use case for this feature, restrict user_id on a multi-user product (SAAS).

[yoannch]

@stof thank you very much for your time, I've been (and still am) overloaded theses weeks, and couldn't deal with it.