Skip to content

Improve unmapped filters#7507

Draft
Seb33300 wants to merge 2 commits intoEasyCorp:5.xfrom
Seb33300:unmapped-filters
Draft

Improve unmapped filters#7507
Seb33300 wants to merge 2 commits intoEasyCorp:5.xfrom
Seb33300:unmapped-filters

Conversation

@Seb33300
Copy link
Copy Markdown
Contributor

@Seb33300 Seb33300 commented Mar 13, 2026

PR to address this comment: #7500 (comment)
#7500 must be merged first, I was not able to target my other PR as base branch so this PR also include changes from #7500, but they will disappear after #7500 is merged.

If we merge this PR, this will break unmapped filters that do not specify the mapped option as false.
That's why I create a separate PR. Maybe you don't want to enforce this.

Eg: The custom filter in EA demo will need to be updated to add that option here:

$filter->dto->setFormTypeOption('mapped', false);

https://github.com/EasyCorp/easyadmin-demo/blob/f21f3e9a7370c21780619601cb05dc7b4b0aeb95/src/Admin/Filter/AuthorWithMinPostsFilter.php#L41-L46

@Amoifr
Copy link
Copy Markdown

Amoifr commented Mar 25, 2026

Hey @Seb33300, thanks for working on this! The approach of extracting and reusing the nested association logic from search into filters looks really clean and well-thought-out. 👍

I have a couple of observations that might be worth discussing:

  1. State in EntityRepository

I noticed the PR removes the readonly modifier from EntityRepository and introduces an instance property $associationAlreadyJoined. Since this class is typically a shared service in Symfony's container, could there be any edge cases where this state persists unexpectedly across different operations within the same request? It might be worth considering whether this could be passed as a parameter or reset at the beginning of each query building operation instead.

  1. Breaking change for unmapped filters

You mentioned in the PR description that this will break existing unmapped filters that don't explicitly set 'mapped' => false. That's a fair trade-off for a cleaner API, but maybe it would help to:

  • Add a note in the UPGRADE/CHANGELOG about this change
  • Or consider a deprecation warning first (detecting unmapped filters without the explicit option and logging a deprecation)

This way, users upgrading won't be caught off guard.

Otherwise, this is a great improvement for anyone working with nested associations in filters. Thanks for taking the time to make this reusable! 🙌

@Seb33300
Copy link
Copy Markdown
Contributor Author

Seb33300 commented Mar 25, 2026

@Amoifr the same PR without the BC break is available here: #7500

Regarding the shared property, it was intentional in order to not re apply the same joins on the same query while combining global search and filters.

This property is only used by search and filters. Those features can only be used on the index page. And it's not possible to render multiple tables on the same index page at the moment so this should not be an issue.

@Amoifr
Copy link
Copy Markdown

Amoifr commented Mar 25, 2026

Thx @Seb33300 ! It's alright !

@Seb33300 Seb33300 marked this pull request as draft April 15, 2026 03:49
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.

2 participants