-
-
Notifications
You must be signed in to change notification settings - Fork 922
Use string values in DateFilter null-management constants #201
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
Conversation
Do we also want to export these class constants as container parameters? So that instead of writing resource.book.date_filter:
parent: api.doctrine.orm.date_filter
arguments:
- datePublished: 0 # Dunglas\ApiBundle\Doctrine\Orm\Filter\DateFilter::EXCLUDE_NULL I can just write resource.book.date_filter:
parent: api.doctrine.orm.date_filter
arguments:
- datePublished: %api.doctrine.orm.date_filter.exclude_null% |
IMO it's better to have string values in |
👍 for string values in |
Are we good? |
I'm okay with that. But as there's a BC break, I'm waiting another maintainer's 👍. |
@@ -126,7 +126,7 @@ For instance, exclude entries with a property value of `null`, with the followin | |||
services: | |||
resource.date_filter: | |||
parent: "api.doctrine.orm.date_filter" | |||
arguments: [ { "dateProperty": ~ } ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not letting the default value settable with ~
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok with Theo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The null value is for using database default (i.e. no special null handling). That has not been changed.
What I'm doing here is correcting the example for excluding null values (view the full file to see the text above it).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense
👍 it's the good time to break things (before 1.0). |
Added a line in changelog |
Thank you @teohhanhui ! 👍 |
Use string values in DateFilter null-management constants
Since there never was a 1.0.0 beta 4
Correct changelog entry for #201
No description provided.