Skip to content

Conversation

dantleech
Copy link
Member

I have added a shortcut for ->getQOMFactory(), ->qomf() which is more analogous to the ORM's ->expr() shortcut.

    $qb->orWhere($qb->qomf()->comparison(
        $qb->qomf()->propertyValue('blog'),
        QOMConstants::JCR_OPERATOR_EQUAL_TO,
        $qb->qomf()->literal('My Blog')
    ));

rather than:

            $qomf = $qb->getQOMFactory();
            $qb->orWhere($qomf->comparison(
                $qomf->propertyValue('blog'),
                Constants::JCR_OPERATOR_EQUAL_TO,
                $qomf->literal($options['blog_uuid'])
            ));

What do you think?

Do you think it would be correct to add a shortcut for the Constants aswell?

  $qb->qomf()->operator('equal_to'); // or
  $qb->constrants('operator_equal_to'); // or
  $qb->operator('equal_to');

@dbu
Copy link
Member

dbu commented Dec 15, 2012

i guess this shortcut makes sense. regarding more shortcuts, what does the doctrine orm query builder do? i think we want to follow as closely to make it not more confusing then necessary for people who work with both.

the other changes in this PR came from #27, can you please rebase your branch now that that is merged, so we are sure what we merge.

- So this is analagous to the ORMs ->expr() helper
@dantleech
Copy link
Member Author

Rebased. The Doctrine query builder would do:

$qb->where($qb->expr()->eq('f.foo', 'bar'));

whereas the phpcr query builder does:

$qb->where($qomf->comparison($qomf->propertyValue('foo'), Constants::JCR_OPERATOR_EQUAL_TO, $qomf->literan('bar'));

Actually, looking at the ORMs Expr class it is just a wrapper for the various expression classes (e.g. Expr\Comparison). So maybe it makes sense to create an Expr class for PHPCR which closely resembles that of the ORM.

@dantleech
Copy link
Member Author

I have made a very poor start at an ExpressionBuilder class: https://github.com/dantleech/phpcr-odm/blob/expression_builder/lib/Doctrine/ODM/PHPCR/Query/ExpressionBuilder.php

Its a direct implementation copy of the ORMs Expr class. I thought it a good idea to put it in the Doctrine ODM as it could almost be a Doctrine version of the PHPCR QueryObjectModelFactory, so perhaps belongs in the Doctrine layer.

Pluses:

  • Queries much simpler: $qb->where($qb->expr()->eq('path', 'url'))
  • Syntax very similar to Doctrine.
  • If this is in the ODM then the documentation has a place to go...

Minuses

  • It shares the same job as the QueryObjectModelFactory, its just more user friendly.

Anyway, thats just an idea.

dbu added a commit that referenced this pull request Dec 16, 2012
@dbu dbu merged commit 4418ff7 into phpcr:master Dec 16, 2012
@dbu
Copy link
Member

dbu commented Dec 16, 2012

i wonder if it could even make sense to extract a common expression builder or an expression builder interface for doctrine commons so its explicit what is the same. @Ocramius does quite some effort in extracting common functionality for all doctrine implementations, maybe he can give some input to the question?

if we build it on a doctrine commons then we really should do it in phpcr-odm and not in phpcr-utils.

@dantleech
Copy link
Member Author

And I wonder also if we put the ExpressionBuilder in ODM, then maybe we
should put the QueryBuilder in ODM too (or layer on top) so that
it returns Documents and not PHPCR nodes.

On Sun, Dec 16, 2012 at 09:15:44AM -0800, David Buchmann wrote:

i wonder if it could even make sense to extract a common expression builder or
an expression builder interface for doctrine commons so its explicit what is
the same. @Ocramius does quite some effort in extracting common functionality
for all doctrine implementations, maybe he can give some input to the question?

if we build it on a doctrine commons then we really should do it in phpcr-odm
and not in phpcr-utils.


Reply to this email directly or view it on GitHub.

@dbu
Copy link
Member

dbu commented Dec 16, 2012

maybe we could do a wrapper QueryBuilder that just overwrites the method to get the actual query object which would extend the phpcr query and make it go through phpcr-odm. anyway i thought we already had something in place to get documents from a query - but maybe not when using the querybuilder? @lsmith77 do you know that?

on the other hand, the full orm compatible approach would be a query builder where you talk about the entity fields and not the phpcr field names as they theoretically can be mapped to different names - we just usually don't re-map and thus we don't see the issue.

@dantleech
Copy link
Member Author

There is the $dm->getDocuemntsFromQuery($query) but the problem I had
the other day was with the KNP paginator. It expects to be able to do
$query->execute() and retrieve the entities/documents. The PHPCR
listener for this component would need a dependency, the document
manager, to hydrate the documents, and as the paginator is a component
there is no way to package a PHPCR listener in the core (it doesn't know
anything about DI).

I do like the idea of QueryBuilder/ExpressionBuilder/Query at ODM level.

On Sun, Dec 16, 2012 at 11:09:24AM -0800, David Buchmann wrote:

maybe we could do a wrapper QueryBuilder that just overwrites the method to get
the actual query object which would extend the phpcr query and make it go
through phpcr-odm. anyway i thought we already had something in place to get
documents from a query - but maybe not when using the querybuilder? @lsmith77
do you know that?

on the other hand, the full orm compatible approach would be a query builder
where you talk about the entity fields and not the phpcr field names as they
theoretically can be mapped to different names - we just usually don't re-map
and thus we don't see the issue.


Reply to this email directly or view it on GitHub.

@dbu
Copy link
Member

dbu commented Dec 16, 2012

i like the idea too. question is how to coordinate with the other doctrines to share what can be shared, and whether the phpcr-odm implementation can extend the phpcr-utils querybuilder or needs to do a composition (the real question is: are some of the methods just the same regardles of whether we are in plain phpcr or phpcr-odm, in which case we would benefit from extending. or are the arguments always slightly different anyways so we can just as well compose)

would be awesome if you can have a stab at this @dantleech . think hard if we can actually share code with the orm and move stuff (interfaces? abstract base classes?) to doctrine commons.

@dantleech
Copy link
Member Author

Actually the query builder is already pretty consistent with the ORM, I
think it would probably suffice to extend it to return a wrapped (?)
instance of the PHPCR query object and implement the ExpressionBuilder
class. But probably more complicated then that, I will be happy to
have a look at I am finishing work on Wednesday, so will have lots of
time to work on CMS stuff :)

