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

Support filter parameters in Configuration #1077

Open
wants to merge 2 commits into
base: old-prototype-3.x
Choose a base branch
from

Conversation

jmikola
Copy link
Member

@jmikola jmikola commented Jul 1, 2014

Based on work done in doctrine/mongodb-odm#908

My understanding is that this makes it easier to setup filters at boot time, instead of having to fetch them from the collection later on.

For added context, the related PR from MongoDB ODM's Symfony bundle is doctrine/DoctrineMongoDBBundle#255

@doctrinebot
Copy link

Hello,

thank you for creating this pull request. I have automatically opened an issue
on our Jira Bug Tracker for you. See the issue link:

http://www.doctrine-project.org/jira/browse/DDC-3200

We use Jira to track the state of pull requests and the versions they got
included in.

{
$this->_attributes['filters'][$name] = $className;
$this->_attributes['filters'][$name] = array(
'className' => $className,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would just use class

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I concur. Will make the change in ODM as well.

@Koc
Copy link
Contributor

Koc commented Jul 1, 2014

@jmikola what about adding possibility to disable/attaching filter per query?

@stof
Copy link
Member

stof commented Jul 1, 2014

@Koc you can already enable or disable filters by using the FilterCollection.

the goal of this PR is to preset some filter parameters in the config

@Koc
Copy link
Contributor

Koc commented Jul 1, 2014

Of course this PR has other goal. Just if Jeremy starts working on improvements of filters maybe he can add some changes.

By the way, code like

$em->getFilters()->disable('ololo_filter');
// my query
$em->getFilters()->enable('ololo_filter');

looks ugly for me. And sometimes it non-acceptable. When you building dynamic query in several application layers, for example.

@jmikola
Copy link
Member Author

jmikola commented Jul 1, 2014

@Koc: I have no plans to make additional filter improvements (too much on my plate already). This PR is just to ensure that ORM and ODM remain in sync.

/cc @lsmith77 in case you'd like this for PHPCR ODM.

@lsmith77
Copy link
Member

lsmith77 commented Jul 1, 2014

i dont think we have filters at all in PHPCR ODM

/cc @dbu

@dbu
Copy link
Member

dbu commented Jul 2, 2014

no, we do not have filters in the SQL2 part of phpcr-odm. sounds like a potentially interesting feature eventually.

*/
public function addFilter($name, $className)
public function addFilter($name, $className, array $parameters = array())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the correct solution here would be to pass in a new instance of a filter, which also removes this auto-instantiation thing, but there's also the fact that filters should not be THAT flexible. Ping @asm89

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Ocramius: I don't see how we could do that without breaking BC, though. Unless $className was changed to conditionally accept an object (in which FilterCollection only needs getFilterClassName() for times when an object wasn't provided).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need @asm89 to look at it to know why this was designed this way in first place

@Ocramius
Copy link
Member

@asm89 ping?

@hanovruslan
Copy link

Sorry folks, i know this is not proper place for my issue but ...
i found that disabled filter with auto mapping also disabled causes error for this filter's parameters

doctrine:
    dbal:
        driver: %database_driver%
        host: %database_host%
        port: %database_port%
        dbname: %database_name%
        user: %database_user%
        password: %database_password%
        charset: %database_charset%
        server_version: %database_server_version%
        options:
            20: true #PDO::ATTR_EMULATE_PREPARES
    orm:
        auto_generate_proxy_classes: %kernel.debug%
        default_entity_manager: default
        entity_managers:
            default:
                metadata_cache_driver: apc
                query_cache_driver: apc
                result_cache_driver: apc
                naming_strategy: doctrine.orm.naming_strategy.underscore
                filters:
                    some_filter:
                        class: SomeNameSpace\SomeFilter
                        enabled: false
                        parameters:
                            name: value

will throw

\InvalidArgumentException("Parameter '" . $name . "' does not exist.");

for enabled by default filter it is ok

doctrine:
#
    orm:
#
        entity_managers:
            default:
#
                filters:
                    some_filter:
#
                        enabled: true

should i make PR with specific tests for this case or what?

Thanks!

@hanovruslan
Copy link

Sorry, any progress on this ?

Base automatically changed from master to old-prototype-3.x February 23, 2021 08:18
@rehman20281

This comment has been minimized.

@lsmith77

This comment has been minimized.

@rehman20281

This comment has been minimized.

@derrabus
Copy link
Member

Please use https://github.com/doctrine/orm/discussions for support questions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet