Skip to content

[RFR] Fixing date filter to not consider empty string as a current date #382

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
Jan 11, 2016

Conversation

salahm
Copy link

@salahm salahm commented Jan 11, 2016

Q A
Bug fix? may be
New feature? no
BC breaks? may be
Deprecations? no
Tests pass? yes
Fixed tickets N/A
License MIT

Actually when we use DateFilter, if we send an empty parameter property[before]=, doctrine will convert it to current date (new \Datetime('')). This is annoying for a filtering behaviour, empty params should be ignored.

If we want use currentDate in DateFilter, we can use now string.

@salahm salahm force-pushed the fix-empty-strings-for-date-filter branch from 501c781 to 253652a Compare January 11, 2016 09:15
@salahm salahm changed the title [RFR] Fixing date filter to not considering empty string as a current date [RFR] Fixing date filter to not consider empty string as a current date Jan 11, 2016
@sstok
Copy link

sstok commented Jan 11, 2016

👍 but you should add some tests to prevent future regressions (and proof it's working).

@sroze
Copy link
Contributor

sroze commented Jan 11, 2016

I definitely agree with the idea. Can you add Behat tests? Then that would be good 👍

@dunglas
Copy link
Member

dunglas commented Jan 11, 2016

👍

@salahm salahm force-pushed the fix-empty-strings-for-date-filter branch from 253652a to fd0d73e Compare January 11, 2016 14:58
@salahm
Copy link
Author

salahm commented Jan 11, 2016

Tests added.

dunglas added a commit that referenced this pull request Jan 11, 2016
[RFR] Fixing date filter to not consider empty string as a current date
@dunglas dunglas merged commit a0ec5bb into api-platform:1.x Jan 11, 2016
@dunglas
Copy link
Member

dunglas commented Jan 11, 2016

Thanks @salahm

@mbrodala
Copy link
Contributor

mbrodala commented Aug 3, 2023

Is there a reason this change is not present in newer APIP versions? It seems this only went into the 1.x branch without forward-porting to newer branches.

@mbrodala
Copy link
Contributor

mbrodala commented Jul 9, 2025

@dunglas Can you check this? It is missing in API Platform 2.x, 3.x and 4.x.

@soyuka
Copy link
Member

soyuka commented Jul 15, 2025

Feel free to cherry-pick that and open a PR

@mbrodala
Copy link
Contributor

Started in #7291 now.

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.

6 participants