On Sun, Dec 16, 2012 at 11:53:56AM -0800, David Buchmann wrote:

i like the idea too. question is how to coordinate with the other doctrines to
share what can be shared, and whether the phpcr-odm implementation can extend
the phpcr-utils querybuilder or needs to do a composition (the real question
is: are some of the methods just the same regardles of whether we are in plain
phpcr or phpcr-odm, in which case we would benefit from extending. or are the
arguments always slightly different anyways so we can just as well compose)

would be awesome if you can have a stab at this @dantleech . think hard if we
can actually share code with the orm and move stuff (interfaces? abstract base
classes?) to doctrine commons.


Reply to this email directly or view it on GitHub.

@Ocramius
Copy link

@dbu is it just me or does this look really like the matching api?
https://github.com/doctrine/common/tree/master/lib/Doctrine/Common/Collections/Expr
https://github.com/doctrine/common/blob/master/lib/Doctrine/Common/Collections/Criteria.php

@dantleech can you check that one? It doesn't have joins though.

@dantleech
Copy link
Member Author

Yeah, those are pretty much perfect minimal APIs -- the problem would be that, as I
understand it, we would want to use the operator classes as given by the
QueryObjectModelFactory. Something like:

 public function andX(ConstraintInterface $constraint1, ConstraintInterface $constraint2)
 {
     return $this->qomf->andConstraint($constraint1, $constraint2);
 }

So, given that, those classes implemented as interfaces would be cool.

On Sun, Dec 16, 2012 at 12:18:38PM -0800, Marco Pivetta wrote:

@dbu is it just me or does this look really like the matching api?
https://github.com/doctrine/common/tree/master/lib/Doctrine/Common/Collections/
Expr
https://github.com/doctrine/common/blob/master/lib/Doctrine/Common/Collections/
Criteria.php

@dantleech can you check that one? It doesn't have joins though.


Reply to this email directly or view it on GitHub.

@Ocramius
Copy link

Hmm... not sure that's needed, since you just need custom walkers to get all the PHPCR specific stuff working...

@dantleech
Copy link
Member Author

ok .. Im not sure what custom walkers are, but if you say that will work
I will look into it. Cheers :)
On Sun, Dec 16, 2012 at 12:57:24PM -0800, Marco Pivetta wrote:

Hmm... not sure that's needed, since you just need custom walkers to get all
the PHPCR specific stuff working...


