[WIP] Query Builder V2 #318

Merged
merged 48 commits into from Sep 13, 2013

Projects

None yet

3 participants

Contributor
Q A
Bug fix? no
New feature? yes
BC breaks? yes
Deprecations? no
Tests pass? no
Fixed tickets N/A
License MIT
Doc PR --

Todo

Part 1

  • Ability to build on existing QB (implementations/equivilents of addSelect, addJoin, etc)
  • Make selector names mandatory, move selector argument before property_name argument in relevant classes.
  • Require source (which means selecting mixed documents is not supported by this QB - you can use a PHPCR query for that)
  • Drop lop and rop. They add about 28 characters to every comparison and are only useful for AutoComplete
  • Finish off functional tests and PHPCR-ODM integration.

Part 2

  • Migrate seperate property/selector arguments to compact single argument with dot notation.
  • Update API method names to reflect ODM (not PHPCR) and to be more intuitive.
  • Change namespace from Query\QueryBuilder to Query\Builder
Member

just to make sure .. you are basing this on PHPCR master where the selector name is no longer optional? phpcr/phpcr#68 .. there are related PRs on phpcr-utils, jackalope*

Contributor

I will do, I havn't started mapping anything to the PHPCR QOM yet. Although I do imagine that we will always have a selector one way or another.

The question is what should come first:

User must define - first argument:

$qb->select()->column('s', 'property_name');

or selector is optional second argument, but we automatically generate one:

$qb->select('property_name');
$qb->where()->equals()->propertyValue('propety_name', 'a');

So question is, in general: $propertyName before $selectorName or vice-versa?

Member

I think should have a default selector name (f.e. source), that by default is used for the FROM.

explicit selector

$qb->select()
    ->column('foobar', null, 'source')
    ->from()->document('Doctrine\Fqn\Foobar', 'source')

equivalent using the implicit selector:

$qb->select()
    ->column('foobar')
    ->from()->document('Doctrine\Fqn\Foobar')
Contributor

ok, makes sense.

Another question: In my current model I do not include the column name in "column". In an ODM this seems (to me) to be pointless, as indeed does the "column" terminology.

I imagine th euse case is to partially hydrate objects?

If so, then $qb->select()->property('foo')->property('bar') would make more sense.

Member
dbu commented Aug 14, 2013

yeah we need to talk about object fields, not phpcr properties (or
columns, which would be the orm equivalent of properties). it should be
the field name, and use the metadata to know what phpcr property that
translates to.

Contributor

Just to update, this is alot of work (lots of classes for this architecture). Making steady progress. Don't expect this to be done before wednesday however.

Contributor

I will try and set a day (or two) aside to finish this off this following week.

@dantleech dantleech commented on an outdated diff Aug 22, 2013
...ne/Tests/ODM/PHPCR/Query/QueryBuilder/BuilderTest.php
+
+use Doctrine\ODM\PHPCR\Query\QueryBuilder\Builder;
+
+class BuilderTest extends NodeTestCase
+{
+ public function provideInterface()
+ {
+ return array(
+ array('where', 'Where'),
+ array('from', 'From'),
+ array('orderBy', 'OrderBy'),
+ array('select', 'Select'),
+ );
+ }
+
+ public function testApi1()
dantleech
dantleech Aug 22, 2013 Contributor

This is what the API looks like now ..

Contributor

ok, have finished the complete API (or at least a first prototype). Next up is the QB => Phpcr Query converter.

@lsmith77 lsmith77 and 2 others commented on an outdated diff Aug 22, 2013
...ne/Tests/ODM/PHPCR/Query/QueryBuilder/BuilderTest.php
+ array('from', 'From'),
+ array('orderBy', 'OrderBy'),
+ array('select', 'Select'),
+ );
+ }
+
+ public function testApi1()
+ {
+ $this->node
+ ->select()
+ ->property('foobar', 'a')
+ ->property('barfoo', 'a')
+ ->end()
+ ->from()
+ ->joinInner()
+ ->left()->document('foobar', 'a')->end()
lsmith77
lsmith77 Aug 22, 2013 Member

like i said before .. this makes no semantical sense. a join isn't a left and a right join. you join onto an existing set of nodes and there you can use a left or right join to determine how missing nodes in the existing set or the set being joined should behave.

dantleech
dantleech Aug 23, 2013 Contributor

This is Phpcr terminology I believe, see the JoinInterdace. We can sort these little things out later, I'm still not 100% on the architecture.

Lukas Kahwe Smith notifications@github.com wrote:

  •        array('from', 'From'),
    
  •        array('orderBy', 'OrderBy'),
    
  •        array('select', 'Select'),
    
  •    );
    
  • }
  • public function testApi1()
  • {
  •    $this->node
    
  •        ->select()
    
  •            ->property('foobar', 'a')
    
  •            ->property('barfoo', 'a')
    
  •        ->end()
    
  •        ->from()
    
  •            ->joinInner()
    
  •                ->left()->document('foobar', 'a')->end()
    

like i said before .. this makes no semantical sense. a join isn't a
left and a right join. you join onto an existing set of nodes and there
you can use a left or right join to determine how missing nodes in the
existing set or the set being joined should behave.


Reply to this email directly or view it on GitHub:
https://github.com/doctrine/phpcr-odm/pull/318/files#r5918711

Sent from my Android device with K-9 Mail. Please excuse my brevity.

lsmith77
lsmith77 Aug 23, 2013 Member

Ah you are referring to https://github.com/phpcr/phpcr/blob/master/src/PHPCR/Query/QOM/JoinInterface.php#L32

As someone with a strong SQL background I find this quite confusing and also doesn't match SQL2:
http://www.h2database.com/jcr/grammar.html#join

But I guess there is some sense in following QOM for a QB.

However I still do not understand why this would be preferred over:

->from('foobar', 'a'))
->joinRightInner('barfoo', 'b')

