SQLFilters are too locked down #6382

New Issue

admin · 2026-01-22T15:32:05+01:00

admin commented

2026-01-22 15:32:05 +01:00

Originally created by @oojacoboo on GitHub (Jan 13, 2020).

I created a pull request for an issue with SQLFilters related to the EntityManager, which is a dependency of the SQLFilter class but not exposed to child classes. However, I'm now faced with another issue related to SQLFilters that is more of the result of the same mentality that continues to haunt me with Doctrine.

I'm certain this won't be met with open arms, because it's honestly just an disagreement with approach. But everything in Doctrine makes the assumption that developers using the library do not know what they're doing and do not need to extend it freely.

Situation:

I need to create an SQLFilter that reads annotations. Due to performance implications, I need to be able to used the CachedReader, as the annotations are already cached, and being that it's a filter which is applied on every query, using the standard AnnotationReader without cache, isn't really sufficient. The performance hit is real.

Problem:

Filters are locked down tight. You do not have access to the constructor and the filters, after being constructed, are not easily accessible. So, adding dependencies is a bit more of a challenge, and nasty, and prone to future BC breaks.

$entityManager->getFilters()->getEnabledFilters()['multi-tenancy']->setAnnotationReader($cachedAnnotationReader);

That is really nasty. It shouldn't be. I also should be able to write filters and use this lib as needed for proper implementation. Without SQLFitlers, this ORM simply wouldn't work. We need proper multi-tenancy support, which needs to be applied via Filters for relational entity access, etc. Annotations are the most logical way of handling this.

I'm requesting that SQLFilter design be re-visited. I'd also encourage a bit of a change in mentality towards locking down the library, but I realize that's just a difference in opinion and most likely unwelcomed.

Originally created by @oojacoboo on GitHub (Jan 13, 2020). I created a pull request for an issue with SQLFilters related to the EntityManager, which is a dependency of the `SQLFilter` class but not exposed to child classes. However, I'm now faced with another issue related to SQLFilters that is more of the result of the same mentality that continues to haunt me with Doctrine. I'm certain this won't be met with open arms, because it's honestly just an disagreement with approach. But everything in Doctrine makes the assumption that developers using the library do not know what they're doing and do not need to extend it freely. Situation: I need to create an `SQLFilter` that reads annotations. Due to performance implications, I need to be able to used the `CachedReader`, as the annotations are already cached, and being that it's a filter which is applied on every query, using the standard `AnnotationReader` without cache, isn't really sufficient. The performance hit is real. Problem: Filters are locked down tight. You do not have access to the constructor and the filters, after being constructed, are not easily accessible. So, adding dependencies is a bit more of a challenge, and nasty, and prone to future BC breaks. ```php $entityManager->getFilters()->getEnabledFilters()['multi-tenancy']->setAnnotationReader($cachedAnnotationReader); ``` That is really nasty. It shouldn't be. I also should be able to write filters and use this lib as needed for proper implementation. Without SQLFitlers, this ORM simply wouldn't work. We need proper multi-tenancy support, which needs to be applied via Filters for relational entity access, etc. Annotations are the most logical way of handling this. I'm requesting that SQLFilter design be re-visited. I'd also encourage a bit of a change in mentality towards locking down the library, but I realize that's just a difference in opinion and most likely unwelcomed.

admin added the Improvement label 2026-01-22 15:32:05 +01:00

admin commented

2026-01-22 15:32:05 +01:00

@SenseException commented on GitHub (Jan 16, 2020):

Hi @oojacoboo, can you please evaluate in which direction you want to see the filters go? Maybe you can provide a code-example for showing what you describe.
Also: Can you please explain in an code example what your custom filter is doing? For now I understand that you'e unhappy with the lack of using a constructor.

@SenseException commented on GitHub (Jan 16, 2020): Hi @oojacoboo, can you please evaluate in which direction you want to see the filters go? Maybe you can provide a code-example for showing what you describe. Also: Can you please explain in an code example what your custom filter is doing? For now I understand that you'e unhappy with the lack of using a constructor.

admin commented

2026-01-22 15:32:06 +01:00

@oojacoboo commented on GitHub (Jan 16, 2020):

Hey @SenseException. I've published the extension. Maybe it'd be helpful to have a look over the Filter class there.

https://github.com/rentpost/doctrine-multi-tenancy

As you can see, there are a number of hacks. Efforts to eliminate all those hacks would be my recommendation.

Access to the constructor is definitely a start
Access to the EntityManager
A better way of managing state
Ability to use the cached annotation reader

@oojacoboo commented on GitHub (Jan 16, 2020): Hey @SenseException. I've published the extension. Maybe it'd be helpful to have a look over the Filter class there. https://github.com/rentpost/doctrine-multi-tenancy As you can see, there are a number of hacks. Efforts to eliminate all those hacks would be my recommendation. - Access to the constructor is definitely a start - Access to the EntityManager - A better way of managing state - Ability to use the cached annotation reader

admin commented

2026-01-22 15:32:07 +01:00

@kgilden commented on GitHub (Feb 14, 2022):

I think "too locked down" might be a bit too harsh, but I'd also like to add to this a couple of observations.

The constructor is locked down, because currently adding filters is done by passing their FQCN to Configuration::addFilter().
Enabling and disabling filters cause them to lose their state. For example, if I set a parameter, then disable it and re-enable, then the parameter is gone and I get a new instance from FilterCollection::getFilter().

So in a nutshell, it would already make using filters really powerful, if we could pass instances instead of class names and if they would stay instantiated through the enable-disable cycle. I'm sure there are really good reasons for doing it the current way, so maybe someone more knowledgeable could elaborate a bit.

/cc @SenseException

@kgilden commented on GitHub (Feb 14, 2022): I think "too locked down" might be a bit too harsh, but I'd also like to add to this a couple of observations. * The constructor is locked down, because currently adding filters is done by passing their FQCN to `Configuration::addFilter()`. * Enabling and disabling filters cause them to lose their state. For example, if I set a parameter, then disable it and re-enable, then the parameter is gone and I get a new instance from `FilterCollection::getFilter()`. So in a nutshell, it would already make using filters really powerful, if we could pass instances instead of class names and if they would stay instantiated through the enable-disable cycle. I'm sure there are really good reasons for doing it the current way, so maybe someone more knowledgeable could elaborate a bit. /cc @SenseException

admin commented

2026-01-22 15:32:07 +01:00

@greg0ire commented on GitHub (Feb 14, 2022):

This seems related: https://github.com/doctrine/orm/pull/434

@greg0ire commented on GitHub (Feb 14, 2022): This seems related: https://github.com/doctrine/orm/pull/434

Sign in to join this conversation.

Branches Tags

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: doctrine/archived-orm#6382