Skip to content

[WIP] Added test for DateTime objectrs in QOM query #95

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

dantleech
Copy link
Member

Not really sure how this works. .. obviously the aim is to test to see that a DateTime object is accepted by the implementation.

Not really sure how this works. ..
@dbu
Copy link
Member

dbu commented Apr 15, 2013

the code looks right to me. did you run that test locally, does it actually test the bug you fixed on the QOM walker?

@dantleech
Copy link
Member Author

hmm,no didn't get round to testing it. Havn't gotten my head around how the test suite works. e.g. what does the array key correspond to? I guess the PHPCR API specification

$queries['6.7.27.1.PropertyValue']

Here I added .1 to .6.7.27 but not sure if that acceptable, if it isn't where should this test go?

@dbu
Copy link
Member

dbu commented Apr 16, 2013

yeah its the jcr spec. i am not too familiar with these tests here, but i think there should be a corresponding key in another file that has the corresponding sql query. i will try to check that out locally and see what happens.

@dbu
Copy link
Member

dbu commented Jun 9, 2013

we need to add something like this to tests/06_Query/QOM/Sql2TestQueries.php to know the sql2 representation (i think the date should look different)

/**
 * 6.7.27. PropertyValue
 */
$queries['6.7.27.1.PropertyValue'] = 'SELECT * FROM nt:unstructured WHERE sel.prop > \'2013-04-15\'';

but i think we have a problem in Sql2ToQomQueryConverter of phpcr-utils, where parseLiteral does not detect dates and parses them as strings. or maybe it is actually correct to have the dates as strings, totally not sure.

@lsmith77 lsmith77 mentioned this pull request Oct 9, 2013
@lsmith77
Copy link
Member

lsmith77 commented Oct 9, 2013

closing in favor of #120

@lsmith77 lsmith77 closed this Oct 9, 2013
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.

3 participants