..

->from('foobar', 'a'))
->joinRightInner('barfoo', 'b')
->joinRightInner('barfooca', 'c')

etc.

dantleech
dantleech Aug 24, 2013 Contributor

Well as you see in the API the join also needs a JoinCondition, it needs three objects. We cannot use the "where" to specify the join condition without doing some pretty confusing abstractions I imagine.

The concept does make sense to me as is, especially when you think in terms of Sources not tables. i. e. you select from a Join source.

Hopefully I will get back online soon and finish the prototype off.

Lukas Kahwe Smith notifications@github.com wrote:

  •        array('from', 'From'),
    
  •        array('orderBy', 'OrderBy'),
    
  •        array('select', 'Select'),
    
  •    );
    
  • }
  • public function testApi1()
  • {
  •    $this->node
    
  •        ->select()
    
  •            ->property('foobar', 'a')
    
  •            ->property('barfoo', 'a')
    
  •        ->end()
    
  •        ->from()
    
  •            ->joinInner()
    
  •                ->left()->document('foobar', 'a')->end()
    

Ah you are referring to
https://github.com/phpcr/phpcr/blob/master/src/PHPCR/Query/QOM/JoinInterface.php#L32

As someone with a strong SQL background I find this quite confusing and
also doesn't match SQL2:
http://www.h2database.com/jcr/grammar.html#join

But I guess there is some sense in following QOM for a QB.

However I still do not understand why this would be preferred over:

->from('foobar', 'a'))
->joinRightInner('barfoo', 'b')

..

->from('foobar', 'a'))
->joinRightInner('barfoo', 'b')
->joinRightInner('barfooca', 'c')

etc.


Reply to this email directly or view it on GitHub:
https://github.com/doctrine/phpcr-odm/pull/318/files#r5953344

Sent from my Android device with K-9 Mail. Please excuse my brevity.

dbu
dbu Aug 25, 2013 Member

i think from()->document('Document') is a bit awkward (and join is the special case, not the standard case). i would prefer from('Document')->end() with optional ->joinSomething after that. the first from needs no condition. only the join with the next document class, so we could provide the join condition there. and i think having both left and right of a join be another join would start to create funny precedence questions that do not exist in SQL2.

i won't veto against the empty from() but i would much prefer to follow lukas' suggestion.

dantleech
dantleech Aug 26, 2013 Contributor

I think it is important to remember that this QB is directly built up from the PHPCR QOM, this is how the QOM does it, and I think abstracting this behavior would give people wrong ideas about what actually happens.

I think there might be some scope for convenience methods, i.e. fromDocument, fromJoin and addJoin[Left|Foo], but essentially I am pretty sure that there is nothing wrong with the current API.

dbu
dbu Aug 26, 2013 Member

okay, i give in :-)

Contributor

ok, this is getting closer to being finished. I think one or two more hacking sessions.

Despite that I had another (slightly painful) thought last night that perhaps the current query builder could be improved if we were to implement a flavor of DQL. i.e.

