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

Revert: Show matching filters in search results #7376

Merged
merged 1 commit into from
Aug 23, 2021

Conversation

core23
Copy link
Member

@core23 core23 commented Aug 9, 2021

Subject

This is a hotfix to make the search useable again if you have cascaded search filters.

    protected function configureDatagridFilters(DatagridMapper $filter): void
    {
        $filter
            ->add('addresses.city')
        ;
    }

The following code needs some rework to make the feature useable again:

 {% set match = name|split('.')|reduce((carry, v) => carry ? attribute(carry, v) : null, result) %}

I am targeting this branch, because #7005 break the search result.

Changelog

### Removed
 - Information about the matching filters in the search results, because it does not work for cascaded oneToMany filters.

@core23 core23 added the bug label Aug 9, 2021
@core23
Copy link
Member Author

core23 commented Aug 9, 2021

cc @phansys

@core23 core23 requested a review from a team August 9, 2021 19:53
Copy link
Member

@VincentLanglet VincentLanglet left a comment

Choose a reason for hiding this comment

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

Should we add a test to avoid the regression to come back ?

@phansys
Copy link
Member

phansys commented Aug 9, 2021

#7005 (comment)

If the feature is removed for some reason about nested properties in a collection, please make it clear in the description.
When I added these changes, I tested the behavior with nested accessors (like city.state.country), but not using collections (like in addresses.city).

@core23
Copy link
Member Author

core23 commented Aug 10, 2021

Should we add a test to avoid the regression to come back ?

It's a revert commit, because the feature does not work. Where do you want a regression test?

@VincentLanglet
Copy link
Member

It's a revert commit, because the feature does not work. Where do you want a regression test?

You're saying that

    protected function configureDatagridFilters(DatagridMapper $filter): void
    {
        $filter
            ->add('addresses.city')
        ;
    }

is not working anymore on this page. So you're fixing a bug, by removing partially the feature.

When phansys added the feature, he added multiple tests ; no this one.
If someone try to add this feature again later, he could re-add the bug, since there is no test preventing him.

Best would be to add a testSearchResultPageWorksWithCascadedSearchFilter test, to avoid someone to re-add the bug.

@core23
Copy link
Member Author

core23 commented Aug 15, 2021

When phansys added the feature, he added multiple tests ; no this one.

I did not see a single test for this feature #7005

Best would be to add a testSearchResultPageWorksWithCascadedSearchFilter test, to avoid someone to re-add the bug.

That's right, it would be good to have a specific test for this. Unfortunately this is a twig bug and can't be covered with a simple php unit test. I don't have much time to create a functional test for a broken feature that I did not create.

In the past we just reverted changes that are breaking and started from scratch when reimplementing the feature.

Copy link
Contributor

@dmaicher dmaicher left a comment

Choose a reason for hiding this comment

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

Simple revert seems good to me here

@VincentLanglet
Copy link
Member

Might better to use the v4-implementation way

But still ok

@core23 core23 merged commit bac4d0d into sonata-project:3.x Aug 23, 2021
@core23 core23 deleted the revert-7005 branch August 23, 2021 18:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants