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

[WIP] Undoctrine doctrine specification #82

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

drymek
Copy link

@drymek drymek commented Dec 22, 2014

Doctrine Specification is a very cool library (probably even the only one for PHP). There is a great concept behind. I have one more reason to refactor it. To make it Doctrine independent and switch to InMemory implementation for tests or to any other, non-Doctrine for any reason (e.g. performance), without changing aplication.

This PR is proof of concept. In a very early stage. This will probably not be the master (ever). PR is just to show my direction and get some support from community.

There is a lot of to do:

  • Check if it really makes sense to change anything
  • Refactor Filter/In
  • Refactor Filter/Comparision
  • Refactor Filter/Equals
  • Refactor Filter/GreatherOrEqual
  • Refactor Filter/GreatherThan
  • Refactor Filter/LessOrEquals
  • Refactor Filter/LessThan
  • Refactor Filter/Like
  • Refactor Filter/NotEquals
  • Clean the code (comments, PSRs etc.)
  • Support all filters in Transformer
  • Implement something better than 100LOC if-else statement in Transformers for all Filters
  • Refactor Logic/AndX
  • Refactor Logic/LogicX
  • Refactor Logic/Not
  • Refactor Logic/OrX
  • Figure out how to change QueryModifiers
  • Refactor Query/Join
  • Refactor Query/OrderBy
  • Refactor Query/QueryModifier
  • Refactor Query/CollectionModifier
  • Figure out how to change ResultModifiers
  • Change ResultModifiers
  • Refactor Specification interface (?)
  • Clean the code (comments, PSRs etc.) second round
  • Refactor specification classes names
  • Move Doctrine Transformer outside DoctrineSpecification
  • Implement ArrayTransformer (InMemory)
  • Rewrite docs
  • Include "henrikbjorn/phpspec-code-coverage": "0.2.*", once again (Yes, I don't have xdebug installed)

@Nyholm
Copy link
Member

Nyholm commented Mar 14, 2015

Sorry for the late reply. Thank you for this massive PR. I really like the idea of decoupling from Doctrine. Can you describe the overall architecture for this refactoring?

@drymek
Copy link
Author

drymek commented Mar 18, 2015

The idea behind is as simple as DoctrineTransformer:: getQueryBuilder :-)
Everything from current classes, related to doctrine should be moved to DoctrineTransformer.
getQueryBuilder($specification) should return $builder ready to ->getQuery()->execute() (already bound parameters). It's getting harder to do it with joins (require aliases, @cakper point me that before, but I didn't listen).
Right now @norzechowicz is doing very similar functionality but on UnitOfWork level in his isolate project (https://github.com/norzechowicz/isolate). I didn't have time to look at it deeply, but it could solve my doctrine issue.

I've also tried to split DoctrineTransformer into smaller pices, but then it's getting more messy, so I decide to join it once again.

@tristanpemble
Copy link

Hi, what's the status on this/is there anything that can be done to help?

@drymek
Copy link
Author

drymek commented Aug 5, 2015

untill today I did not feel any pressure on it ;-)

As you can see, it's been a while since my last activity. Recently I focused on projects without database (at least the one where you can use specs). Specification was not a highest priority on my to do list.

Some time ago I read that there is a book (article?) about the specification and relational databases (Fowler - I will find a title). I would like to look into it before continuing.

As Far As I Remember the todo list above is up to date. So I stack on "Figure out how to change QueryModifiers" - the join parts. Nothing has change since my last comment on this. Sorry.

@drymek
Copy link
Author

drymek commented Aug 5, 2015

http://martinfowler.com/eaaCatalog/repository.html and http://martinfowler.com/apsupp/spec.pdf
BTW: I wonder if the query with for example joins should not be "manually" translated to DQL

class MyRepository extends SpecificationEntityRepository 
{
    public function match(Specification $specification)
    {
        if ($specification instanceof MyCustomSpec) {
            return $this->myCustomMethod();
        }

        return parent::match($specification);
    }
}

it should be much easier to maintain such library.

@tristanpemble
Copy link

