Skip to content

Date querying #120

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

Merged
merged 2 commits into from
Jan 7, 2014
Merged

Date querying #120

merged 2 commits into from
Jan 7, 2014

Conversation

lsmith77
Copy link
Member

@lsmith77 lsmith77 commented Oct 9, 2013

supersedes #95

requires phpcr/phpcr-utils#99

@dbu
Copy link
Member

dbu commented Nov 15, 2013

does it matter when using the QOM if i use a date string or an actual date object?

@dbu
Copy link
Member

dbu commented Jan 4, 2014

can we wrap this up too?

@dbu
Copy link
Member

dbu commented Jan 7, 2014

we have methods in BaseCase to compare dates: https://github.com/phpcr/phpcr-api-tests/blob/master/inc/BaseCase.php#L246 and also assertSimilarDateTime added in #125 - could that help?

Not really sure how this works. ..
@lsmith77
Copy link
Member Author

lsmith77 commented Jan 7, 2014

That is not so trivial. We just have two QOM object graphs to compare. What we would need to do is then somewhere store an optional path for where we might have date strings that need to be compared differently.

Now the question is, doesn't it make sense to convert things that look like a date to a date instance? imho if you use a string representation (ie. SQL2) then you simply subject yourself to this kind of unclarity .. ie. did the person just use a string or was it meant to be a date? But I think it makes sense to assume they meant a date.

Alternatively we could also do the following change which would then mean in the QOM we use a string rather than a date. To me this highlights that it makes sense to do a string to date conversion in the parser instead:

diff --git a/tests/06_Query/QOM/QomTestQueries.php b/tests/06_Query/QOM/QomTestQueries.php
index fb5a1b2..007c36a 100644
--- a/tests/06_Query/QOM/QomTestQueries.php
+++ b/tests/06_Query/QOM/QomTestQueries.php
@@ -409,7 +409,7 @@ class QomTestQueries
                 $factory->comparison(
                     $factory->propertyValue('sel', 'prop'),
                     Constants::JCR_OPERATOR_GREATER_THAN,
-                    $factory->literal(new \DateTime('2013-04-15'))),
+                    $factory->literal('2013-04-15')),
                 array(),
                 array());

@dbu
Copy link
Member

dbu commented Jan 7, 2014

maybe the parser would need to understand the CAST syntax to create dates, and we would move closer to how what jackrabbit works. in jackrabbit, writing a date string will not match with a date property.

@lsmith77
Copy link
Member Author

lsmith77 commented Jan 7, 2014

hmm yeah I guess CAST could clarify this, though its technically not necessary to always write explicit CAST calls into SQL when working with Jackrabbit for example.

btw we also already automatically convert integers/floats as well as booleans. so to me converting dates is consistent.

@dbu
Copy link
Member

dbu commented Jan 7, 2014

the spec says this: http://www.day.com/specs/jcr/2.0/6_Query.html#6.7.34%20Literal

An UncastLiteral may be interpreted as a Value of property type STRING or some other type inferred from static analysis. A CastLiteral, on the other hand, is interpreted as the string form of a Value of the PropertyType indicated.

so the parser could try to guess if something is a date, but we should probably only match the YYYY-mm-dd format and the jcr timestamp and nothing else. and i added a TODO on phpcr-utils about the cast parsing. should actually be not that difficult to do phpcr/phpcr-utils#98

@lsmith77
Copy link
Member Author

lsmith77 commented Jan 7, 2014

we match on '/^\d{4}-\d{2}-\d{2}( \d{2}:\d{2}:\d+)?$/' see phpcr/phpcr-utils#99

lsmith77 added a commit that referenced this pull request Jan 7, 2014
@lsmith77 lsmith77 merged commit 0570897 into master Jan 7, 2014
@lsmith77 lsmith77 deleted the date_querying branch January 7, 2014 20:07
@dbu
Copy link
Member

dbu commented Jan 7, 2014

thanks!

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