Change query to also query date, not just time

[ryanmitchell]

This PR includes a small fix to limit the query to the date as well as the time, otherwise we will match and invalidate entries with the correct time, but on the wrong date.

[martyf]

OK so this may be me being dumb… but queryTime is only doing time and not date?

[ryanmitchell]

yeah whereTime just does the time part of the date, it ignores the rest

[martyf]

Is this for Eloquent or File too?

Looking at the filterWhereTime function of Stache\Query\Builder.php, the $compareValue that gets built is a full date time, not just time. Unless I'm missing something?

I've updated tests, running file, to try to fail (i.e. two entries, different collections, different days, but same time each day) and tests still pass.

[ryanmitchell]

https://github.com/statamic/cms/blob/112f877d2b1185b23e02b558636e1962c07277eb/src/Stache/Query/Builder.php#L232 it should only affecting the time portion of the string - not the date (if its doing more its a bug in core!). and yeah eloquent works the same way - whereTime only considers the date part of what you compare it against. I always want there to be a whereDateTime but there isnt.

[martyf]

Humour me then... can you write a test that fails just using whereTime? I can't, and I'd like to have coverage of this.

This is where it'd be great to get a query dump (i.e. ray()->queries()) to see exactly what it looks like.

[ryanmitchell]

Yeah Im just noticing that the stache will always work as its using the entry date as the base, then setting the time from the comparison date... so it will always be the same day. If it was the other way round it would fail. What idiot wrote that (me).

Anyway, its needed for eloquent as it definitely doesnt work there!

[martyf]

I've added a collection that has the same time but different day, and the suite passes still.

Correct me if I'm wrong, but is there no way to be able to test against eloquent without requiring the eloquent-driver package? I'm wondering if I need to somehow write a test case for the ScheduledCacheInvalidatorTest for Eloquent - i.e. duplicate this test, but before running re-configure for Eloquent. Is this where require-dev would allow this? Sorry if that's a numpty question.

[ryanmitchell]

Yeah I think we've stumbled on a bug in the stache query builder - when you pass a full date string to whereTime, it's setting that whole date, rather than just the time parts. See this commit for what I'd change here to align the behaviour core currently expects.

If you take out the whereDate() on line 28 and just keep whereTime(), you'll see it now fails.

I'm going to fix up that bug on the core side, making sure that whereTime only considers the time part.

[ryanmitchell]

PR on core here with a bit of explanation: https://github.com/statamic/cms/pull/9360

[martyf]

Just to ensure I have the right understanding here:

• this PR is needed
• your eloquent package is doing things correctly
• current core is doing things incorrectly (but I see that Jason has merged your PR in already)

The PR to core fixes the issue, meaning with the next Statamic release, tests will start to fail.

Just making sure I understand all the moving parts.

[ryanmitchell]

Yeah that’s correct

[martyf]

With the latest core branch, without your guts of the PR, those tests fail correctly. This is awesome, thank you.

[ryanmitchell]

Got there in the end! Thanks for your patience.

[martyf]

And yours with timezones and weekends