Fix precedence parenthesis #140
Conversation
|
We shouldn't really have these tests here - they apply to I think we should have some separate package which can provide things like the QOM factory so that this stuff can be tested where it is. Maybe we could split the jackalope QOM code into a separate package? |
|
well phpcr utils is not mandatory for PHPCR implementations |
|
Yeah. So we should move these tests to phpcr-utils then I guess: use PHPCR\Util\QOM\Sql2Generator;
use PHPCR\Util\QOM\Sql2ToQomQueryConverter;
use PHPCR\Util\QOM\QomToSql2QueryConverter;
use PHPCR\Util\ValueConverter;We could use Jackalope FS to bootstrap the QOM maybe. |
| @@ -59,7 +59,7 @@ public static function getQueries() | |||
|
|
|||
| $queries['6.7.12.Constraint.Precedence.3'] = array( | |||
| 'SELECT * FROM [nt:file] AS file WHERE NOT file.prop1 = \'1\' OR file.prop2 = \'2\' AND NOT file.prop3 = \'3\'', | |||
| 'SELECT * FROM [nt:file] AS file WHERE (NOT file.prop1 = \'1\' OR (file.prop2 = \'2\' AND NOT file.prop3 = \'3\'))', | |||
| 'SELECT * FROM [nt:file] AS file WHERE ((NOT file.prop1 = \'1\') OR (file.prop2 = \'2\' AND (NOT file.prop3 = \'3\')))', | |||
There was a problem hiding this comment.
i think the line above is correct, so we should not change it here. or add an additional one if you want.
the problem is that the parser and everything depend on a qom factory which is in the implementation. i think the original idea is that having the test here confirms that a particular implementation works correctly with the parser and query builder. but indeed its a bit annoying. lets do like this: move tests into phpcr-utils and add jackalope-doctrine-dbal as require-dev there. but keep all jackalope specific things in one bootstrap bit that can be replaced, should another phpcr implementation want to use the tests to check their query functionality. |
|
i think https://travis-ci.org/jackalope/jackalope-jackrabbit/builds/37575242 is related to this. can you just add the lines as additional lines instead of replacing? assuming they are all valid and equivalent, just adding parenthesis to state the obvious :-) |
dantleech
force-pushed
the
fix-precedence-parenthesis
branch
from
655eee0
to
2fc26b9
Compare
|
Updated, I have added the variations instead of replacing them. With regards to moving the tests to PHPCR-Utils - I am a bit confused - if we move the Parser tests to phpcr-utils, what do we need to keep here? |
| $queries['6.7.15.Not'] = 'SELECT * FROM [nt:file] AS file WHERE NOT file.prop1 IS NOT NULL'; | ||
| $queries['6.7.15.Not'] = array( | ||
| 'SELECT * FROM [nt:file] AS file WHERE NOT file.prop1 IS NOT NULL', | ||
| 'SELECT * FROM [nt:file] AS file WHERE (NOT file.prop1 IS NOT NULL)', |
There was a problem hiding this comment.
would the most parenthesis not need to do WHERE (NOT (file.prop1 IS NOT NULL))? what if i negate a combined expression like NOT(a OR b)?
There was a problem hiding this comment.
@dantleech wdyt? is there more to fix or is that case already handled?
There was a problem hiding this comment.
I am not sure. Are you implying that the logic is broken and it should be doing what you suggest, or that we should add an additional variation to cover if it is ever handled differently?
There was a problem hiding this comment.
i was wondering whether your fix is complete or there are more things to fix for the NOT case. how does the query builder distinguish NOT(a OR b) from (NOT a) OR b? afaik NOT a OR b precedence would make it (NOT a) or b.
There was a problem hiding this comment.
I am not sure, but this is about the test not the code?. We should probably create an issue on phpcr-utils
Broken since: phpcr/phpcr-utils#128