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.

Sign up for GitHub

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

Introduce "force_case_insensitivity" option and deprecate "case_sensitive" in StringFilter #1416

Merged
merged 2 commits into from
Apr 16, 2021
+104 −308
Merged

Introduce "force_case_insensitivity" option and deprecate "case_sensitive" in StringFilter #1416

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1d6cbc4
Revert "Use "BINARY()" DQL function for case sensitive MySQL pattern …
phansys Apr 14, 2021
0a62b6e
Introduce "force_case_insensitivity" option and deprecate "case_sensi…
phansys Apr 14, 2021
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
16 changes: 6 additions & 10 deletions UPGRADE-3.x.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,16 +6,12 @@ UPGRADE FROM 3.x to 3.x

### Sonata\DoctrineORMAdminBundle\Filter\StringFilter

* Omitting or not passing an instance of `Doctrine\ORM\EntityManagerInterface` as argument 1 to `StringFilter::__construct()`
is deprecated.

* The default value for the "case_sensitive" option is changed from `true` to `null`, keeping the behavior of its previous value.

* In order to perform real case sensitive comparisons along the MySQL platform, the `BINARY()` function
will be used when "case_sensitive" option is set to `true` and the [`DoctrineExtensions\Query\Mysql\Binary`](https://github.com/beberlei/DoctrineExtensions#doctrineextensions)
DQL extension is registered using the name "binary". This behavior requires the package "beberlei/doctrineextensions".
If you want to keep the previous behavior, you can use `null` as value for the "case_sensitive" option.
For more information, see https://dev.mysql.com/doc/refman/8.0/en/string-comparison-functions.html#operator_like.
The option "case_sensitive" is deprecated in favor of "force_case_insensitivity".
You must pass `true` in this option when you need to force the matching criteria
to avoid honoring the case sensitivity in the filter values. Any other values than
`true` will cause the database to use its default behavior.
The option "case_sensitive" will be respected only if "force_case_insensitivity"
is not set.

UPGRADE FROM 3.31 to 3.32
=========================
Expand Down
2 changes: 0 additions & 2 deletions composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,6 @@
"sonata-project/admin-bundle-persistency-layer": "1.0.0"
},
"require-dev": {
"beberlei/doctrineextensions": "^1.3",
"doctrine/doctrine-fixtures-bundle": "^3.4",
"phpstan/extension-installer": "^1.0",
"phpstan/phpstan": "^0.12.52",
Expand All @@ -70,7 +69,6 @@
"vimeo/psalm": "^4.1.1"
},
"suggest": {
"beberlei/doctrineextensions": "If you need case sensitive comparisons using MySQL.",
"sonata-project/entity-audit-bundle": "If you want to support for versioning of entities and their associations."
},
"config": {
Expand Down
11 changes: 2 additions & 9 deletions docs/reference/filter_field_definition.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,18 +73,11 @@ StringFilter

The string filter has additional options:

* ``case_sensitive`` - set to ``false`` to make the search case insensitive. By default ``null`` is used;
* ``force_case_insensitivity`` - set to ``true`` to make the search case insensitive. By default ``false`` is used,
letting the database to apply its default behavior;
* ``trim`` - use one of ``Sonata\DoctrineORMAdminBundle\Filter\TRIM_*`` constants to control the clearing of blank spaces around in the value. By default ``Sonata\DoctrineORMAdminBundle\Filter\TRIM_BOTH`` is used;
* ``allow_empty`` - set to ``true`` to enable search by empty value. By default ``false`` is used.

.. versionadded:: 3.x

In order to perform real case sensitive comparisons along the MySQL platform, the ``BINARY()`` function
will be used when "case_sensitive" option is set to ``true`` and the `DoctrineExtensions\Query\Mysql\Binary <https://github.com/beberlei/DoctrineExtensions#doctrineextensions>`_
DQL extension is registered using the name "binary". This behavior requires the package "beberlei/doctrineextensions".
If you want to keep the previous behavior, you can use ``null`` as value for the ``case_sensitive`` option.
For more information, see https://dev.mysql.com/doc/refman/8.0/en/string-comparison-functions.html#operator_like.

StringListFilter
----------------

Expand Down
56 changes: 20 additions & 36 deletions src/Filter/StringFilter.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,6 @@

namespace Sonata\DoctrineORMAdminBundle\Filter;

use Doctrine\ORM\Configuration;
use Doctrine\ORM\EntityManagerInterface;
use DoctrineExtensions\Query\Mysql\Binary;
use Sonata\AdminBundle\Datagrid\ProxyQueryInterface as BaseProxyQueryInterface;
use Sonata\AdminBundle\Form\Type\Filter\ChoiceType;
use Sonata\AdminBundle\Form\Type\Operator\StringOperatorType;
Expand Down Expand Up @@ -50,29 +47,6 @@ class StringFilter extends Filter
StringOperatorType::TYPE_NOT_CONTAINS,
];

