Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
## 2.7.0

* Doctrine: Better exception to find which resource is linked to an exception (#3965)
* Doctrine: Allow mixed type value for date filter (notice if invalid) (#3870)
* MongoDB: `date_immutable` support (#3940)

## 2.6.3
Expand Down
14 changes: 14 additions & 0 deletions src/Bridge/Doctrine/Common/Filter/DateFilterTrait.php
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
namespace ApiPlatform\Core\Bridge\Doctrine\Common\Filter;

use ApiPlatform\Core\Bridge\Doctrine\Common\PropertyHelperTrait;
use ApiPlatform\Core\Exception\InvalidArgumentException;

/**
* Trait for filtering the collection by date intervals.
Expand Down Expand Up @@ -79,4 +80,17 @@ protected function getFilterDescription(string $property, string $period): array
],
];
}

private function normalizeValue($value, string $operator): ?string
Copy link
Contributor

@drupol drupol Dec 14, 2020

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

Copy link
Contributor

@drupol drupol Dec 14, 2020

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

{
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;
}
}
8 changes: 7 additions & 1 deletion src/Bridge/Doctrine/MongoDbOdm/Filter/DateFilter.php
Original file line number Diff line number Diff line change
Expand Up @@ -108,8 +108,14 @@ protected function filterProperty(string $property, $values, Builder $aggregatio
/**
* Adds the match stage according to the chosen null management.
*/
private function addMatch(Builder $aggregationBuilder, string $field, string $operator, string $value, string $nullManagement = null): void
private function addMatch(Builder $aggregationBuilder, string $field, string $operator, $value, string $nullManagement = null): void
{
$value = $this->normalizeValue($value, $operator);

if (null === $value) {
return;
}

try {
$value = new \DateTime($value);
} catch (\Exception $e) {
Expand Down
8 changes: 7 additions & 1 deletion src/Bridge/Doctrine/Orm/Filter/DateFilter.php
Original file line number Diff line number Diff line change
Expand Up @@ -128,9 +128,15 @@ protected function filterProperty(string $property, $values, QueryBuilder $query
*
* @param string|DBALType $type
*/
protected function addWhere(QueryBuilder $queryBuilder, QueryNameGeneratorInterface $queryNameGenerator, string $alias, string $field, string $operator, string $value, string $nullManagement = null, $type = null)
protected function addWhere(QueryBuilder $queryBuilder, QueryNameGeneratorInterface $queryNameGenerator, string $alias, string $field, string $operator, $value, string $nullManagement = null, $type = null)
{
$type = (string) $type;
$value = $this->normalizeValue($value, $operator);

if (null === $value) {
return;
}

try {
$value = false === strpos($type, '_immutable') ? new \DateTime($value) : new \DateTimeImmutable($value);
} catch (\Exception $e) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,7 @@ private function provideApplyTestArguments(): array
[
'dummyDate' => [
'after' => '1932iur123ufqe',
'before' => [],
],
],
],
Expand Down