Reply to this email directly or view it on GitHub.

@Ocramius
Copy link

@dantleech you can look at how this stuff was implemented in ORM on the persistent collections =)

@dbu
Copy link
Member

dbu commented Dec 16, 2012

thanks for the pointers ocramius. so you suggest to use the doctrine common query object model to build a doctrine query and implement phpcr-odm walkers that build a phpcr query from that?

regarding joins, they are really secondary, joining in phpcr is not such a great idea often (and i think jackalope-doctrine-dbal has not even implemented it yet). so we can leave that out for now imho.

@Ocramius
Copy link

@dbu well, the criteria API of common is more like a filter for an existing collection. It cannot really replace the power of queries.

With a query, you could return anything, with the criteria API you're working on the assumption that you already have a collection of homogeneous existing objects (on DB or in an already loaded collection) and that you want to filter them.

But yes, this would be awesome to have across all object managers we currently have :)

As far as I know, the ORM implements the matching API on Doctrine\ORM\EntityRepository and on Doctrine\ORM\PersistentCollection. Doctrine\Common\Collections\ArrayCollection already implements it, so be aware that filtering should probably happen in the same way both on ArrayCollection and anything not yet loaded from DB.

@stof
Copy link

stof commented Dec 24, 2012

the ORM also supports merging a Criteria object into a QueryBuilder. This could be useful to add it here too.

@dantleech
Copy link
Member Author

ok. I am just about ready to start working on this -- I have looked at the SqlExpressionVisitor and I think making a PHPCR visitor should be easy enough - it would compile an object structure using the Query Object Model Factory., Some potential problems:

  • We will need to extend the ExpressionBuilder -- adding getQuery for example -- but this is hard coded to Commons in Criteria.
  • The comparisson class does not have "LIKE", pseudo example: select all nodes where path starts with "/sites/mysite/mydocs.
$qb->where('path', $qb->expr()->like('/sites/mysite/mydocs/%'));
  • Don't know if that is a valid example, but generally I can imagine being constrained to constants defined in the Comparison class could be problematic.

Does it make more sense to simply create a new QueryBuilder and friends and extract a common interface(s)?

@Ocramius
Copy link

Ocramius commented Jan 1, 2013

Query builders exist in DBAL, ORM and PHPCR afaik. Not sure if it should
live in common.

Marco Pivetta

http://twitter.com/Ocramius

http://ocramius.github.com/

On 1 January 2013 19:42, dantleech notifications@github.com wrote:

ok. I am just about ready to start working on this -- I have looked at the
SqlExpressionVisitor and I think making a PHPCR visitor should be easy
enough - it would compile an object structure using the Query Object Model
Factory., Some potential problems:

  • We will need to extend the ExpressionBuilder -- adding getQuery for
    example -- but this is hard coded to Commons in Criteria.
  • The comparisson class does not have "LIKE", pseudo example: select
    all nodes where path starts with "/sites/mysite/mydocs.

$qb->where('path', $qb->expr()->like('/sites/mysite/mydocs/%'));

  • Don't know if that is a valid example, but generally I can imagine
    being constrained to constnats container within the Comparison class
    could be problematic.

Does it make more sense to simply create a new QueryBuilder and friends
and extract a common interface(s)?


Reply to this email directly or view it on GitHubhttps://github.com//pull/28#issuecomment-11791621.

@dantleech
Copy link
Member Author

Well I meant create a new [Query|Expression]Builder class in ODM\PHPCR\Query which is not connected to the stuff in Commons\Collections except perhaps by a common minimal interface for the QueryBuilder -- but for the instant I could just copy the ORM builder as closely as possible.

@dbu
Copy link
Member

dbu commented Jan 2, 2013

if commons needs to be more flexible, e.g. optionally pass a classname to know what expressionbuilder to instantiate or a factory if we need more constructor arguments, i suggest doing a PR on that repository. just be careful to avoid BC breaks if ever possible. defaults should reproduce the current behaviour.
or maybe it can help to make factory methods on classes, like getExpressionBuilder that in the common implementation does a simple new but the Odm Criteria would overwrite that method...

as a work-in-progress you could maybe try to extend the ORM stuff so that you see what actually needs to be changed. if you end up with overwriting everything, there is little potential for code sharing. if however you can use many of the methods, we could indeed split the querybuilder into an AbstractQueryBuilder in commons.

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

Successfully merging this pull request may close these issues.

4 participants