Fix nested associations and embedded properties - #7500
Conversation
18c6ca4 to
646cbcb
Compare
a009235 to
6a93dff
Compare
6a93dff to
1b66e5e
Compare
|
Just found an issue with the EntityFilter + nested associations, I am investigating. |
9fbf27a to
1a53387
Compare
| * property_name: string, | ||
| * } | ||
| */ | ||
| public function resolveNestedAssociations(?QueryBuilder $queryBuilder, EntityDto $rootEntityDto, string $propertyName, bool $mustEndWithAssociation = false): array |
There was a problem hiding this comment.
While extracting that logic from search, I also modified:
$queryBuilderis nullable so theresolveNestedAssociations()method can be called to resolve the nestedEntityDto& property name without applying the joins to the query (eg:EntityConfiguratoruse it to resolve the entity used to fetch choice list)$mustEndWithAssociationis used to allow property ending on an association field (eg:post.authorinstead ofpost.author.fullName) as required byEntityFilter
|
Issue on |
4823239 to
4758cb5
Compare
| // Fallback to support custom filters with unmapped property names | ||
| $resolvedProperty = [ | ||
| 'entity_dto' => $entityDto, | ||
| 'entity_alias' => current($queryBuilder->getRootAliases()), | ||
| 'property_name' => $originalPropertyName, | ||
| ]; |
There was a problem hiding this comment.
Without this fallback, AuthorWithMinPostsFilter was not working in the EasyAdmin demo project.
There was a problem hiding this comment.
Note about this:
Instead of try/catch, it would be cleaner to check if the filter is unmapped.
This may be achieved by checking the related form option as suggested by EA documentation:
https://symfony.com/bundles/EasyAdminBundle/current/filters.html#unmapped-filters
However, it looks like previous code was working on unmapped fields even if the mapped option was not specified.
For example, the custom filter from EA demo is not defining the mapped option, and it still work:
https://github.com/EasyCorp/easyadmin-demo/blob/main/src/Admin/Filter/AuthorWithMinPostsFilter.php
And if we replace the try/catch by a check on the form type option, this may introduce a BC break on unmapped filter that does not specify the unmapped option, but this should also fix this bug: #6386
There was a problem hiding this comment.
e8ad127 to
1be57a5
Compare
|
@javiereguiluz please review |
|
This is awesome! Should we add the new |
|
This PR is missing some tests, and I believe this one of the reasons why it's not reviewed/merged yet. |
1be57a5 to
6459419
Compare
* Fix filters on nested associations and embedded properties * Add tests for EntityRepository and EntityConfigurator * Import EasyCorp#7584 into main PR * Import EasyCorp#7585 into main PR * Fix tests after merge * remove outdated comment, change parameter order for createEntityDto and moved it back down * fix parameters for createEntityDto and reorder it * minor cleanup * Fix Phpstan issues * Fix PHP-Linter (I hope) * Add missing linebreaks --------- Co-authored-by: Sébastien Alfaiate <s.alfaiate@webarea.fr>
|
Do we need to notify Javier that this changed btw? Don't see him in the list of participants so not sure |
|
Hi @javiereguiluz, if you have any concern about this PR, please let us know so we can improve it. |
|
Hi, great work! I tested the latest patch on EA 5.1.0 and one thing that did not work was adding a field like this to my crudcontroller:
Where the CrudController is for users, and the property is users.coachProfile.labels (ManyToMany). This throws an error 'Can't get a way to read the property "id" in class "Doctrine\ORM\PersistentCollection"'. In this case the AssociationConfigurator does not distinguish to-one vs to-many via isSingleValuedAssociation()/isCollectionValuedAssociation(). It always assumes the final segment is to-one. Maybe this is out of scope for this issue? |
|
Hi @finnef, can you please try again and confirm if this is working now? I just pushed a fix for this. |
|
Great! that works fine now. Good addition. |
|
Ah, sorry to say there is still an issue. When accessing the NEW page of my CoachCrudcontroller (of entity user), which has an CollectionField::new('coachProfile.attachments'), so it should access user.coachProfile.attachments, I get an error "Uncaught PHP Exception Symfony\Component\PropertyAccess\Exception\NoSuchPropertyException: "Can't get a way to read the property "attachments" in class "App\Entity\User"." at PropertyAccessor.php line 456" |
|
Hi @finnef just pushed another fix, if you can test it |
|
Latest patch works everywhere for me! |
|
If it helps make a decision if this is gonna be merged anytime soon... |
|
This is now merged! Thanks a lot @Seb33300 and all reviewers and testers! While merging we did some changes (36b92aa):
|
|
Hi @javiereguiluz, I only have 1 concern regarding this point:
Isn't this a BC break if we used that alias inside a I have many places where I did something like that in my projects: AssociationField::new(...)
->setQueryBuilder(fn (QueryBuilder $queryBuilder) => $queryBuilder
->where('entity.field = :field')
->setParameter('field', $this->getMyValue())
) |
|
My bad, just checked the code and realized I misunderstood this line. Seems all good |
Same as #7295 with a different approach.
Fixes #7452, #6664 and #7514
In
resolveNestedAssociations()I have extracted the logic handling nested associations from search to make it reusable.It also properly handles doctrine embeddables.
It's now possible to use associations in filters using the dot syntax:
This PR also includes contributions from @KDederichs to reuse
resolveNestedAssociations()and fix usage of nested associations inAssociationFieldandCollectionField