Skip to content

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

Merged
merged 1 commit into from
Aug 17, 2015
Merged

Use string values in DateFilter null-management constants #201

merged 1 commit into from
Aug 17, 2015

Conversation

teohhanhui
Copy link
Contributor

No description provided.

@teohhanhui
Copy link
Contributor Author

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%

@sroze
Copy link
Contributor

sroze commented Aug 9, 2015

IMO it's better to have string values in DateFilter constants.

@ogizanagi
Copy link
Contributor

👍 for string values in DateFilter constants

@teohhanhui teohhanhui changed the title Fix DateFilter documentation Use string values in DateFilter null-management constants Aug 11, 2015
@teohhanhui
Copy link
Contributor Author

Are we good?

@sroze
Copy link
Contributor

sroze commented Aug 14, 2015

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": ~ } ]
Copy link
Contributor

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 ~?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok with Theo.

Copy link
Contributor Author

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense

@dunglas
Copy link
Member

dunglas commented Aug 14, 2015

👍 it's the good time to break things (before 1.0).
Can you also update the changelog please?

@teohhanhui
Copy link
Contributor Author

Added a line in changelog

@sroze
Copy link
Contributor

sroze commented Aug 17, 2015

Thank you @teohhanhui ! 👍

sroze added a commit that referenced this pull request Aug 17, 2015
Use string values in DateFilter null-management constants
@sroze sroze merged commit 1c88abe into api-platform:master Aug 17, 2015
teohhanhui added a commit to teohhanhui/api-platform-core that referenced this pull request Oct 6, 2015
Since there never was a 1.0.0 beta 4
dunglas added a commit that referenced this pull request Oct 6, 2015
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.

5 participants