$qb->where($qb->expr()->eq('UpperCase(DocumentName(a.foobar))', 'FOOBAR'));
$qb->createQuery('SELECT sel_1.foobar FROM Document\FooBar sel_1 WHERE DocumentName(sel_1.bar) = 'foo');

That would certainly be much more consistent with the Doctrine way of doing things. Will finish this in anycase.

@lsmith77 how do join conditions work in SQL2 ?

@dbu dbu and 1 other commented on an outdated diff Aug 25, 2013
...DM/PHPCR/Query/QueryBuilder/BuilderConverterPhpcr.php
+ $meta = $this->mdf->getMetadataFor($node->getDocumentFqn());
+ $this->selectorMetadata[$node->getSelectorName()] = $meta;
+
+ // get the PHPCR Selector
+ $selector = $this->qomf->selector(
+ $meta->getNodeType(),
+ $node->getSelectorName()
+ );
+
+ return $selector;
+ }
+
+ protected function walkSourceJoin(SourceJoin $node)
+ {
+ $left = $this->dispatch($node->getChildOfType('SourceJoinLeft'));
+ $riht = $this->dispatch($node->getChildOfType('SourceJoinRight'));
dbu
dbu Aug 25, 2013 Member

why riht and not right?

dantleech
dantleech Aug 25, 2013 Contributor

Err, symmetry :) Can change..
On Sun, Aug 25, 2013 at 05:42:47AM -0700, David Buchmann wrote:

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

  •    $meta = $this->mdf->getMetadataFor($node->getDocumentFqn());
    
  •    $this->selectorMetadata[$node->getSelectorName()] = $meta;
    
  •    // get the PHPCR Selector
    
  •    $selector = $this->qomf->selector(
    
  •        $meta->getNodeType(),
    
  •        $node->getSelectorName()
    
  •    );
    
  •    return $selector;
    
  • }
  • protected function walkSourceJoin(SourceJoin $node)
  • {
  •    $left = $this->dispatch($node->getChildOfType('SourceJoinLeft'));
    
  •    $riht = $this->dispatch($node->getChildOfType('SourceJoinRight'));
    

why riht and not right?


Reply to this email directly or [1]view it on GitHub.

References

Visible links

  1. https://github.com/doctrine/phpcr-odm/pull/318/files#r5968273
Member
dbu commented Aug 25, 2013

well eventually a DQL for phpcr-odm would be cool, but lets not try to do this here and now, it will be another big pile of work. (defining all the syntax and implementing another parser). i think the orm also has both, a query builder and the DQL.

Contributor

My concern is that IF we have DQL then the existing QueryBuilder model
would be more valid. This query builder has its pros and cons as does
the other, but the other is more familiar to Doctrine ORM users.

On Sun, Aug 25, 2013 at 05:45:37AM -0700, David Buchmann wrote:

well eventually a DQL for phpcr-odm would be cool, but lets not try to do
this here and now, it will be another big pile of work. (defining all the
syntax and implementing another parser). i think the orm also has both, a
query builder and the DQL.


Reply to this email directly or [1]view it on GitHub.

References

Visible links

  1. #318 (comment)
Contributor

ok, this /should/ actually work now, I am half way through documenting the classes, although do not currently have sufficient internet to pull in the dependencies required for functional testing.

@dbu or anybody else who uses PHPStorm/Eclipse/other smart IDE, I imagine one of the benefits of this approach is auto-completion, will specifying the return type in the DocBlock suffice? Could somebody check if auto-completion works with this API?

Member

when you say "work" you mean in the real world .. but the tests have just not been updated?

Contributor

I mean "in theory". I havn't had the chance to do functional tresting, so some integrations with ODM may not be there, but in theory the the qb is finished. If anyone wants to test it out that would be great, if not I will finish when I have stable internet connection.

Contributor

Just an update, this is getting closer all the time, although much more work then I ever imagined.

I have droped lop() and rop() as discussed, added andWhere, orWhere, addOrdering and addSelect, although still no addJoin[Inner|Left|Outer].

Contributor

ok, this is now ready for review. all the tests pass. Still missing is an "addJoin" type of functionality. I guess such functionality would look something like the following:

$qb->addJoinInner()
  ->right()->document('..')->end()
  ->condition()->equi(..)->end();

Depending on if we keep "left(), right() and condition()" in the "join factory"

  • Should we abbreviate things like ->propertyValue? $where->eq()->propVal('p', 'foo')->literal('foobar') ? We have kept the doctrine expression builder abbreviations, (eq(), gte, etc).
Member
lsmith77 commented Sep 3, 2013

needs a rebase

Contributor

Rebased.

Member
lsmith77 commented Sep 3, 2013

seems like there is an issue in the tests requiring PHP 5.3

but there are also 2 failures on PHP 5.4
https://travis-ci.org/doctrine/phpcr-odm/jobs/10951457

Member
lsmith77 commented Sep 3, 2013

what would be the best test case to look at to see the real world use? is there more of a functional test as well?

Contributor

https://github.com/doctrine/phpcr-odm/pull/318/files#diff-55 for afunctional test, or the BuilderTest for a full exposition of the API. See als the doc PR:doctrine/phpcr-odm-documentation#29

Member
lsmith77 commented Sep 4, 2013

thx .. will try to review this today on my way to ezsummercamp.

@lsmith77 lsmith77 referenced this pull request Sep 4, 2013
Closed

Qbv2 #323

Member
lsmith77 commented Sep 4, 2013

i pushed a few fixes in #323

also one note order for the selector name should always be last, default to 'a' .. and actually maybe the default should be something longer .. like default_selector_name

@dantleech dantleech closed this Sep 4, 2013
@dantleech dantleech reopened this Sep 4, 2013
Contributor

@lsmith77 for the selector name. I breifly discussed this with @dbu and we agreed to move it to the front (it was at the back). T

One of the reasons for this was that it is mandatory in the Doctrine ORM, the difference is just that they use dot notation to delimit "alias" and field name, where as we use arguments. I also think default selectors might make things rather opaque (although the exception message will tell them which selectors are available).

I am something like +0.66 on this. I think it is more transparent.

Member
lsmith77 commented Sep 5, 2013

Hmm .. I think its just a useless parameter in most use cases. Furthermore its the 2nd parameter in document() ..

Contributor

yeah, its the second parameter in document because its like an assignation, more generally the whole thing follows the SQL grammer FROM foo AS f WHERE f.foo = 'bar' => $from->document('foo', 'f')->end()->where()->eq()->propVal('f', 'foo')->literal('bar')

Contributor

ok, have added the addJoin syntax and also addDocument():

$qb
  ->fromDocument('FooBar', 'a')
  ->addJoinInner()
    ->right()->document('BarFoo', 'b')
    ->condition()->equi('a', 'prop_1', 'b', 'prop_2')
  ->end();
Contributor

Have just added a little utility command for splurging out a query builder reference; I am using it for helping with the documentation, but not sure if i should be "officially" supported yet (I don't want to spend more time developing it if you get what I mean)

./bin/phpcrodm doctrine:phpcr:qb:dump-reference                                       
 -> where() : where [0..1]
   -> andX() : constraint [1..1]
       -> ** recursion **
     -> orX() : constraint [2..2]
     -> propertyExists($selectorName, $propertyName) : constraint [2..2]
     -> fullTextSearch($selectorName, $propertyName, $fullTextSearchExpression) : constraint [2..2]
     -> sameDocument($selectorName, $path) : constraint [2..2]
Member
lsmith77 commented Sep 9, 2013

So what is the decision regarding making the selector name optional? i guess that is really the only open question?

Contributor

@lsmith77 so yeah, I think we have a rough consensus that we should adopt the selector_name.field_name notation. I will also update the method names of the API so that they are less verbose and fit the PHPCR-ODM paradigm rather than that of PHPCR.

Contributor

ok, renamed factory methods, switch to compact dot notation for fields and changed namespace from Query\QueryBuilder to Query\Builder

@dantleech dantleech and 2 others commented on an outdated diff Sep 10, 2013
...octrine/Tests/ODM/PHPCR/Query/Builder/BuilderTest.php
+
+ public function testNonExistantMethod()
+ {
+ $this->setExpectedException('BadMethodCallException',
+ 'Unknown method "foobar" called on class "Doctrine\ODM\PHPCR\Query\Builder\Builder", did you mean one of: "where, andWhere, orWhere'
+ );
+ $this->node->foobar();
+ }
+
+ // this test serves no other purpose than to demonstrate
+ // the API
+ public function testApi1()
+ {
+ $this->node
+ ->select()
+ ->field('a.prop_1')
dantleech
dantleech Sep 10, 2013 Contributor

Should this really be field (and not column)? What is the use case for this anyway?

lsmith77
lsmith77 Sep 10, 2013 Member

I guess for the ODM it might not make sense atm. But generally with PHPCR you do not have to fetch the underlying nodes, you can simply specify the node properties you want and depending on the use case this might be enough. That being said, Jackrabbit for example doesn't do a very good job of dealing with multi value properties there making it impossible to handle multi value properties with values that contain strings.

dbu
dbu Sep 10, 2013 Member

yeah, column would be very wrong. if anything it would be property, but it should be the field name of the class, not the phpcr property name. this is relevant when you hydrate to array and not to objects.

dantleech and others added some commits Aug 12, 2013
@dantleech dantleech First steps with new QB API 93d1b05
@dantleech dantleech Finished AbstractNode 53f56a4
@dantleech dantleech Building in more nodes ee7d82e
@dantleech dantleech Added all join stuff.
- next step: test all the join stuff
5555ff4
@dantleech dantleech Added tests for join
- next: JoinCondition tests
e5e7b1b
@dantleech dantleech Added JoinCondition tests 6809b75
@dantleech dantleech Added property + select 5695e9f
@dantleech dantleech Propvisional where f050501
@dantleech dantleech Added constraints
- todo: Test constraints
18c878e
@dantleech dantleech Fixed tests 489bb77
@dantleech dantleech ADded provisional operand factory
- @todo: All the operand stuff
861090b
@dantleech dantleech Added operand stuff d36ac00
@dantleech dantleech Added ordering 4df734d
@dantleech dantleech Fixed validation 8ccae25
@dantleech dantleech Added some example API tests 1668108
@dantleech dantleech All tests pass 40a25ea
@dantleech dantleech First essayage on PHPCR walker
- Notes on QB API:

 - Selectors mandatory? I think so, they should precede other args.
7e8a3b6
@dantleech dantleech Started converter ac3f6ac
@dantleech dantleech Started testing and building converter ed63d88
@dantleech dantleech Added select walking and tests d7a3b36
@dantleech dantleech Added composites a2facdb
@dantleech dantleech Added more horrendous tests 5f3dad2
@dantleech dantleech Refactored test cases, futher implementations e010cdb
@dantleech dantleech Finished tests (??) 915db53
@dantleech dantleech Nearly there aparently f191b22
@dantleech dantleech Stuff 982f461
@dantleech dantleech Refactored to use explicit node types (no longer using reflection mess) b080e8d
@dantleech dantleech Added class documentation 61ae17a
@dantleech dantleech Removed lop() and rop() ad88834
@dantleech dantleech Made selector name mandatory and changed arg precedence 8c0952e
@dantleech dantleech QB Functional tests OK fa5b7dc
@dantleech dantleech Added andWhere and orWhere f1d912c
@dantleech dantleech Add OrderBys 24e8779
@dantleech dantleech addSelect a8a8a6d
@dantleech dantleech All tests pass 9a653ec
@dantleech dantleech Annotated factory methods e3f9ab5
@dantleech dantleech Added ->fromJoin[Inner|LeftOuter|RightOuter] shortcuts
also ->fromDocument('foobar', 'a');
55b02a2
@dantleech dantleech Added phpcr:qb:dump-ref command f7e7c97
@dantleech dantleech Migrated seperate property/selector arguments
Migrate seperate property/selector arguments to compact single argument
with dot notation.
d521b83
@dantleech dantleech Update API method names.
Update API method names to reflect ODM (not PHPCR) and to be more
intuitive.
637a577
@dantleech dantleech Changed namespace
Change namespace from Query\QueryBuilder to Query\Builder
30412ab
@dantleech dantleech WIP 5a2bf48
@dantleech Daniel Leech Join Tests for QB a977e4f
@dantleech dantleech Numerous changes
- Disabled QB join functionality temporarily
- Swaped selectorName to last argument where applicable
- findBy ordering now accepts array($field => 'asc|desc') as with
  doctrine ORM
f905012
@dantleech dantleech Added test fixes (from lsmith PR) b0f7fd6
@lsmith77 @dantleech lsmith77 ensure there is a property 16c7646
Contributor

Right.. so I have added the changes from @lsmith77 's PR, I am throwing Exceptions when people try to do joins (joins do work but I am unsure about how they should work, so don't want to roll them out yet) and havesd swaped the selectorName argument around where applicable.

Member

there seems to be a PHP 5.3 issue in the test suite:
https://travis-ci.org/doctrine/phpcr-odm/jobs/11325232#L114

as for the doctrine dbal failures, i just pushed a fixed to master that will hopefully fix this so that you then just need to rebase on master.

btw once we have this PR merged, the next step is preparing for the PHPCR BC breaks:
https://github.com/phpcr/phpcr/pull/68/files

@lsmith77 lsmith77 commented on the diff Sep 13, 2013
bin/phpcrodm.php
@@ -48,6 +48,7 @@
new \PHPCR\Util\Console\Command\WorkspaceQueryCommand(),
new \PHPCR\Util\Console\Command\NodeTypeRegisterCommand(),
new \Doctrine\ODM\PHPCR\Tools\Console\Command\RegisterSystemNodeTypesCommand(),
+ new \Doctrine\ODM\PHPCR\Tools\Console\Command\DumpQueryBuilderReferenceCommand(),
lsmith77
lsmith77 Sep 13, 2013 Member

i guess you decided in the end to include it? fine for me

dantleech
dantleech Sep 13, 2013 Contributor

yeah.. I might modify it to make it dump the rst reference, or delete it yet. In any case let's not bootstrap it in the bundle.

Lukas Kahwe Smith notifications@github.com wrote:

@@ -48,6 +48,7 @@
new \PHPCR\Util\Console\Command\WorkspaceQueryCommand(),
new \PHPCR\Util\Console\Command\NodeTypeRegisterCommand(),
new
\Doctrine\ODM\PHPCR\Tools\Console\Command\RegisterSystemNodeTypesCommand(),

  • new
    \Doctrine\ODM\PHPCR\Tools\Console\Command\DumpQueryBuilderReferenceCommand(),

i guess you decided in the end to include it? fine for me


Reply to this email directly or view it on GitHub:
https://github.com/doctrine/phpcr-odm/pull/318/files#r6352958

Sent from my Android device with K-9 Mail. Please excuse my brevity.

@lsmith77 lsmith77 commented on the diff Sep 13, 2013
lib/Doctrine/ODM/PHPCR/DocumentRepository.php
@@ -131,9 +131,7 @@ public function findAll()
*/
public function findBy(array $criteria, array $orderBy = null, $limit = null, $offset = null)
{
- $qb = $this->dm->createQueryBuilder();
-
- $qb->from($this->className);
+ $qb = $this->createQueryBuilder('a');
lsmith77
lsmith77 Sep 13, 2013 Member

i guess there is no way around to having a default selector name here but it sort of sets a precedent for having a default and the name chosen. that being said i do not have a better suggestion than a so its fine.

dantleech
dantleech Sep 13, 2013 Contributor

well in this case it is opaque for the user - they can't do anything where they will encounter the qb created here.

Lukas Kahwe Smith notifications@github.com wrote:

@@ -131,9 +131,7 @@ public function findAll()
*/
public function findBy(array $criteria, array $orderBy = null,
$limit = null, $offset = null)
{

- $qb = $this->dm->createQueryBuilder();

  •    $qb->from($this->className);
    
  •    $qb = $this->createQueryBuilder('a');
    

i guess there is no way around to having a default selector name here
but it sort of sets a precedent for having a default and the name
chosen. that being said i do not have a better suggestion than a so
its fine.


Reply to this email directly or view it on GitHub:
https://github.com/doctrine/phpcr-odm/pull/318/files#r6353006

Sent from my Android device with K-9 Mail. Please excuse my brevity.

@lsmith77 lsmith77 commented on an outdated diff Sep 13, 2013
lib/Doctrine/ODM/PHPCR/Query/Builder/AbstractNode.php
+ * @throws \OutOfBoundsException if there are more than one or none
+ * @return array AbstractNode[]
+ */
+ public function getChild()
+ {
+ $children = $this->getChildren();
+
+ if (!$children) {
+ throw new \OutOfBoundsException(sprintf(
+ 'Expected exactly one child, got "%s"',
+ count($children)
+ ));
+ }
+
+ if (count($children) > 1) {
+ var_dump($children);
@lsmith77 lsmith77 and 1 other commented on an outdated diff Sep 13, 2013
...Doctrine/ODM/PHPCR/Query/Builder/AbstractLeafNode.php
+ *
+ * [selector_name].[field_name]
+ *
+ * e.g. my_selector.first_name
+ *
+ * @param string $field
+ *
+ * @return array
+ */
+ protected function explodeField($field)
+ {
+ $parts = explode('.', $field);
+
+ if (count($parts) == 2) {
+ return $parts;
+ } else {
lsmith77
lsmith77 Sep 13, 2013 Member

else isnt needed here

dantleech
dantleech Sep 13, 2013 Contributor

Why not? If the user specifies "foo.foo.foo" or just "foo" we will get 3
parts or 1 part and the field will be invalid.

On Fri, Sep 13, 2013 at 10:06:16AM -0700, Lukas Kahwe Smith wrote:

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

  • *
    
  • \*     [selector_name].[field_name]
    
  • *
    
  • \* e.g. my_selector.first_name
    
  • *
    
  • \* @param string $field
    
  • *
    
  • \* @return array
    
  • */
    
  • protected function explodeField($field)
  • {
  •    $parts = explode('.', $field);
    
  •    if (count($parts) == 2) {
    
  •        return $parts;
    
  •    } else {
    

else isnt needed here


Reply to this email directly or view it on GitHub.*

lsmith77
lsmith77 Sep 13, 2013 Member

i was unclear .. the code in the else makes sense .. but the if already returns .. so the else is superfluous.

@lsmith77 lsmith77 commented on an outdated diff Sep 13, 2013
...ine/ODM/PHPCR/Query/Builder/BuilderConverterPhpcr.php
+ * can append constraints to match the phpcr:class and phpcr:classparents
+ * later on.
+ *
+ * @var SourceDocument[]
+ */
+ protected $sourceDocumentNodes;
+
+ protected $from = null;
+ protected $columns = array();
+ protected $orderings = array();
+ protected $constraint = null;
+
+ public function __construct(
+ DocumentManager $dm,
+ QueryObjectModelFactoryInterface $qomf
+ )
lsmith77
lsmith77 Sep 13, 2013 Member

i think we usually do ) { in this case

@lsmith77 lsmith77 commented on an outdated diff Sep 13, 2013
...octrine/ODM/PHPCR/Query/Builder/ConstraintFactory.php
+ *
+ * $qb->where()
+ * ->andX()
+ * ->propertyExsts('prop_1', 'sel_1')
+ * ->propertyExsts('prop_2', 'sel_1')
+ * ->end()
+ *
+ * @factoryMethod
+ * @return ConstraintAndx
+ */
+ public function andX()
+ {
+ return $this->addChild(new ConstraintAndx($this));
+ }
+
+
lsmith77
lsmith77 Sep 13, 2013 Member

extra new line

@lsmith77 lsmith77 commented on the diff Sep 13, 2013
...octrine/ODM/PHPCR/Query/Builder/ConstraintFactory.php
+
+ public function getNodeType()
+ {
+ return self::NT_CONSTRAINT_FACTORY;
+ }
+
+ /**
+ * And composite constraint, usage
+ *
+ * $qb->where()
+ * ->andX()
+ * ->propertyExsts('prop_1', 'sel_1')
+ * ->propertyExsts('prop_2', 'sel_1')
+ * ->end()
+ *
+ * @factoryMethod
lsmith77
lsmith77 Sep 13, 2013 Member

hmm i wonder if this could come to bite us in the ass with Doctrines annotation parser

/cc @beberlei

lsmith77
lsmith77 Sep 13, 2013 Member

ah isn't there some syntax for marking an annotation as ignored in the class?

dantleech
dantleech Sep 13, 2013 Contributor

Maybe change it to #factoryMethod ?
On Fri, Sep 13, 2013 at 10:09:55AM -0700, Lukas Kahwe Smith wrote:

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

  • public function getNodeType()
  • {
  •    return self::NT_CONSTRAINT_FACTORY;
    
  • }
  • /**
  • \* And composite constraint, usage
    
  • *
    
  • \*   $qb->where()
    
  • \*     ->andX()
    
  • \*       ->propertyExsts('prop_1', 'sel_1')
    
  • \*       ->propertyExsts('prop_2', 'sel_1')
    
  • \*     ->end()
    
  • *
    
  • \* @factoryMethod
    

hmm i wonder if this could come to bite us in the ass with Doctrines annotation
parser

/cc @beberlei


Reply to this email directly or view it on GitHub.*

lsmith77
lsmith77 Sep 13, 2013 Member

there seems to be @IgnoreAnnotation for this purpose

@lsmith77 lsmith77 commented on the diff Sep 13, 2013
.../Console/Command/DumpQueryBuilderReferenceCommand.php
+
+ if (!in_array($ret, $iterated)) {
+ $iterated[] = $type;
+
+ $out = array_merge(
+ $out,
+ $this->formatTree($nodeAssocMap, $ret, $level + 1, $iterated)
+ );
+ }
+
+ }
+
+ return $out;
+ }
+
+ // protected function formatRst($nodeAssocMap)
lsmith77
lsmith77 Sep 13, 2013 Member

should this code not be removed rather than commented?

@lsmith77 lsmith77 commented on an outdated diff Sep 13, 2013
...HPCR/Query/Builder/SourceJoinConditionFactoryTest.php
+ array('descendant', 'SourceJoinConditionDescendant', array(
+ 'selector_1', 'selector_2',
+ )),
+ array('equi', 'SourceJoinConditionEqui', array(
+ 'selector1.property1', 'selector2.property2',
+ )),
+ array('child', 'SourceJoinConditionChildDocument', array(
+ 'child_selector', 'parent_selector',
+ )),
+ array('same', 'SourceJoinConditionSameDocument', array(
+ 'selector_1', 'selector_2', '/path/to/doc',
+ )),
+ );
+ }
+}
+
lsmith77
lsmith77 Sep 13, 2013 Member

extra new lines

@lsmith77 lsmith77 commented on an outdated diff Sep 13, 2013
...rine/Tests/ODM/PHPCR/Query/Builder/SourceJoinTest.php
+ }
+
+ public function provideInterface()
+ {
+ return array(
+ array('left', 'SourceJoinLeft', array(
+ '/Fqn/To/Class', 'a',
+ )),
+ array('right', 'SourceJoinRight', array(
+ )),
+ array('condition', 'SourceJoinConditionFactory', array(
+ )),
+ );
+ }
+}
+
lsmith77
lsmith77 Sep 13, 2013 Member

extra new lines

Member

went through the code and made a few comments. one more thing I just merged http://github.com/doctrine/phpcr-odm/pull/322 which aimed to get rid of all use of exceptions outside of the phpcr odm namespace

Contributor

cheers lukas.. I have added PHPCR exceptions for BadMethodCall and Runtime, added @IgnoreAnnotation and addressed the other points.

@lsmith77 lsmith77 commented on an outdated diff Sep 13, 2013
...DM/PHPCR/Query/Builder/SourceJoinConditionFactory.php
@@ -0,0 +1,109 @@
+<?phph
lsmith77
lsmith77 Sep 13, 2013 Member

small typo

Member

getting there .. a small typo and the php 5.3 issue left ..

@dantleech dantleech Code Review Fixes
- Added Runtime and BadMethodCall Exceptions
- Added @IgnoreAnnotation
- Removed extra lines
3b8334b
Member

there is now only a minor issue left with doctrine dbal

@lsmith77 lsmith77 merged commit f21d35e into doctrine:master Sep 13, 2013

1 check passed

default The Travis CI build passed
Details
@lsmith77 lsmith77 commented on the diff Sep 15, 2013
...octrine/ODM/PHPCR/Query/Builder/ConstraintFactory.php
+ *
+ * @param string $path
+ * @param string $selectorName
+ *
+ * @factoryMethod
+ * @return ConstraintSame
+ */
+ public function same($path, $selectorName)
+ {
+ return $this->addChild(new ConstraintSame($this, $selectorName, $path));
+ }
+
+ /**
+ * Descendant document constraint:
+ *
+ * $qb->where()->descendantDocument('/ancestor/path', 'sel_1')
lsmith77
lsmith77 Sep 15, 2013 Member

noticed that when you renamed the methods you missed the comments

dantleech
dantleech Sep 15, 2013 Contributor

yeah, I noticed that too :) Will be fixed in #326

Lukas Kahwe Smith notifications@github.com wrote:

  • *
    
  • \* @param string $path
    
  • \* @param string $selectorName
    
  • *
    
  • \* @factoryMethod
    
  • \* @return ConstraintSame
    
  • */
    
  • public function same($path, $selectorName)
  • {
  •    return $this->addChild(new ConstraintSame($this,
    
    $selectorName, $path));
  • }
  • /**
  • \* Descendant document constraint:
    
  • *
    
  • \*   $qb->where()->descendantDocument('/ancestor/path', 'sel_1')
    

noticed that when you renamed the methods you missed the comments


Reply to this email directly or view it on GitHub:
https://github.com/doctrine/phpcr-odm/pull/318/files#r6367502

Sent from my Android device with K-9 Mail. Please excuse my brevity.

@lsmith77 lsmith77 referenced this pull request in sonata-project/SonataDoctrinePhpcrAdminBundle Sep 15, 2013
Closed

fix filters #171

@dbu dbu commented on the diff Sep 16, 2013
lib/Doctrine/ODM/PHPCR/DocumentManager.php
$qm = $this->session->getWorkspace()->getQueryManager();
+ $qomf = $qm->getQOMFactory();
+ $converter = new BuilderConverterPhpcr($this, $qomf);
+ $builder->setConverter($converter);
dbu
dbu Sep 16, 2013 Member

is the converter an optional dependency of the querybuilder? otherwise i would prefer constructor injection to make sure the builder always has a converter with it...

dantleech
dantleech Sep 16, 2013 Contributor

Constructor injection would make more sense I guess, at the slight cost of having to mock a Converter in the tests.

dbu
dbu Sep 16, 2013 Member

i think that is a cost that is not too high :-)

dantleech
dantleech Sep 17, 2013 Contributor

But then, I guess this class will be used quite alot in tests (mocking this class would be a complete pain). I was going to suggest also that we might want to set different converters at runtime, although, short of using this QB as an abstract interface for the ORM, not sure what the use case would be.

dbu
dbu Sep 17, 2013 Member

to use different converters, you could also just instantiate new query
builders.

@dbu dbu commented on the diff Sep 16, 2013
lib/Doctrine/ODM/PHPCR/DocumentRepository.php
foreach ($criteria as $field => $value) {
- $qb->andWhere(
- $qb->expr()->eq($field, $value)
- );
+ if ($first) {
+ $first = false;
+ $where = $qb->where();
+ } else {
+ $where = $qb->andWhere();
dbu
dbu Sep 16, 2013 Member

could we not allow andWhere on a qb that has no where yet? so that where overwrites the criteria, but andWhere just adds another and-ed criteria to the list, even if the list is currently empty.

dantleech
dantleech Sep 16, 2013 Contributor

yeah, I thought about that but didn't want to be too "adventurous" :) We can do that.

dbu
dbu Sep 16, 2013 Member

imo it would make sense. i think the orm is doing the same - otherwise when you add to a query somewhere, you would always need to check if it already has that part or not. @lsmith77 do you agree?

lsmith77
lsmith77 Sep 16, 2013 Member

yeah .. would simplify end user code and i dont think there is a draw back.

dantleech
dantleech Sep 16, 2013 Contributor

Created issue:

https://github.com/doctrine/phpcr-odm/issues/328

in Mon, Sep 16, 2013 at 02:59:08AM -0700, Lukas Kahwe Smith wrote:

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

     foreach ($criteria as $field => $value) {
  •        $qb->andWhere(
    
  •            $qb->expr()->eq($field, $value)
    
  •        );
    
  •        if ($first) {
    
  •            $first = false;
    
  •            $where = $qb->where();
    
  •        } else {
    
  •            $where = $qb->andWhere();
    

yeah .. would simplify end user code and i dont think there is a draw
back.


Reply to this email directly or [1]view it on GitHub.

References

Visible links

  1. https://github.com/doctrine/phpcr-odm/pull/318/files#r6373018
@dbu dbu commented on the diff Sep 16, 2013
lib/Doctrine/ODM/PHPCR/Mapping/ClassMetadata.php
@@ -766,6 +766,10 @@ protected function validateAndCompleteFieldMapping(array $mapping, ClassMetadata
throw new MappingException("Field name must be of type string in '{$this->name}'.");
}
+ if (empty($mapping['property'])) {
+ $mapping['property'] = $mapping['fieldName'];
+ }
dbu
dbu Sep 16, 2013 Member

isn't this the responsitility of the yml/xml/annotation loader? i am pretty sure that this is already happening for most of the cases, so we would need to cleanup elsewhere if we move it all here.

dantleech
dantleech Sep 16, 2013 Contributor

Not sure, @lsmith77 added this.

lsmith77
lsmith77 Sep 16, 2013 Member

without this change this was failing https://github.com/doctrine/phpcr-odm/pull/318/files#L8R96
as for the responsibility .. we do a lot of normalization and validation in the ClassMetaData class .. so it seemed appropriate to do this there too

dbu
dbu Sep 16, 2013 Member

it probably makes sense to do this here, but then we should remove all other places that do the same. i will have a look and do a new pr.

dbu
dbu Sep 27, 2013 Member

in fact that default handling must have been lost at some point. while
hunting for this, i came across deprecated things that i remove in #344

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