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

Introduce AsFilter attribute to define filter type and form type #348

Open
wants to merge 3 commits into
base: 1.14
Choose a base branch
from

Conversation

vasilvestre
Copy link

@vasilvestre vasilvestre commented Oct 21, 2024

This PR introduces a new attribute, AsFilter, to provide a more concise and structured way of defining filter types and form types for grid filters.

Previously, filter types and form types were defined two methods inherited from ConfigurableFilterInterface.

By using the AsFilter attribute, developers can now explicitly declare the type and form type of a filter directly above the class definition. This approach improves code readability, reduces reliance on tag attributes, and enhances maintainability.

Steps:

  • Create the attribute and implement it's logic
  • Test
  • Documentation (should be on Sylius stack now)

Example usage:

use Sylius\Component\Grid\Filtering\FilterInterface;
use Sylius\Component\Grid\Metadata\AsFilter;

#[AsFilter(type: 'product_name', formType: ProductNameFilterType::class)]
final class ProductNameFilter implements FilterInterface
{
    public function apply(DataSourceInterface $dataSource, string $name, $data, array $options): void
    {
        if (empty($data['value'])) {
            return;
        }

        $queryBuilder = $dataSource->getQueryBuilder();
        $rootAlias = $queryBuilder->getRootAliases()[0];

        $queryBuilder
            ->andWhere(sprintf('%s.name LIKE :name', $rootAlias))
            ->setParameter('name', '%' . $data['value'] . '%');
    }
}

The RegisterFiltersPass compiler pass has been updated to check for the AsFilter attribute and ignore duplicate filter configuration.

Tests are provided in the PR, documentation too.

A new behaviour is that type is now optional as the FQCN is now used by default. Which means you can reference it directly in PHP or YAML .

Future considerations:

In a future PR, we can consider deprecating the ConfigurableFilterInterface as it becomes redundant by introducing the AsFilter attribute. This will further simplify the process of creating grid filters and improve consistency.

@vasilvestre vasilvestre changed the title feat(attributes): AsGrid attribute feat(attributes): AsFilter attribute Oct 21, 2024
@vasilvestre vasilvestre force-pushed the feat/atttributes/asgrid branch 2 times, most recently from bfb5387 to 4b58c75 Compare October 21, 2024 16:07
@vasilvestre vasilvestre changed the title feat(attributes): AsFilter attribute Introduce AsFilter attribute for defining filter type and form type Oct 21, 2024
@loic425 loic425 changed the base branch from 1.13 to 1.14 October 22, 2024 08:46
@vasilvestre vasilvestre force-pushed the feat/atttributes/asgrid branch from cd7c39a to 215bc11 Compare October 22, 2024 08:59
@vasilvestre vasilvestre force-pushed the feat/atttributes/asgrid branch from f6641a8 to 12a8575 Compare October 22, 2024 09:10
@vasilvestre vasilvestre force-pushed the feat/atttributes/asgrid branch from c62a533 to f63f29a Compare October 22, 2024 09:30
loic425
loic425 previously approved these changes Oct 22, 2024
@vasilvestre vasilvestre force-pushed the feat/atttributes/asgrid branch from f63f29a to 50b13cc Compare October 22, 2024 09:56
@vasilvestre vasilvestre force-pushed the feat/atttributes/asgrid branch from 50b13cc to 81ae79d Compare October 22, 2024 10:33
@Roshyo Roshyo self-requested a review October 22, 2024 11:40
@loic425 loic425 changed the title Introduce AsFilter attribute for defining filter type and form type Introduce AsFilter attribute to define filter type and form type Oct 22, 2024
src/Component/Metadata/AsFilter.php Outdated Show resolved Hide resolved
src/Component/Metadata/AsFilter.php Outdated Show resolved Hide resolved
src/Component/Metadata/AsFilter.php Outdated Show resolved Hide resolved
src/Bundle/DependencyInjection/SyliusGridExtension.php Outdated Show resolved Hide resolved
src/Bundle/DependencyInjection/SyliusGridExtension.php Outdated Show resolved Hide resolved
src/Component/Metadata/AsFilter.php Outdated Show resolved Hide resolved
@loic425
Copy link
Member

loic425 commented Oct 23, 2024

@vasilvestre you have two commits with "-" as commit message
https://github.com/Sylius/SyliusGridBundle/pull/348/commits

