Adding the a test for combined tokens

[mamazu]

As mentioned in the comment of the other merge request there is still a problem with tokens that directly preceded a string.

This however might not be the intended behavior for bracketed strings.

[dbu]

interesting find! glad if we can fix some years-old bugs 👍

i think there indeed are side effects from this. how did the previous code handle quotes? did they show up or get dropped?

can you please also add a test with just square brackets but no quotes? aparently both forms should work.

[mamazu]

So I also have added tests for the brackets. I am not sure if that is the correct behavior for this. I could also add the function to the tokenizer to parse the bracketed expression as one.
The question is which is the intended way. But with the tests added, at least we have some kind of documentation on how it works.

[dbu]

the Sql2QomQueryConverter expects something different:

1) PHPCR\Tests\Query\QOM\ConvertQueriesBackAndForthTest::testBackAndForth with data set "6.7.20.SameNode.Selector_Space" ('6.7.20.SameNode.Selector_Space', array('SELECT * FROM [nt:file] AS fi...ode"])', 'SELECT * FROM [nt:file] AS fi...node])'))
PHPCR\Query\InvalidQueryException: Syntax error: Expected ')', found '"/home node"' in SELECT * FROM [nt:file] AS file WHERE ISSAMENODE(file, ["/home node"])

jackalope-doctrine-dbal/vendor/phpcr/phpcr-utils/src/PHPCR/Util/QOM/Sql2Scanner.php:94
jackalope-doctrine-dbal/vendor/phpcr/phpcr-utils/src/PHPCR/Util/QOM/Sql2ToQomQueryConverter.php:572
jackalope-doctrine-dbal/vendor/phpcr/phpcr-utils/src/PHPCR/Util/QOM/Sql2ToQomQueryConverter.php:425
jackalope-doctrine-dbal/vendor/phpcr/phpcr-utils/src/PHPCR/Util/QOM/Sql2ToQomQueryConverter.php:361
jackalope-doctrine-dbal/vendor/phpcr/phpcr-utils/src/PHPCR/Util/QOM/Sql2ToQomQueryConverter.php:118
jackalope-doctrine-dbal/vendor/phpcr/phpcr-api-tests/tests/Query/QOM/ConvertQueriesBackAndForthTest.php:77

looking at

phpcr-utils/src/PHPCR/Util/QOM/Sql2ToQomQueryConverter.php

Line 561 in d487942

protected function parseSameNode()

, i think we expect the quotes to not be returned but discarded?

unfortunately, we can't run the functional tests in phpcr-utils because the QOM model implementation is not part of phpcr-utils.

to see those tests in action, i did a checkout of jackalope-doctrine-dbal 1.x branch, installed all dependencies and replaced phpcr-utils with this PR.

[dbu]

i think the quotes need to completely be removed while scanning (not sure exactly what the old logic did). maybe we need to backport the test to a version with the old scanner and make the test assert the result of that scanner, and then make the new scanner do the same behaviour?

when i run the tests with this branch, i get:

There were 2 failures:

1) PHPCR\Tests\Query\QOM\ConvertQueriesBackAndForthTest::testBackAndForth with data set "6.7.20.SameNode.Selector_Space" ('6.7.20.SameNode.Selector_Space', array('SELECT * FROM [nt:file] AS fi...ode"])', 'SELECT * FROM [nt:file] AS fi...node])'))
QOM-->SQL2->QOM: Query variation 6.7.20.SameNode.Selector_Space resulted in SQL2 that is not found: array (
  0 => 'SELECT * FROM [nt:file] AS file WHERE ISSAMENODE(file, ["/home node"])',
  1 => 'SELECT * FROM [nt:file] AS file WHERE ISSAMENODE(file, [/home node])',
)

jackalope-doctrine-dbal/vendor/phpcr/phpcr-api-tests/tests/Query/QOM/ConvertQueriesBackAndForthTest.php:84

2) PHPCR\Tests\Query\QOM\Sql2ToQomConverterTest::testQueries with data set "6.7.20.SameNode.Selector_Space" ('6.7.20.SameNode.Selector_Space', array('SELECT * FROM [nt:file] AS fi...ode"])', 'SELECT * FROM [nt:file] AS fi...node])'))
Original query = SELECT * FROM [nt:file] AS file WHERE ISSAMENODE(file, ["/home node"])
Failed asserting that two objects are equal.
--- Expected
+++ Actual
@@ @@
     'source' => Jackalope\Query\QOM\Selector Object (...)
     'constraint' => Jackalope\Query\QOM\SameNode Object (
         'selectorName' => 'file'
-        'path' => '/home node'
+        'path' => '"/home node"'
     )
     'orderings' => Array ()
     'columns' => Array ()

jackalope-doctrine-dbal/vendor/phpcr/phpcr-api-tests/tests/Query/QOM/Sql2ToQomConverterTest.php:96

[mamazu]

Since we didn't have tests before. I couldn't tell that I changed the logic. Would have probably made more sense to create the tests first. I can check the previous implementation though.

But I that shouldn't be too hard to implement. The question still remains then though, what about invalid strings inside those brackets. Should they be validated?

[mamazu]

Should be fixed now as well.

[dbu]

yay, with this the phpcr acceptance tests run through again \o/

thanks a lot!

Since we didn't have tests before. I couldn't tell that I changed the logic. Would have probably made more sense to create the tests first. I can check the previous implementation though.

just to avoid misunderstandings: that was no complaint about your work. we should have added more tests in the beginning, that would have made it easier. thanks for adding the tests you did add.

[dbu]

tagged as https://github.com/phpcr/phpcr-utils/releases/tag/1.6.0