Skip to content

Allow mixed type value when apply to DateFilter.#3870

Merged
alanpoulain merged 4 commits into
api-platform:mainfrom
vuongxuongminh:fix-3858
Feb 16, 2021
Merged

Allow mixed type value when apply to DateFilter.#3870
alanpoulain merged 4 commits into
api-platform:mainfrom
vuongxuongminh:fix-3858

Conversation

@vuongxuongminh

Copy link
Copy Markdown
Contributor
Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tickets fixes #3858
License MIT

Comment thread src/Bridge/Doctrine/MongoDbOdm/Filter/DateFilter.php Outdated

@dunglas dunglas left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM!

Comment thread src/Bridge/Doctrine/Common/Filter/DateFilterTrait.php Outdated
/**
* Normalize the value.
*/
private function normalizeValue($value, string $operator): ?string

@drupol drupol Dec 14, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I might be picky on this, but wouldn't it be better (semantically wise) to use early returns?

First exclude the bad scenario and then only return the happy scenario ?

Suggested change
private function normalizeValue($value, string $operator): ?string
private function normalizeValue($value, string $operator): ?string
{
if (false === \is_string($value)) {
$this
->getLogger()
->notice(
'Invalid filter ignored',
[
'exception' => new InvalidArgumentException(
sprintf(
'Invalid value for "[%s]", expected string',
$operator
)
),
]
);
return null;
}
return $value;
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Using a guard clause reduces the cyclomatic complexity and makes the code more readable (imho). I would keep this block as is.

@drupol drupol Dec 14, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

But this is what I'm trying to say here :-)

First the guard (the unhappy scenario), then at the end, the relevant returned $value (the happy scenario).

I would keep this block as is.

Which block are you "talking" about?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm 👍 for @drupol's suggestion.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let's not forget this thing :-)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oh it's done already ! Great :-)

@vuongxuongminh

Copy link
Copy Markdown
Contributor Author

Is there anything needed to get this merged?

Base automatically changed from master to main January 23, 2021 21:59
/**
* Normalize the value.
*/
private function normalizeValue($value, string $operator): ?string

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oh it's done already ! Great :-)

@alanpoulain alanpoulain merged commit 959f082 into api-platform:main Feb 16, 2021
@alanpoulain

Copy link
Copy Markdown
Member

Thank you @vuongxuongminh.

@drupol

drupol commented Feb 16, 2021

Copy link
Copy Markdown
Contributor

Yes, thanks !

@vuongxuongminh vuongxuongminh deleted the fix-3858 branch February 17, 2021 03:16
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.

ORM DateFilter throw fatal error when user submit unexpected query params.

4 participants