Conversation
|
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:
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.
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:
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! 🙌 |
|
@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. |
|
Thx @Seb33300 ! It's alright ! |
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
mappedoption asfalse.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:
https://github.com/EasyCorp/easyadmin-demo/blob/f21f3e9a7370c21780619601cb05dc7b4b0aeb95/src/Admin/Filter/AuthorWithMinPostsFilter.php#L41-L46