Query builder odm #204

Merged
merged 30 commits into from Jan 14, 2013

Conversation

Projects
None yet
4 participants
Contributor

dantleech commented Jan 2, 2013

This is a work-in-progress, it certainly does not work, so just ignore the blatant problems for now. Its just to show the direction Im going in.

The next steps will be to implement the ExpressionBuilder as in Doctrine\Commons.

I have indicated things I am not sure on with @notsure

Member

stof commented Jan 2, 2013

Please remove the swap files. You should probably ignore them locally: https://help.github.com/articles/ignoring-files

@stof stof and 1 other commented on an outdated diff Jan 2, 2013

lib/Doctrine/ODM/PHPCR/Mapping/ClassMetadataFactory.php
@@ -215,8 +215,6 @@ protected function validateRuntimeMetadata($class, $parent)
$class->validateLifecycleCallbacks($this->getReflectionService());
$class->validateTranslatables();
- // verify inheritance
- // TODO
@stof

stof Jan 2, 2013

Member

Why removing this TODO ?

@dantleech

dantleech Jan 2, 2013

Contributor

I shouldn't have, I started this work on an older existing branch that I rebased -- hence that problem and the swap files. Will revert in the next PR.

@stof stof and 1 other commented on an outdated diff Jan 2, 2013

lib/Doctrine/ODM/PHPCR/Query/QueryBuilder.php
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ *
+ * This software consists of voluntary contributions made by many individuals
+ * and is licensed under the MIT license. For more information, see
+ * <http://www.doctrine-project.org>.
+ */
+
+namespace Doctrine\ODM\PHPCR\Query;
+
+use PHPCR\Util\QOM\QueryBuilder as BaseQueryBuilder;
+use PHPCR\Query\QOM\QueryObjectModelInterface;
+use PHPCR\Query\QOM\ComparisonInterface;
+
+/**
+ * This class extends the wraps the PHPCR query builder
@stof

stof Jan 2, 2013

Member

This sentence is invalid

@dantleech

dantleech Jan 2, 2013

Contributor

Yeah I know, as I said in the PR .. just ignore these stupid errors, I will revise it all in the next PR.

@stof stof and 2 others commented on an outdated diff Jan 2, 2013

lib/Doctrine/ODM/PHPCR/Query/QueryBuilder.php
+ * @return QueryBuilder - this query builder instance
+ */
+ public function setParameter($key, $value)
+ {
+ $this->parameters[$key] = $value;
+
+ return $this;
+ }
+
+
+ /**
+ * Sets the parameters used in the query being constructed
+ * Note: Will overwrite any existing parameters.
+ *
+ * @todo: ORM uses Common\ArrayCollection and ORM\Query\Parameter
+ * Should probably do the same here (move ORM\Query\Parameter to Common ?)
@stof

stof Jan 2, 2013

Member

The reason to use a Parameter object (which was not the case in 2.2 btw) is to handle binding types properly. Does PHPCR need this ? If no, there is no reason to use a Parameter object

@dantleech

dantleech Jan 2, 2013

Contributor

Im not sure, @dbu @lsmith77

@dbu

dbu Jan 3, 2013

Member

well PHPCR properties always have a type, but afaik for querying the types do not matter. @dantleech may i ask you to write some phpcr query to check this? create a document with a Double property and query with a string or something like that? if that works then i guess we don't need the Parameter object

@stof stof and 1 other commented on an outdated diff Jan 2, 2013

lib/Doctrine/ODM/PHPCR/Query/QueryBuilder.php
+ */
+ public function getParameters()
+ {
+ return $this->parameters;
+ }
+
+ /**
+ * Gets a parameter for the query being constructed
+ *
+ * @param string $key - key of parameter to get
+ *
+ * @return QueryBuilder - this query builder instance
+ */
+ public function getParameter($key)
+ {
+ return $this->parameters[$key];
@stof

stof Jan 2, 2013

Member

you need to handle invalid keys properly

@dantleech

dantleech Jan 2, 2013

Contributor

ok.

@stof stof and 1 other commented on an outdated diff Jan 2, 2013

lib/Doctrine/ODM/PHPCR/Query/QueryBuilder.php
+ public function setFirstResult($firstResult)
+ {
+ $this->firstResult = $firstResult;
+
+ return $this;
+ }
+
+ /**
+ * Gets the position of the first result the query object was set to retrieve (the "offset").
+ * Returns NULL if {@link setFirstResult} was not applied to this QueryBuilder.
+ *
+ * @return integer The position of the first result.
+ */
+ public function getFirstResult()
+ {
+ return $this->getFirstResult();
@stof

stof Jan 2, 2013

Member

What ? This is an infinite loop

@dantleech

dantleech Jan 2, 2013

Contributor

hmm... yeah.

@dantleech dantleech commented on an outdated diff Jan 2, 2013

lib/Doctrine/ODM/PHPCR/Query/QueryBuilder.php
+ * <http://www.doctrine-project.org>.
+ */
+
+namespace Doctrine\ODM\PHPCR\Query;
+
+use PHPCR\Util\QOM\QueryBuilder as BaseQueryBuilder;
+use PHPCR\Query\QOM\QueryObjectModelInterface;
+use PHPCR\Query\QOM\ComparisonInterface;
+
+/**
+ * This class extends the wraps the PHPCR query builder
+ * and integrates it with the ODM
+ *
+ * @author Daniel Leech <daniel@dantleech.com>
+ */
+class QueryBuilder extends BaseQueryBuilder
@dantleech

dantleech Jan 2, 2013

Contributor

I should probably point out that this class DOES NOT extend BaseQueryBuilder, it will not extend any class.

Contributor

dantleech commented Jan 4, 2013

Right, have added the ExpressionVisitor and tests.

I am not sure ho to handle multipe ands

$expr->andx($expr->eq(..), $expr->gt(..), $expr->lt(..), $expr->eq(..));

The AndInterface has 2 operands. In this PR I have nested them, so
For the moment

WHERE (a=b AND b=c AND c=d)

becomes the equivilent of

WHERE (a=b and b=c) AND c=d

Is there a better way to do this, or should be limit andX to 2 constraints and force the user to nest manually?

Member

dbu commented Jan 4, 2013

we should follow the orm example here. but grouping of several ANDed terms does not change anything, neither for ORed as (a AND b) AND c is equivalent to a AND (b AND c). the only place it matters is when mixing, and for that the query output puts paranthesis around everything.

Contributor

dantleech commented Jan 5, 2013

updated. it should basically "work" but the following points need addressing:

  • Joining - I think we can improve the interface here, having shortcut methods for "joinDescendantNodes" etc.
  • QueryObjectModelFactory - The ExpressionVistor theoretically makes this redundant, the Visitor should handle all mapping between the layers.
  • OrderBy - Currently the QB accepts a string which is converted to a propertyValue class, but the QOMF will accept a DynamicOperandInterface, which includes things like the FullTextScoreInterface class for example. Do we need to support all these different types? If so any ideas on the best way to do it... can we use introspection?
  • setFromQuery - Is this required? It is provided by the DocumentManager as $dm->createQuery($statement, $language).

@dantleech dantleech commented on the diff Jan 5, 2013

lib/Doctrine/ODM/PHPCR/DocumentManager.php
@@ -488,7 +488,7 @@ public function createQuery($statement, $language)
public function createQueryBuilder()
{
$qm = $this->session->getWorkspace()->getQueryManager();
- return new QueryBuilder($qm->getQOMFactory());
+ return new QueryBuilder($this, $qm->getQOMFactory());
@dantleech

dantleech Jan 5, 2013

Contributor

I think we can remove this dependence and put all in the ExpressionVisitor

@lsmith77

lsmith77 Jan 8, 2013

Member

what dependency?

@lsmith77

lsmith77 Jan 8, 2013

Member

i guess you mean passing the QOM factory? yeah i think that makes sense .. ie. only pass $this

@dantleech

dantleech Jan 8, 2013

Contributor

I think I was confused actually - I thought that the QOMFactory could be encapsulated within the ExpressionVisitor, but this class is instantiated within the QueryBuilder anyway so we still need to pass it in. And the Factory is still used directly for orderBy.

Maybe Im being lazy, but I think I prefer to just carry on passing it as an argument, as its easier to mock and the QOMFactory is at the end of a pretty big method chain ($this->session->workspace->queryManage->qomFactory). But maybe I should change it.

@lsmith77

lsmith77 Jan 8, 2013

Member

k fine with me.

Member

dbu commented Jan 7, 2013

  • joining: i think this can also be added later, we could finalize this PR and merge, then add convenience methods
  • is QOM factory a odm thing or a phpcr thing?
  • orderBy: maybe a cheap solution would be to check is_string and if it is not string try to use the argument directly without creating a PropertyValue? basic usage would be simple, but advanced usage still possible.
  • setFromQuery indeed seems redundant, if $dm->createQuery has the same logic to handle SQL2 and QOM.
Contributor

dantleech commented Jan 7, 2013

  • Joining: I will throw "not supported" for now -- the logic is mostly there but not tested.
  • QOM = PHPCR-Utils - but contrary to my comment we do need to inject because it is a dependence of the ExpressionVistor. However, I think we can encapsulate its use in this class.
  • orderBy: That would be a cheap solution, but then again it is a bit dirty IMO, it suggests that we accept PHPCR QOM objects for all the methods. (maybe we should?). I think I would prefer something like $qb->addOrderByScore('asc') - and so on for each type of order by, maybe, as with joins, we can leave it for the moment.
  • setFromQuery: The ORM of course allows you to build queries with DQL, so I imagine that this is a somewhat similar way to provide the same functionality. But that makes me wonder if we should implement DQL. And so have one language to rule them all. But for now I will just drop this method.
Member

dbu commented Jan 7, 2013

orderBy - depends on how many cases there are. if its just a few clearly enumerable ones, then i think it makes sense to have convenience methods. otherwise having a mixed argument to generically allow advanced use seems ok to me.

ah yeah, DQL. that would be a DQL-PHPCR or something to express SQL2 in doctrine terms. i think its really not that important, as we don't have the table names that doctrine uses. the main benefit would be to use field names instead of property names if people remap them to different names. we don't have joins anyways in the sense of orm. so i think its just not worth the effort.

Contributor

dantleech commented Jan 8, 2013

ok, have added some functional tests, fixed some bugs and removed the Parameter stuff - its not supported by Jackalope and I don't know otherwise how to test it. I have also added exceptions to say that joining is not supported and removed the setFromQuery method.

Contributor

dantleech commented Jan 8, 2013

I am generally happy with this, but its a pretty big BC break. What do you think needs to be done to merge?

I wonder if we should provide a proxy method on the DocumentManager to give the PHPCR query builder.

Member

lsmith77 commented Jan 8, 2013

i dont think we need to offer a BC solution here. btw did you guys talk about adding the createQueryBuilder method and some basic interfaces for the query builder into the persistence API in doctrine common?

/cc @beberlei @Ocramius

Contributor

dantleech commented Jan 8, 2013

I have just noticed that removing setFromQuery has broken things, namely the DocumentRepository::createQuery($statement, $language) method. I guess I should probably add that back in.

Member

stof commented Jan 8, 2013

I don't think adding ObjectRepository::createQueryBuilder is a good idea. This cannot be done without a common interface for query builders and queries (as common builders are useless and cannot be specified cleanly if their getQuery() method simply returns mixed).

And a common builder interface does not make sense IMO. Each storage has its own querying capabilities. Trying to build a common interface would require keeping the lowest common denominator, which would make it useless. As you can see in this PR, half of the methods of the ORM builder simply don't make any sense in PHPCR. And for others, the expected arguments are not the same (PHPCR uses storage property names whereas the ORM uses the object properties for instance). So even with a common interface, it would still be impossible to build a storage-agnostic query, making the common interface useless.

Contributor

dantleech commented Jan 8, 2013

Another thing: The from() method - in the ORM this would be the object name, in the ODM it currently corresponds to the nodeType. Has anybody got any ideas on how this should behave?

Should this behave like the ORM? FROM could specify the FQN of the desired object, and would be mutually exclusive with a nodeType('foo:bar') method.

$qb->from('DCMS\Bundle\CoreBundle\Document\Foobar');
// or
$qb->nodeType('my:nodeType');

and when the from method is used, we can use introspection to determine the correct nodeType.

Member

lsmith77 commented Jan 8, 2013

I guess we could support both. a node type would just filter on node type, a class name would filter on the associated node type and class name

Contributor

dantleech commented Jan 8, 2013

ok .. I think it makes sense that from corresponds to the object class and not the node type (at least as I am used to the ORM) and that there should be a separate method to specify the nodeType and that the two are mutually exclusive (i.e. why would you want to specify nodeType and object class?).

if nobody has any objections I will do that tomorrow.

Member

lsmith77 commented Jan 8, 2013

no it would be either or. using separate method would mean that f.e from() calls would be overwritten by nodeType() calls. then again it should be rare for this scenario to happen

Contributor

dantleech commented Jan 8, 2013

yeah, we would throw an Exception if you tried to set the nodeType after calling from or vice-versa.

@dantleech dantleech Prefixed PHPCR Query methods
- Changed method names to reflect that they operate on Phpcr queries.
1621467
Contributor

dantleech commented Jan 9, 2013

ok, regarding the setFromQuery problem, I have added a proxy method to the DocumentManager, getPhpcrQueryBuilder, and also prefixed certain other methods

$dm->getDocumentsByPhpcrQuery(..)
$dm->createPhpcrQuery(..)

What do you think? I also thought of maybe adding a proxy class:

$dm->getPhpcr()->getSession();
$dm->getPhpcr()->createQueryBuilder();
$dm->getPhpcr()->createQuery(..);
$dm->getPhpcr()->getDocumentsByQuery(..);

Which would encapsulate the layer a bit more.

@dantleech dantleech Refactored "from" logic
- ->from() now corresponds to the Document Class
- ->nodeType() corresponds to nodeType
72e5cdd
Member

dbu commented Jan 9, 2013

regarding the from, i agree from should be document class and have a nodeType that are mutually exclusive. however contrary to the orm, it is not required to do either, you can just query everything if that makes sense. (this is more of a documentation issue i guess, but we should not complain if neither from nor nodeType are set) on the other hand at least atm, when using from with FQN we will not find extending documents, like we would with orm table inheritance.

maybe could do something to determine all possible classes by requiring inheritance annotations on documents or maybe with introspection. or by adding a multivalue field for base classes in each node that represents a document, so they can be found.

Member

dbu commented Jan 9, 2013

btw, i would not try to do the inheritance handling in this PR, this is something for later.

Contributor

dantleech commented Jan 9, 2013

ok, i have just updated the PR with the new from logic, I didn't realise you could query everything, will make this possible now.

Contributor

dantleech commented Jan 9, 2013

hmm, how do you not specify the nodeType? $qomf->createQuery($selector, .., .., .., ), selector must be an instance of SelectorInterface, and I cannot instantiate it without a nodeTypeName (and using "*" doesn't work)

@dantleech dantleech commented on an outdated diff Jan 9, 2013

lib/Doctrine/ODM/PHPCR/DocumentRepository.php
- $qb->from($qf->selector($this->class->nodeType));
- $qb->andWhere($qf->comparison($qf->propertyValue('phpcr:class'), Constants::JCR_OPERATOR_EQUAL_TO, $qf->literal($this->className)));
+ $qb->from($this->class->nodeType);
@dantleech

dantleech Jan 9, 2013

Contributor

this needs to be changed to $this->class->name

Member

dbu commented Jan 9, 2013

nt:base ? i think the node type filter does take inheritance into account

@dantleech dantleech commented on an outdated diff Jan 9, 2013

lib/Doctrine/ODM/PHPCR/DocumentRepository.php
- $qb->from($qf->selector($this->class->nodeType));
- $qb->andWhere($qf->comparison($qf->propertyValue('phpcr:class'), Constants::JCR_OPERATOR_EQUAL_TO, $qf->literal($this->className)));
+ $qb->from($this->class->nodeType);
+ $qb->andWhere(
@dantleech

dantleech Jan 9, 2013

Contributor

and this deleted.

Contributor

dantleech commented Jan 9, 2013

yep, nt:base works well.

Contributor

dantleech commented Jan 9, 2013

Travis is failing on the dependency downloading problem, all the tests pass here with jackrabbit.

@dbu dbu and 1 other commented on an outdated diff Jan 9, 2013

lib/Doctrine/ODM/PHPCR/DocumentManager.php
@@ -488,7 +489,18 @@ public function createQuery($statement, $language)
public function createQueryBuilder()
{
$qm = $this->session->getWorkspace()->getQueryManager();
- return new QueryBuilder($qm->getQOMFactory());
+ return new QueryBuilder($this, $qm->getQOMFactory());
+ }
+
+ /**
+ * Create lower level PHPCR query builder.
+ *
+ * @return QueryBuilder
+ */
+ public function createPhpcrQueryBuilder()
@dbu

dbu Jan 9, 2013

Member

do we need this method here? i would assume that people working with the odm should use the odm query builder now, or else they know how to work with the PHPCR api to build their phpcr-querybuilder.

the risk with having it is that people see this and think its what they should use. for the same reason i suggest dropping createPhpcrQuery. the only thing to keep would be getDocumentsByPhpcrQuery, but it needs to be well documented to understand that usually you just want the normal querybuilder of the odm.

@dantleech

dantleech Jan 9, 2013

Contributor

I added this method to provide a solution for $documentRepository->createQuery($statement, $language) https://github.com/doctrine/phpcr-odm/pull/204/files#L1L242

This method is valid enough in that if you provide an SQL statement we will (when the other PR is merged) return an ODM query. I'd prefer the ODM to be language agnostic - that would remove the ambiguities, but as you have said thats not important now.

I had a look at implementing the setFromQuery method on the ODM QueryBuilder but it isn't very practical (would have to convert the PHPCR constraints to their Doctrine Expression equivilents). And it didn't really feel right in that class.

As previously noted, I thought also to provide a proxy class which would encapsulate the PHPCR methods, e.g. $dm->phpcr()->getQueryBuilder. If thats any better.

@dbu

dbu Jan 9, 2013

Member

ok. well i think then its ok like this. the proxy class is not really helping that much imho. rather be more explicit in the doc comment how one should query normally and when to use this method.

@dbu dbu commented on an outdated diff Jan 9, 2013

lib/Doctrine/ODM/PHPCR/Query/QueryBuilder.php
+ * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ *
+ * This software consists of voluntary contributions made by many individuals
+ * and is licensed under the MIT license. For more information, see
+ * <http://www.doctrine-project.org>.
+ */
+
+namespace Doctrine\ODM\PHPCR\Query;
+
+use PHPCR\Query\QOM\QueryObjectModelFactoryInterface;
+use Doctrine\Common\Collections\Expr\Expression;
+use Doctrine\ODM\PHPCR\DocumentManager;
+use Doctrine\Common\Collections\ExpressionBuilder;
+
+/**
+ * @author Daniel Leech <daniel@dantleech.com>
@dbu

dbu Jan 9, 2013

Member

can you please add 1-2 lines of documentation to explain the purpose of the querybuilder?

@dbu dbu commented on an outdated diff Jan 9, 2013

lib/Doctrine/ODM/PHPCR/Query/PhpcrExpressionVisitor.php
@@ -0,0 +1,122 @@
+<?php
+
+namespace Doctrine\ODM\PHPCR\Query;
+use Doctrine\Common\Collections\Expr\ExpressionVisitor;
+use Doctrine\Common\Collections\Expr\Comparison;
+use Doctrine\Common\Collections\Expr\Value;
+use Doctrine\Common\Collections\Expr\CompositeExpression;
+
+use PHPCR\Query\QOM\DynamicOperandInterface;
+use PHPCR\Query\QOM\QueryObjectModelFactoryInterface;
+use PHPCR\Query\QOM\StaticOperandInterface;
+use PHPCR\Query\QOM\QueryObjectModelConstantsInterface as QOMConstant;
+
+class PhpcrExpressionVisitor extends ExpressionVisitor
@dbu

dbu Jan 9, 2013

Member

can you please add a short documentation what this is for (and mention its used by the implementation and not used by the user (if i understood that correctly))

@dbu dbu and 1 other commented on an outdated diff Jan 9, 2013

lib/Doctrine/ODM/PHPCR/Query/QueryBuilder.php
+ * @param Expression $expression
+ *
+ * @return QueryBuilder This QueryBuilder instance.
+ */
+ public function orWhere(Expression $expression)
+ {
+ if ($existingExpression = $this->getPart('where')) {
+ $this->add('where', $this->expr()->orX($existingExpression, $expression));
+ } else {
+ $this->add('where', $expression);
+ }
+
+ return $this;
+ }
+
+ // public function groupBy($groupBy)
@dbu

dbu Jan 9, 2013

Member

not sure if PHPCR supports anything comparable to groupBy, but i think not. no aggregation here. @lsmith77 true, right? if so i think we should drop those commented out methods, maybe add a comment in the class docblock that contrary to orm, there is no groupby.

what is having other than where? is there an equivalent in phpcr?

@lsmith77

lsmith77 Jan 9, 2013

Member

correct .. there is no such thing as the current spec defines that every piece of data maps 1:1 to a property. this BTW is also why adding support for count/facets into JCR is so tricky.

Contributor

dantleech commented Jan 10, 2013

Updated the doc block comments. Would UPDATE and DELETE queries be in the scope of this class eventually?

Member

lsmith77 commented Jan 10, 2013

Updated the doc block comments. Would UPDATE and DELETE queries be in the scope of this class eventually?

no .. i dont think we will ever have those

Member

lsmith77 commented Jan 11, 2013

alright .. @dbu merge time?

Member

lsmith77 commented Jan 11, 2013

btw how does this PR relate to #202 and #199?

@dbu dbu and 1 other commented on an outdated diff Jan 11, 2013

lib/Doctrine/ODM/PHPCR/DocumentManager.php
@@ -488,19 +489,34 @@ public function createQuery($statement, $language)
public function createQueryBuilder()
{
$qm = $this->session->getWorkspace()->getQueryManager();
- return new QueryBuilder($qm->getQOMFactory());
+ return new QueryBuilder($this, $qm->getQOMFactory());
+ }
+
+ /**
+ * Create lower level PHPCR query builder.
+ *
+ * NOTE: The ODM QueryBuilder (@link createQueryBuilder) in preference to
+ * the PHPCR QueryBuilder at this level. This method comes with a
+ * deprecation risk.
@dbu

dbu Jan 11, 2013

Member

i would say ... is preferred over the PHPCR QueryBuilder in the ODM. This method is here for special cases where the ODM QueryBuilder is not flexible enough.

if we think we will deprecate it again i wonder if we should not declare it @private by annotation right away. but i think its ok to let people use it for their special cases.

@dantleech

dantleech Jan 11, 2013

Contributor

Yeah, should have been "You should use ** the ODM QueryBuidler ..."

@dbu dbu and 1 other commented on an outdated diff Jan 11, 2013

lib/Doctrine/ODM/PHPCR/Query/QueryBuilder.php
+ * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ *
+ * This software consists of voluntary contributions made by many individuals
+ * and is licensed under the MIT license. For more information, see
+ * <http://www.doctrine-project.org>.
+ */
+
+namespace Doctrine\ODM\PHPCR\Query;
+
+use PHPCR\Query\QOM\QueryObjectModelFactoryInterface;
+use Doctrine\Common\Collections\Expr\Expression;
+use Doctrine\ODM\PHPCR\DocumentManager;
+use Doctrine\Common\Collections\ExpressionBuilder;
+
+/**
+ * Class to pragmatically construct query objects for the PHPCR ODM.
@dbu

dbu Jan 11, 2013

Member

pragmatically? or programmatically?

@dantleech

dantleech Jan 11, 2013

Contributor

Im not sure where that came from, I was sure I copied that wording somewhere, but can't find it now. I don't think pragmatically is the right word.

Member

dbu commented Jan 11, 2013

i think the other 2 PR are the same code at earlier stage and are obsolete. right @dantleech ?

i added two questions, the one about getPhpcrQueryBuilder we should probably decide before merging if this is some internal thing that we don't want people to use, or an entry point to low-level functionality that is ok for advanced users. i would vote for the later, accepting it as a valid thing to do for end users.

we will need to follow this up with a doc update quickly. and maybe even a short changelog documentation. i think it will not be very hard for people to update their code - but they need to understand what happened.

Contributor

dantleech commented Jan 11, 2013

The other PR (I have closed one) is related to using a native ODM Query object (with a $->execute method that returns documents). That is completmentary to this PR and would also be a BC break if merged separtely, so I wonder if I should merge that one into this one. That would also make sense for the documentation.

I have started some documentation, which is similar to the annotation documenation that I did before - i.e. its more a reference than a guide. I'll make a PR for that soon.

So I think...

  1. Merge the other PR into this PR
  2. Make/update a/the changelog
  3. Update the docs.
  4. Merge?
Member

lsmith77 commented Jan 11, 2013

+1

Daniel and others added some commits Dec 23, 2012

@dantleech Daniel Query Builder uses wrapped query
- DocumentManager returns wrapper PHPCR QueryBuilder
- Query builder uses a wrapper ODM query
- Can call ->execute() on returned query object and get Documents
e9e76c8
@dantleech dantleech ->execute() is proxy, ->getResults returns Docs
- ->execute() now does the same as it did before
- ->getResults() returns an ArrayCollecton of ODM Documents
- Cleaned up visibility
- Fixed doc annotations
- Added QueryTest
- Added primitive test for DocumentRepository::findBy
af7ed97
@dantleech dantleech Refactoring query
@at: Max results etc.
93d8c30
@dantleech dantleech Big query refactor
- PHPCR ODM Query is now heavily influenced by the ORM query

NOTE: Lots of functional tests are currently broken.
5b88a66
@dantleech dantleech Commit before removing DocumentClass stuff 0d9f1f3
@dantleech dantleech - Removed [get|set]DocumentClass stuff
- Fixed tests
- Added getPhpcrNode to Query
46e26b7
@dantleech dantleech Reintroduced document class methods
Renamed HYDRATE_PHPCR_NOTE to HYDRATE_PHPCR
c70d538
@dantleech dantleech Specified Document type in test a865310
@dantleech dantleech Merge branch 'odm_native_query_2' into query_builder_odm
Conflicts:
	lib/Doctrine/ODM/PHPCR/DocumentManager.php
	lib/Doctrine/ODM/PHPCR/DocumentRepository.php
	lib/Doctrine/ODM/PHPCR/Query/QueryBuilder.php
	tests/Doctrine/Tests/ODM/PHPCR/DocumentRepositoryTest.php
	tests/Doctrine/Tests/ODM/PHPCR/Functional/QuerySql2Test.php
	tests/Doctrine/Tests/ODM/PHPCR/Query/QueryBuilderTest.php
4fc94ec
Contributor

dantleech commented Jan 12, 2013

ok, have merged #202 into this PR

dantleech referenced this pull request Jan 12, 2013

Closed

Odm native query 2 #202

Contributor

dantleech commented Jan 12, 2013

There was an open quetstion on whether we should rename the references to Document with Object, e.g.

const HYDRATE_[DOCUMENT|OBJECT]

OBJECT is consistent with the ODM, but DOCUMENT makes more sense IMO, and if we rename here then I think the same logic would apply to all the .*Document.* methods.

@dantleech dantleech commented on an outdated diff Jan 12, 2013

lib/Doctrine/ODM/PHPCR/Mapping/ClassMetadata.php
@@ -40,10 +40,6 @@
*/
class ClassMetadata implements ClassMetadataInterface
@dantleech

dantleech Jan 12, 2013

Contributor

Not sure where this came from, I'm guessing this is wrong?

@dantleech dantleech commented on an outdated diff Jan 12, 2013

lib/Doctrine/ODM/PHPCR/Query/QueryBuilder.php
+ *
+ * This software consists of voluntary contributions made by many individuals
+ * and is licensed under the MIT license. For more information, see
+ * <http://www.doctrine-project.org>.
+ */
+
+namespace Doctrine\ODM\PHPCR\Query;
+
+use PHPCR\Query\QOM\QueryObjectModelFactoryInterface;
+use Doctrine\Common\Collections\Expr\Expression;
+use Doctrine\ODM\PHPCR\DocumentManager;
+use Doctrine\ODM\PHPCR\Query\Query;
+use Doctrine\Common\Collections\ExpressionBuilder;
+
+/**
+ * Class to pragmatically construct query objects for the PHPCR ODM.
@dantleech

dantleech Jan 12, 2013

Contributor

Still need to fix this

@dantleech dantleech and 2 others commented on an outdated diff Jan 12, 2013

lib/Doctrine/ODM/PHPCR/UnitOfWork.php
@@ -258,7 +258,7 @@ public function createDocument($className, NodeInterface $node, array &$hints =
foreach ($class->fieldMappings as $fieldName => $mapping) {
if (isset($properties[$mapping['name']])) {
- if ($class->isCollectionValuedAssociation($fieldName)) {
+ if (true === $mapping['multivalue']) {
@dantleech

dantleech Jan 12, 2013

Contributor

Again this isn't my change, not sure what has happened here.

@dbu

dbu Jan 13, 2013

Member

@lsmith77 was fixing stuff about metadata but i thought that is not yet in master? (same for the interface above)

@lsmith77

lsmith77 Jan 13, 2013

Member

a lot of it is .. there are still some open questions more. so i suspect this was a merge conflict with master

@dbu

dbu Jan 13, 2013

Member

4fc94ec says nothing about merge conflicts in UnitOfWork - or did the conflict happen earlier?

@dantleech can you merge master into this branch again and then compare those places that are still strange with the code in master?

Contributor

dantleech commented Jan 13, 2013

ok .. updated. have fixed the tests, incidentally the new paranthesis of the SQL2 queries has changed which changes the results of the tests. e.g.

SELECT s FROM nt:unstructured WHERE username = 'dtl' OR username = 'js' AND name = 'foobar'
// is interpreted by jackrabbit (paranthesis added) as
SELECT s FROM nt:unstructured WHERE username = 'dtl' OR (username = 'js' AND name = 'foobar)' // one result

where as with the new paranthesis it yields 0 results:

SELECT s FROM nt:unstructured WHERE ((username = 'dtl' OR username = 'js') AND name = 'foobar')```

I guess you know that already.

Contributor

dantleech commented Jan 13, 2013

So, as far as I'm concerned this is good to go (!!).. the documentation is almost done (doctrine/phpcr-odm-documentation#14) I'll try and update that with @dbu s suggestions tomorrow.

@lsmith77 lsmith77 commented on an outdated diff Jan 13, 2013

lib/Doctrine/ODM/PHPCR/Query/PhpcrExpressionVisitor.php
+ {
+ $constraintList = array();
+
+ foreach ($expr->getExpressionList() as $child) {
+ $constraintList[] = $this->dispatch($child);
+ }
+
+ if (count($constraintList) < 2) {
+ throw new \RuntimeException(sprintf(
+ 'Composite "%s" must have at least two constraints! (%d given)',
+ $expr->getType(),
+ count($constraintList)
+ ));
+ }
+
+ switch($expr->getType()) {
@lsmith77

lsmith77 Jan 13, 2013

Member

missing space after switch, ie. switch (

@lsmith77 lsmith77 and 1 other commented on an outdated diff Jan 13, 2013

lib/Doctrine/ODM/PHPCR/Query/Query.php
+ $this->setHydrationMode($hydrationMode);
+ }
+
+ if (null !== $this->maxResults) {
+ $this->query->setLimit($this->maxResults);
+ }
+
+ if (null !== $this->firstResult) {
+ $this->query->setOffset($this->firstResult);
+ }
+
+ foreach ($this->parameters as $key => $value) {
+ $this->query->bindValue($key, $value);
+ }
+
+ if ($this->hydrationMode === self::HYDRATE_PHPCR) {
@lsmith77

lsmith77 Jan 13, 2013

Member

why not use a switch statement here?

@dantleech

dantleech Jan 13, 2013

Contributor

My VIM has rubbish indentation for switch statements, so I tend never to
use them, but yeah I can change it here.
On Sun, Jan 13, 2013 at 02:54:53PM -0800, Lukas Kahwe Smith wrote:

In lib/Doctrine/ODM/PHPCR/Query/Query.php:

  •        $this->setHydrationMode($hydrationMode);
    
  •    }
    
  •    if (null !== $this->maxResults) {
    
  •        $this->query->setLimit($this->maxResults);
    
  •    }
    
  •    if (null !== $this->firstResult) {
    
  •        $this->query->setOffset($this->firstResult);
    
  •    }
    
  •    foreach ($this->parameters as $key => $value) {
    
  •        $this->query->bindValue($key, $value);
    
  •    }
    
  •    if ($this->hydrationMode === self::HYDRATE_PHPCR) {
    

why not use a switch statement here?


Reply to this email directly or view it on GitHub.

Contributor

dantleech commented Jan 14, 2013

updated.

dbu merged commit 9a48d49 into doctrine:master Jan 14, 2013

1 check passed

default The Travis build passed
Details

dbu referenced this pull request in sonata-project/SonataDoctrinePhpcrAdminBundle Jan 14, 2013

Closed

use phpcr-odm querybuilder instead of phpcr qb #68

Member

dbu commented Jan 14, 2013

thanks a lot for your persistence in this one! great to have a real query builder.

i created an issue on sonata phpcr-odm admin as this will need update for sure. not sure if the sandbox or other pieces of the cmf also use query builder currently and need to be adjusted. updating the sandbox and running the tests should reveil at least some of the problems.

for hydrate array i created http://www.doctrine-project.org/jira/browse/PHPCR-85 so we track this somewhere.

there is also http://www.doctrine-project.org/jira/browse/PHPCR-84 which i guess would mean to implement DQL and gives a good argument why DQL would indeed be useful. talking about DQL: how can we prevent another BC break with existing code if/when we implement DQL? i think the ORM query builder only accepts DQL (that is, class field names and not table columns/phpcr property name)

Contributor

dantleech commented Jan 14, 2013

No problem, I enjoyed it :)

For me the next evolutions here would be Join support and also an expanded expression builder with, for example, the method descendantNode($path). Joins might also be linked to a DQL-type ticket, e.g. I;m not sure if you can do stuff like $qb->where($qb->expr('table1.foobar', 'barfoo')), although I have a feeling that that would work with the native selector stuff.

Would DQL break BC? Don't we map the fieldNames directly to propertyNames anyway?

Member

dbu commented Jan 14, 2013

great! i think completeness on expressions like descendantNode would be really cool. the join thing is rather exotic in PHPCR so i think its not that important. and i think jackalope-doctrine-dbal does not implement joins yet anyways.

regarding BC: like in ORM if you say nothing, the names are the same, but you can change that default setting. and then it would start to matter. the other thing is with querying properties of translated documents where the logic would be really different depending on whether you have to do it by hand or a DQL layer handles this for you. so maybe we should quickly change the selector and where things to somehow make explicit we talk about phpcr property as once we have DQL the default would be the document field name. maybe add an optional parameter $phpcrProperty = false where it could be both, and change our examples to pass true and mention the other case is not yet implemented? wdyt @lsmith77

Member

lsmith77 commented Jan 14, 2013

indeed currently in the results if people dont simply fetch documents, there could be a mismatch between the PHPCR name and the name of the property in the document, which could lead to confusion. i dont have a perfect overview of what we currently have, but yes a result set created from an ODM query builder should use the mapped names and not the phpcr internal property names.

Member

lsmith77 commented Jan 14, 2013

oh and i dont think we should support an option to get the phpcr internal names. if someone wants them, they can simply use a native query. if at all we could allow them to get the native query object build inside the odm query builder.

Contributor

dantleech commented Jan 14, 2013

Yes, if they want to explicitly use the PHPCR property names they could use the PHPCR query builder, e.g. $dm->getPhpcrQueryBuilder and use $dm->getDocumentsForPhpcrQuery, and there is also a method on the new Query object to get the PHPCR query, $query->getPhpcrQuery().

Member

dbu commented Jan 14, 2013

i agree with you lukas, but right now we have a "ODM" query builder that expects the PHPCR property names. so either we would have to implement DQL and revert this merge until then or we need a temporary solution. or we do another BC break once we have DQL for people who differ between PHPCR property and document field names. my suggestion was to avoid that BC break by adding this option until we have DQL and deprecate it right away.

hm. was going to write we can just lookup the property names in the field names and thus do not need full DQL to do that. even more as we do not have joins yet. BUT as we do not always select from a specific document, we can not really translate fields to properties whatever we try! we don't know what document classes might be selected and thus we do not know where to look. now this is a bit of a bummer for DQL :-(

we could say that we later support d.field if the user did specify the document in from, but if you set something to field it will be interpreted as a PHPCR property name. this would keep BC and bring both flexibility and the option to have more strict DQL.

Contributor

dantleech commented Jan 14, 2013

Some ideas:

  • We could have two modes for the query builder, PHPCR and ODM,

    $qb->from('DocumentClass') // ODM mode
    ->select('name'); // selects on the document field name

    $qb->nodeType('nt:unstructured') // PHPCR mode
    ->select('foobar'); // select on PHPCR property

    Currently you can either select from the DocumentClass OR the
    nodeType, so that much is there already. Also ommitting "from"
    would default to PHPCR, which would make sense.

  • As @dbu says, we can force the user to specify an alias
    then that alias can be used to identify the select mode.

    $qb->from('DocumentClass', 'd')
    $qb->where($qb->expr()->eq('d.fieldName', 'value'));
    $qb->andWhere($qb->expr()->eq('d>ns:docProperty', 'value'));
    $qb->andWhere($qb->expr()->eq('ns:globalProperty', 'value'));

    (last line makes no sense, we already have selected the phpcr type of
    the DocumentClass).

  • We forget the PHPCR specifics in the ODM query builder, there is a
    PHPCR-Utils QB.

Member

dbu commented Jan 14, 2013

  • We could have two modes for the query builder, PHPCR and ODM,

Currently you can either select from the DocumentClass OR the
nodeType, so that much is there already. Also ommitting "from"
would default to PHPCR, which would make sense.

+1
really convincing for me. it is a bit implicit and will confuse people
who did not fully read and understand things but i think this can not
really be avoided anyways. and at least those who do not grasp phpcr
will use the from and thus is always in odm mode, saving them confusion
about the low level details...

Member

lsmith77 commented Jan 14, 2013

+1

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