@vasilvestre vasilvestre force-pushed the feat/atttributes/asgrid branch from 9edf05d to 52e0442 Compare October 23, 2024 07:59
@vasilvestre vasilvestre requested a review from diimpp October 23, 2024 08:02
@vasilvestre vasilvestre force-pushed the feat/atttributes/asgrid branch 2 times, most recently from ccb9197 to a106327 Compare October 23, 2024 08:53
@vasilvestre vasilvestre requested a review from diimpp October 25, 2024 08:54
@vasilvestre vasilvestre force-pushed the feat/atttributes/asgrid branch from 1c49e46 to f500a89 Compare October 25, 2024 09:00
@@ -31,6 +32,11 @@
null,
['author.nationality'],
))
->addFilter(AttributeNationalityFilter::create(
AttributeNationalityFilter::class,
Copy link
Member

Choose a reason for hiding this comment

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

it seems to be incorrect, or at least misleading

Copy link
Author

Choose a reason for hiding this comment

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

Why ? We can use FQCN as name and also call the create method of the filter.

Copy link
Member

@loic425 loic425 Nov 20, 2024

Choose a reason for hiding this comment

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

Yes, this is a bit strange... but I understand why.
The FQCN is the type/name of the filter.

Copy link
Member

@loic425 loic425 Nov 20, 2024

Choose a reason for hiding this comment

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

@vasilvestre Could you remember me why you don't use the Filter::create method instead?

Copy link
Author

Choose a reason for hiding this comment

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

I did line 35

@@ -31,6 +32,11 @@
null,
['author.nationality'],
))
->addFilter(AttributeNationalityFilter::create(
Copy link
Member

Choose a reason for hiding this comment

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

How is it tested? Does it override nationality filter and is covered by this test:

public function it_includes_all_rows_even_when_sorting_by_a_nullable_path(): void
{
$this->client->request('GET', '/authors/');
$totalItemsCountBeforeSorting = count($this->getAuthorNamesFromResponse());
$this->client->request('GET', '/authors/?sorting[nationality]=desc');
$totalItemsCountAfterSorting = count($this->getAuthorNamesFromResponse());
$this->assertSame($totalItemsCountBeforeSorting, $totalItemsCountAfterSorting);
}
or is not covered by test and we don't know if it works?

Copy link
Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

I added tests on the RegisterFiltersPass itself.

@loic425 loic425 dismissed their stale review December 17, 2024 16:29

I've tried locally and have seen some errors.

Copy link
Member

@loic425 loic425 left a comment

Choose a reason for hiding this comment

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

Does not change it yourself, I have tests in progress.

src/Component/Attribute/AsFilter.php Outdated Show resolved Hide resolved
@loic425 loic425 force-pushed the feat/atttributes/asgrid branch from ce6ee05 to 8986a33 Compare December 18, 2024 09:02
@loic425 loic425 force-pushed the feat/atttributes/asgrid branch from 8986a33 to bf029b4 Compare December 18, 2024 09:04
$container->registerForAutoconfiguration(FilterInterface::class)
->addTag('sylius.grid_filter')
->addTag('sylius.legacy_grid_filter')
Copy link
Member

@loic425 loic425 Dec 18, 2024

Choose a reason for hiding this comment

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

We now tagged the Attribute instead, but we need the ones that implements TypeAwareFilterInterface and FormTypeAwareFilterInterface which contains static methods for the type and the form type.

@@ -28,10 +30,21 @@ public function process(ContainerBuilder $container): void
return;
}

$registry = $container->getDefinition('sylius.registry.grid_filter');
$filterRegistry = $container->getDefinition('sylius.registry.grid_filter');
Copy link
Member

Choose a reason for hiding this comment

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

If registry is renamed to filterRegistry, then formTypeRegistry should be become filterFormTypeRegister, hence I propose to remove this change.

Suggested change
$filterRegistry = $container->getDefinition('sylius.registry.grid_filter');
$registry = $container->getDefinition('sylius.registry.grid_filter');

$registry->addMethodCall('register', [$type ?? $attribute['type'], new Reference($id)]);
$formTypeRegistry->addMethodCall('add', [$type ?? $attribute['type'], 'default', $formType ?? $attribute['form_type']]);
if (null === $formType && null === ($attribute['form_type'] ?? null)) {
throw new InvalidArgumentException(sprintf('Tagged grid filters needs to have "form_type" attribute or implements "%s".', FormTypeAwareFilterInterface::class));
Copy link
Member

Choose a reason for hiding this comment

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

According to SF CS, which is basis for sylius CS, global classes should be used directly and not added to use.

Suggested change
throw new InvalidArgumentException(sprintf('Tagged grid filters needs to have "form_type" attribute or implements "%s".', FormTypeAwareFilterInterface::class));
throw new \InvalidArgumentException(sprintf('Tagged grid filters needs to have "form_type" attribute or implements "%s".', FormTypeAwareFilterInterface::class));

@@ -14,10 +14,12 @@
namespace Sylius\Bundle\GridBundle\DependencyInjection\Compiler;

use InvalidArgumentException;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
use InvalidArgumentException;


namespace App\Filter;

use App\Grid\Type\NationalityFilterType;
Copy link
Member

Choose a reason for hiding this comment

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

Please move form type to usual SF placement.

Suggested change
use App\Grid\Type\NationalityFilterType;
use App\Form\Type\Grid\NationalityFilterType;

]);
},
);

$container->registerForAutoconfiguration(FilterInterface::class)
Copy link
Member

Choose a reason for hiding this comment

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

we should use this interface instead, that was a mistake to use it cause the FilterInterface itself is not enough to auconfigure it.

Suggested change
$container->registerForAutoconfiguration(FilterInterface::class)
$container->registerForAutoconfiguration(ConfigurableFilterInterface::class)

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.

5 participants