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

Filter factory added #1453

Closed
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@TomasVotruba
Contributor

TomasVotruba commented Jul 8, 2015

This proposal is to allow DI/IOC support for filters.
Based on #1129, resp. #1129 (comment)

I don't have Doctrine coding standard under my skin yet, so feel free to comment as well.
I look forward for any feedback.

@doctrinebot

This comment has been minimized.

doctrinebot commented Jul 8, 2015

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-3813

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

public function createFromName($name)
{
$filterClass = $this->ormConfiguration->getFilterClassName($name);
return new $filterClass($this->entityManager);

This comment has been minimized.

@krixon

krixon Jul 9, 2015

Given that an EntityManagerInterface instance is required when constructing the filter, doesn't it make more sense to require that the EM be provided to createFromName() rather than set separately?

Currently there is nothing here which ensures that the EM is set before trying to use it, and I don't see any reason for this class to hold a reference to the EM itself. It also means other implementations of the FilterFactory are possible, because there is no special treatment of DefautlFilterFactory in terms of receiving the EM.

This comment has been minimized.

@TomasVotruba

TomasVotruba Jul 9, 2015

Contributor

In such case, you'd have to modify FilterFactoryInterface interface, which would disallow dependency inversion.

Also this factory aims to future compatibility with DI approach. Proper solution would be to pass it via constructor, but I could figure out how to do that. I'd appreciate suggestion better location for instantiating DefaultFilterFactory. Setter or method argument is just workaround.

This comment has been minimized.

@krixon

krixon Jul 10, 2015

I can't see how you'd be able to use constructor injection without running into circular dependency issues since the FilterFactory is part of the Configuration which is provided to the EM.

I suppose you could take the same approach as is used for changing the ClassMetadataFactory - passing the class name into the EM rather than an object and having the EM instantiate it, passing itself to the FilterFactory constructor. I'm not a fan of this approach personally though as it means you can't have other dependencies in your factory.

Personally I think I would favour having the EM pass itself to the Factory via createFromName rather than having the Factory be permanently tied to a specific EM. You might want to share the same Factory for multiple EMs for example. If you want to create a factory for each EM rather than sharing them, you'd just set up your DI container to generate the EM Configuration objects accordingly.

This comment has been minimized.

@TomasVotruba

TomasVotruba Jul 10, 2015

Contributor

I can't see how you'd be able to use constructor injection without running into circular dependency issues since the FilterFactory is part of the Configuration which is provided to the EM.

Me neither, that's why used setter (usual solution to circular dependencies).

Personally I think I would favour having the EM pass itself to the Factory via createFromName rather than having the Factory be permanently tied to a specific EM. You might want to share the same Factory for multiple EMs for example. If you want to create a factory for each EM rather than sharing them, you'd just set up your DI container to generate the EM Configuration objects accordingly.

This would force FilterFactoryInterface to use EM even when not needed.

This comment has been minimized.

@krixon

krixon Jul 10, 2015

Isn't the EM always going to be needed? It needs to be passed to the filter itself even if the factory doesn't use it directly.

This comment has been minimized.

@krixon

krixon Jul 13, 2015

That is what I meant yes. I would argue that there is a contract. Various parts of doctrine expect to be able to call addFilterConstraint, and this is only defined as an abstract method on SQLFilter. For example, FilterCollection has return type annotations which specify that SQLFilter instances are returned.

If it's the case that the EM is not required for some filters, it would be cleaner imo to create a FilterInterface which just specifies addFilterConstraint - however any time you want to use parameters you will need the EM in order to quote them so this still doesn't solve the core problem that an EM is often required.

This comment has been minimized.

@TomasVotruba

TomasVotruba Jul 14, 2015

Contributor

@krixon

  1. Thanks for this idea, I'm really happy for it. Actually I already use it in my package, with purpose to provide contract. Just forgot it :)

SQLFilter is weak contract = breakable = no contract at all. FilterInterface might be the solution we look for.

  1. Setting EM for DefaultFilterFactory only would be ok then?

This comment has been minimized.

@TomasVotruba

TomasVotruba Jul 17, 2015

Contributor

FilterInterface added /cc @krixon @Ocramius

Any further comments?

This comment has been minimized.

@asm89

asm89 Dec 29, 2015

Member

While not currently checked, it is currently important to use the SQLFilter base class, because it takes care of the overall state of filters and how the ORM behaves. That's why there's code like the following in there:

Looking at the code you linked it seems you don't keep track of any of that?

This comment has been minimized.

@Wirone

Wirone Jul 28, 2016

@asm89 I was surprised that SQLFilter isn't verified anywhere, it's only in docblocks. It looks like anything can be passed to Configuration::addFilter()...

@TomasVotruba

This comment has been minimized.

Contributor

TomasVotruba commented Oct 7, 2015

What is missing here?

@TomasVotruba

This comment has been minimized.

Contributor

TomasVotruba commented Feb 25, 2016

Ping @Ocramius

What needs to be done here?

I'm writing same modular usage for Symfony now and this is still relevant issue.

namespace Doctrine\ORM\Query\Filter;
interface FilterFactoryInterface

This comment has been minimized.

@Majkl578

Majkl578 Jul 28, 2016

Member

deleted, wrong file

/**
* The base interface that user defined filters should implement.
*/
interface FilterInterface

This comment has been minimized.

@Majkl578

Majkl578 Jul 28, 2016

Member

Although it's strange, this probably should include also __construct (EntityManagerInterface) since this signature is forced by the factory. Yes, you may not need EM (and if so, you can write trait with empty constructor to handle this situation), but on the other hand, you are not allowed to use any other constructor signature as it would violate the contract.

@TomasVotruba

This comment has been minimized.

Contributor

TomasVotruba commented Oct 4, 2016

Close due to no response timeout.

@TomasVotruba TomasVotruba deleted the TomasVotruba:filter-factory branch Oct 4, 2016

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