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

feature: add a way to always include null in the date filter #1928

Merged

Conversation

Simperfit
Copy link
Contributor

@Simperfit Simperfit commented May 3, 2018

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #669
License MIT
Doc PR todo

Add a way to always include null values.

@@ -33,6 +33,7 @@ class DateFilter extends AbstractContextAwareFilter
const EXCLUDE_NULL = 'exclude_null';
const INCLUDE_NULL_BEFORE = 'include_null_before';
const INCLUDE_NULL_AFTER = 'include_null_after';
const ALWAYS_INCLUDE_NULL = 'always_include_null';
Copy link
Contributor

Choose a reason for hiding this comment

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

INCLUDE_NULL only maybe to be more inline with the existing options name ?

@@ -190,7 +191,8 @@ protected function addWhere(QueryBuilder $queryBuilder, QueryNameGeneratorInterf
$queryBuilder->andWhere($baseWhere);
} elseif (
(\in_array($operator, [self::PARAMETER_BEFORE, self::PARAMETER_STRICTLY_BEFORE], true) && self::INCLUDE_NULL_BEFORE === $nullManagement) ||
(\in_array($operator, [self::PARAMETER_AFTER, self::PARAMETER_STRICTLY_AFTER], true) && self::INCLUDE_NULL_AFTER === $nullManagement)
(\in_array($operator, [self::PARAMETER_AFTER, self::PARAMETER_STRICTLY_AFTER], true) && self::INCLUDE_NULL_AFTER === $nullManagement) ||
(\in_array($operator, [self::PARAMETER_AFTER, self::PARAMETER_STRICTLY_AFTER, self::PARAMETER_BEFORE, self::PARAMETER_STRICTLY_BEFORE], true) && self::ALWAYS_INCLUDE_NULL === $nullManagement)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe invert the conditions as the === might have a lower cost ?

@Simperfit Simperfit force-pushed the feature/add-always-include-null branch from 7cbce65 to 3695f5f Compare May 4, 2018 08:23
@teohhanhui
Copy link
Contributor

I'm not sure I understand the purpose of this. Aren't null values already included if you use one of INCLUDE_NULL_BEFORE or INCLUDE_NULL_AFTER?

@Simperfit
Copy link
Contributor Author

Look at the linked issue:

The OP wants to include null values always and it seems that doesn't work for now, so it's like a merge of both after and before so it will always includes null values.

@teohhanhui
Copy link
Contributor

Uhh, yeah I just looked at the issue... Maybe call it INCLUDE_NULL_BEFORE_AND_AFTER, to be explicit about what it actually does. I think it will cause confusion otherwise, when you somehow get null values at both ends.

@Simperfit
Copy link
Contributor Author

done

@Simperfit Simperfit force-pushed the feature/add-always-include-null branch from 3695f5f to 1375b2b Compare May 4, 2018 09:05
@Simperfit Simperfit force-pushed the feature/add-always-include-null branch from 1375b2b to 28bbbae Compare May 4, 2018 09:06
@soyuka soyuka merged commit dd2d49e into api-platform:master May 4, 2018
@soyuka
Copy link
Member

soyuka commented May 4, 2018

thanks @Simperfit

@Simperfit Simperfit deleted the feature/add-always-include-null branch May 4, 2018 09:57
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.

None yet

4 participants