a great example of a library that has done this successfully is rulerZ -- could probably look there for inspiration. I may look over this PR this weekend to see if I can help out

@drymek
Copy link
Author

drymek commented Aug 7, 2015

at first I wanted to ask "what is wrong with rulez", but (when I look on it) first I will ask , "how to use it?" ;-)

From what I can see, this library is much simpler, does not support joins?

@tristanpemble
Copy link

So the reason we can't use RulerZ is because they require you to use their DSL (made with hoa/ruler) for writing out the rules, which doesn't fit with our needs. I'd prefer to be building out rules the way it is done with this library

the developer is actually currently working Doctrine joining on this branch, so it may be more helpful to look at their code there

@webdevilopers
Copy link

👍 for decoupling! I just started using these Specifications. And they work like a charm.

My project follow tactical DDD approaches. Repositories can be InMemory, Doctrine or to be more concrete MysqlFooRepository or MongoFooRepository - not even mentioning Doctrine in the Filename but maybe the namespace above.

Some discussions going on:


private function getFilterDqlPart(FilterInterface $specification, ParametersBag $parameters)
{
switch (get_class($specification)) {
Copy link

Choose a reason for hiding this comment

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

That looks ugly and not extensible.

I would propose something like this (with better naming, I didn't put much thought in this):

class DoctrineTransformer
{
    public function __construct(DqlPartFromFilterTransformer $dqlPartFromFilterTransformer);

    public function getDQLPart(FilterInterface $specification, ParametersBag $parameters)
    {
        $dql = $this->dqlPartFromFilterTransformer->transform($specification, $parameters);

        if (null === $dql) {
            throw new InvalidArgumentException(sprintf(
                'Specification "%s" is not supported', 
                get_class($specification)
            ));
        }

        return $dql;
    }
}

interface DqlPartFromFilterTransformer
{
    /**
     * @return string|null Dql part string or null when not supported
     */
    public function transform(FilterInterface $specification, ParametersBag $parameters);
}

class GreaterOrEqualThanTransformer implements DqlPartFromFilterTransformer
{
    public function transform(FilterInterface $specification, ParametersBag $parameters)
    {
        if (!$specification instanceof \Happyr\DoctrineSpecification\Filter\GreaterOrEqualThan) {
            return;
        }

        $parameters->add($filter->getValue());

        return (string) $this->expression->gte($filter->getField(), $parameters->getLastName());
    }
}

class ArrayTransformer implements DqlPartFromFilterTransformer
{
    /**
     * @param DqlPartFromFilterTransformer[] $transformers
     */
    public function __construct(array $transformers);

    public function transform(FilterInterface $specification, ParametersBag $parameters)
    {
        foreach ($this->transformers as transformer) {
            $dql = $transformer->transform($specification, $parameters);

            if (null !== $dql) {
                return $dql;
            }
        }
    }
}

$doctrineTransformer = DoctrineTransformer(
    $queryBuilder, 
    new ArrayTransformer([
        new GreaterOrEqualThanTransformer(),
        new GreaterThanTransformer(),
        new InTransformer(),
        new IsNotNullTransformer(),
        // etc
    ])
);

Now, we're building Lego house ;-) DoctrineTransformer no longer needs big, ugly switch. Has less methods. You can add as many transformers as you like. Tests should get simpler. It's extensible for new implementations.

private $left;
private $right;

public function __construct(FilterInterface $left, FilterInterface $right)
Copy link
Member

Choose a reason for hiding this comment

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

AndX specification can contain more the two specifications

Copy link
Author

Choose a reason for hiding this comment

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

There is always left and right side for logic operator. A and B and C is more like (A and B) and C rather than and(A, B, C).

However I didn't finish that PR and I'm not interested in Doctrine anymore, so if you want to finish it, feel free to do it.

Copy link
Member

Choose a reason for hiding this comment

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

private $left;
private $right;

public function __construct(FilterInterface $left, FilterInterface $right)
Copy link
Member

Choose a reason for hiding this comment

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

OrX specification can contain more the two specifications

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.

6 participants