Skip to content
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

daterange - allow optionnal start date #3350

Merged
merged 3 commits into from May 7, 2021
Merged

daterange - allow optionnal start date #3350

merged 3 commits into from May 7, 2021

Conversation

ghost
Copy link

@ghost ghost commented May 7, 2021

While testing dateRange filtering for collection, I encountered error when the start value is not specified (see #3345 for the issue).

After looking deeper in the source code, I found that the method dateRange() come from an interface and has three implementations and the fix I submited in the issue was not complete. That's why I submit this pull request.

I've divided the fix into three commit so you may discard some of them :

  • First commit deal only with the fix of behaviour and touch only minimum lines of codes,
  • Second commit simplify a little bit the code,
  • Third commit is about the interface (method definition) : using null instead of false (as the behaviour is not specified if you pass true`), updating the documentation of the code (and synchronize the documentation accross all implemtnations) and fixing the only part of the code that call the method (with null instead of a mix of 0, false and null).

As 0, null and false have the same behaviour, the new version is retro-compatible with hypothetical legacy plugins that use the method.

The code have been manually tested on page collection with a fresh install of the fork and default theme :

  • Three pages, one in the pase (1800's), one in the present (2000's) and one in the future (2200's),
  • Test with start only, end only, both set,
  • Same test but with the field attribute specified.

@mahagr mahagr merged commit ec98ddc into getgrav:develop May 7, 2021
@mahagr
Copy link
Member

mahagr commented May 7, 2021

FYI: it's good practice to set return value as tight as possible from the docblock -- it allows chaining the method calls without static analyzer reporting errors on undefined methods.

I wish we were using PHP 7.4 or 8.0 minimum as it would allow us to define the return values.

@ghost
Copy link
Author

ghost commented May 7, 2021

Thank you for the information about the type in "@return". I didn't know why it was different from one method to the other, now, I know :-)

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.

1 participant