/**
* @var Configuration|null
*/
private $doctrineConfig;

/**
* NEXT_MAJOR: Do not accept null value in argument 1.
*/
public function __construct(?EntityManagerInterface $em = null)
{
if (null === $em) {
@trigger_error(sprintf(
'Omitting or passing other type than "%s" as argument 1 to "%s()" is deprecated since'
.' sonata-project/doctrine-orm-admin-bundle 3.x and will be not allowed in version 4.0.',
EntityManagerInterface::class,
__METHOD__
), \E_USER_DEPRECATED);
}
$this->doctrineConfig = $em ? $em->getConfiguration() : null;
// NEXT_MAJOR: Replace the previous line with the following one.
// $this->doctrineConfig = $em->getConfiguration();
}

public function filter(BaseProxyQueryInterface $query, $alias, $field, $data)
{
/* NEXT_MAJOR: Remove this deprecation and update the typehint */
Expand Down Expand Up @@ -113,16 +87,23 @@ public function filter(BaseProxyQueryInterface $query, $alias, $field, $data)
// c.name > '1' => c.name OPERATOR :FIELDNAME
$parameterName = $this->getNewParameterName($query);

$caseSensitive = $this->getOption('case_sensitive');
$forceCaseInsensitivity = true === $this->getOption('force_case_insensitivity', false);

// NEXT_MAJOR: Remove the following condition and its body.
if (null !== $this->getOption('case_sensitive')) {
@trigger_error(
'Option "case_sensitive" is deprecated since sonata-project/doctrine-orm-admin-bundle 3.x'
.' and will be removed in version 4.x. Use the "force_case_insensitivity" option instead.',
\E_USER_DEPRECATED
);

if (false === $caseSensitive && '' !== $data['value']) {
if (null === $this->getOption('force_case_insensitivity')) {
$forceCaseInsensitivity = false === $this->getOption('case_sensitive', true);
}
}

if ($forceCaseInsensitivity && '' !== $data['value']) {
$clause = 'LOWER(%s.%s) %s :%s';
} elseif (true === $caseSensitive && '' !== $data['value'] && null !== $this->doctrineConfig
&& Binary::class === $this->doctrineConfig->getCustomStringFunction('binary')
) {
// NEXT_MAJOR: Remove the type check against `$this->doctrineConfig` in the `elseif` condition.
// @see https://dev.mysql.com/doc/refman/8.0/en/string-comparison-functions.html#operator_like.
$clause = '%s.%s %s BINARY(:%s)';
} else {
$clause = '%s.%s %s :%s';
}
Expand Down Expand Up @@ -165,17 +146,20 @@ public function filter(BaseProxyQueryInterface $query, $alias, $field, $data)
$parameterName,
sprintf(
$format,
false !== $caseSensitive || '' === $data['value'] ? $data['value'] : mb_strtolower($data['value'])
true !== $forceCaseInsensitivity || '' === $data['value'] ? $data['value'] : mb_strtolower($data['value'])
)
);
}

