Violating Interface Segregation Principle

[cakper]

Hello,

PR #35 and question asked by @Nyholm forced me to think about design of our library and I have some thoughts on it:

• All classes in Comparison and Logic namespaces implement only Specification::match but not Specification::modifyQuery
• Some classes in Query namespaces implement Specification::match others implement Specification::modifyQuery but never both
• In all cases if we don't implement match/modifyQuery we only call parents
• It doesn't make sense to do LogicX logical operations on Specifications that only implement match - example: LogicOr(AsArray, Cache) will not take any sensible effect
• All Specification::modifyQuery are logical And's, have to be matched otherwise don't make sense
• We are using match and modifyQuery sequentially, it's always: $query = $qb->where($specification->match($qb, 'e'))->getQuery(); $specification->modifyQuery($query);

Above thoughts led me to conclusion that we are violating Interface Segregation Principle which states:

no client should be forced to depend on methods it does not use

So basically we have two different concepts here - one that is a Specification and allows us to build query, the other that I don't have name for that allows to manipulate query result (cache it, hydrate as array etc).

In my opinion we should split these two things into two separate interfaces and allow clients to implement single or both. Also because they look like separate things should be passed to EntitySpecificationRepository::match separately so we will need to change it's interface to something like match(Specification $specification, QueryOption $option).

That will enable us to write more sensible queries to repository. Example:

• we have Specification that contains rules for selecting power users, name could be PowerUserSpecification
• we have option that allows us to cache queries for a standard amount of seconds (we don't really care how log it is, as in PowerUserSpecification we don't care what are the exact rules for selecting those users), name of this option could be StandardCacheOption
• now for the standard display we can get results from our repository with EntitySpecificationRepository::match(PowerUserSpecification,StandardCacheOption)
• but for administration we need non cached version of it so we can simply call EntitySpecificationRepository::match(PowerUserSpecification) and get non cached results

@Nyholm please let me know what you think. I know it changes lib quite dramatically but because we are in pre release mode I think it's the best moment to discuss such thing.

One remaining issue is way of grouping so-called options together (just highlighting it).

[Nyholm]

That is an interesting thought. It is good to discuss such things now. 👍

ATM we got two specs using Specification::modifyQuery that is AsArray and Cache. So to make a proper decision, lets see what you can do in the body of modifyQuery. You may:

• Get the SQL
• Modify the parameters
• Set cache preferences
• Modify the fetch mode
• Modify hydration mode
• Get the result

I can't come up with a use case where a Specification _need_ that the PowerUser spec need to do any thing of the above to work properly. I think you are right, it makes sense to split the Specification interface into Specification and QueryOption.

But let's think of some implications of this... This means that it is always the class that calling $repository->match() that can decide about the QueryOptions. If I always want the PowerUser spec to have a CacheOption there is no way to achieve that, right? Or is that feature overkill?

[Nyholm]

From Lumbendil

Since query builder modification and Query modification wouldn't be closely related, wouldn't it be better to have specifications which modifies the Query and specifications which modifies the QueryBuilder?

@cakper Will you provide a PR with the changes you suggested?

[cakper]

@Nyholm thanks for reference, yep, that make sense and I'm keen to deliver this change, just need to find some spare time (travelling this weekend again so maybe in a plane or so :) ).

[Nyholm]

Awesome @cakper!