public function getDefaultOptions()
{
return [
// NEXT_MAJOR: Remove the format option.
// NEXT_MAJOR: Remove the "format" option.
'format' => '%%%s%%',
// NEXT_MAJOR: Remove the "case_sensitive" option.
'case_sensitive' => null,
// NEXT_MAJOR: Use `false` as default value for the "force_case_insensitivity" option.
'force_case_insensitivity' => null,
Copy link
Member

Choose a reason for hiding this comment

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

Why null ?
What's the difference between null and false ?

If I understand the option, the only allowed value would be false and true

Copy link
Member Author

Choose a reason for hiding this comment

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

We haven't other way to detect if this option is set or not. Currently, I'm using the value from "case_sensitive" if this option is not set.
I think we have 2 choices: add a hasOption() method, or leave this value adding a NEXT_MAJOR comment.
WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

Why do you need to know if the option is set ?

If case_sensitive is not null, use this (deprecated) value
If case_sensitive is null, use force_case_insensitivity, which is by default to false.

Copy link
Member Author

Choose a reason for hiding this comment

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

Because I'd like the new option to be respected if it is set, regardless the value of the deprecated option.

Copy link
Member

Choose a reason for hiding this comment

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

Then we could add a NEXT_MAJOR comment to change the default value to false and remove the true === or true !== checks. But It feel weird to provide a null value and a false value which has the same behavior. And introducing a null default value which is already deprecated and will be changed to false in the next major...

But when I read force_case_insensitivity => false, case_sensitive => false:
I would expect

  • force_case_insensitivity won't force the case insensitivity so it will do nothing. (It never was said that false will force the case sensitivity)
  • case_sensitive will force the case insensitivity because it's already the behavior of this option

Copy link
Member Author

Choose a reason for hiding this comment

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

But when I read force_case_insensitivity => false, case_sensitive => false:
I would expect

force_case_insensitivity won't force the case insensitivity so it will do nothing. (It never was said that false will force the case sensitivity)
case_sensitive will force the case insensitivity because it's already the behavior of this option

I think the idea transmitted by the name case_sensitive leads to wrong assumptions. In fact, that's the reason for this change.
That's why I feel like a more natural behavior to completely ignore the option if its counterpart (force_case_insensitivity) is set.

Copy link
Member Author

Choose a reason for hiding this comment

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

The NEXT_MAJOR comment about the default value was added.

Copy link
Member Author

Choose a reason for hiding this comment

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

@VincentLanglet, I've added a comment in the upgrade document to make this behavior explicit.

'trim' => self::TRIM_BOTH,
'allow_empty' => false,
];
Expand Down
6 changes: 0 additions & 6 deletions src/Resources/config/doctrine_orm_filter_types.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,12 +41,6 @@
<tag name="sonata.admin.filter.type" alias="doctrine_orm_number"/>
</service>
<service id="sonata.admin.orm.filter.type.string" class="Sonata\DoctrineORMAdminBundle\Filter\StringFilter">
<argument type="service">
<service class="Doctrine\ORM\EntityManager">
<factory service="doctrine" method="getManager"/>
<argument>%sonata_doctrine_orm_admin.entity_manager%</argument>
</service>
</argument>
<tag name="sonata.admin.filter.type" alias="doctrine_orm_string"/>
</service>
<service id="sonata.admin.orm.filter.type.string_list" class="Sonata\DoctrineORMAdminBundle\Filter\StringListFilter">
Expand Down
23 changes: 7 additions & 16 deletions tests/Filter/FilterTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@

namespace Sonata\DoctrineORMAdminBundle\Tests\Filter;

use Doctrine\ORM\Configuration;
use Doctrine\ORM\EntityManagerInterface;
use Doctrine\ORM\Query\Expr;
use Sonata\AdminBundle\Datagrid\ProxyQueryInterface;
Expand Down Expand Up @@ -99,7 +98,8 @@ public function testOrExpression(string $expected, array $filterOptionsCollectio

$proxyQuery = new ProxyQuery($queryBuilder);

foreach ($filterOptionsCollection as [$filter, $orGroup, $defaultOptions, $field, $options]) {
foreach ($filterOptionsCollection as [$type, $orGroup, $defaultOptions, $field, $options]) {
$filter = new $type();
$filter->initialize($field, $defaultOptions);
$filter->setCondition(Filter::CONDITION_OR);
if (null !== $orGroup) {
Expand All @@ -117,21 +117,12 @@ public function testOrExpression(string $expected, array $filterOptionsCollectio

public function orExpressionProvider(): iterable
{
$doctrineConfig = $this->createMock(Configuration::class);
$doctrineConfig->method('getCustomStringFunction')
->with('binary')
->willReturn(null);

$em = $this->createStub(EntityManagerInterface::class);
$em->method('getConfiguration')
->willReturn($doctrineConfig);

yield 'Using "or_group" option' => [
'SELECT e FROM MyEntity e WHERE 1 = 2 AND (:parameter_1 = 4 OR 5 = 6)'
.' AND (e.project LIKE :project_0 OR e.version LIKE :version_1) AND 7 = 8',
[
[
new StringFilter($em),
StringFilter::class,
'my_admin',
[
'field_name' => 'project',
Expand All @@ -143,7 +134,7 @@ public function orExpressionProvider(): iterable
],
],
[
new StringFilter($em),
StringFilter::class,
'my_admin',
[
'field_name' => 'version',
Expand All @@ -162,7 +153,7 @@ public function orExpressionProvider(): iterable
.' AND e.project LIKE :project_0 AND 7 = 8',
[
[
new StringFilter($em),
StringFilter::class,
'my_admin',
[
'field_name' => 'project',
Expand All @@ -182,7 +173,7 @@ public function orExpressionProvider(): iterable
.' OR e.project LIKE :project_0 OR e.version LIKE :version_1) AND 7 = 8',
[
[
new StringFilter($em),
StringFilter::class,
null,
[
'field_name' => 'project',
Expand All @@ -194,7 +185,7 @@ public function orExpressionProvider(): iterable
],
],
[
new StringFilter($em),
StringFilter::class,
null,
[
'field_name' => 'version',
Expand Down
Loading