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

[Search] Allow Searching in nested ORM\Embedded property #3898

Open
wants to merge 2 commits into
base: 4.x
Choose a base branch
from

Conversation

gmorel
Copy link

@gmorel gmorel commented Oct 20, 2020

Pain Point

Thanks to #3864 you can now search on nested entities 🚀.
But if you try to search on an Entity which has nested Embedable you might have this error
Screenshot from 2020-10-20 18-52-46

CRUD config:
Screenshot from 2020-10-20 19-00-34

Usecase:

  • property (ORM\ManyToOne)
    • landlord (ORM\ManyToOne)
      • person1 (ORM\Embedded)
        • firstName (string)

Screenshot from 2020-10-20 18-54-38

It fails to find person1 since only person1.firstName is mapped in landlord.
Screenshot from 2020-10-20 18-54-12

TODO

  • Allow searching on nested embeddable
  • Unit Test

$propertyMetadata = $associatedEntityDto->getPropertyMetadata($associatedPropertyName);
$targetEntity = $propertyMetadata->get('targetEntity');
$associatedEntityDto = $this->entityFactory->create($targetEntity);
} catch (\InvalidArgumentException $exception) {
Copy link
Author

Choose a reason for hiding this comment

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

if fails to find metadata

$targetEntity = $propertyMetadata->get('targetEntity');
$associatedEntityDto = $this->entityFactory->create($targetEntity);
} catch (\InvalidArgumentException $exception) {
if ($this->isNestedEmbbededProperty($associatedProperties)) {
Copy link
Author

Choose a reason for hiding this comment

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

try embeddable

@gmorel gmorel force-pushed the feature/allow_search_in_nested_embeddable branch from 52c698d to 648d9c4 Compare October 20, 2020 17:13
$targetEntity = $propertyMetadata->get('targetEntity');
$associatedEntityDto = $this->entityFactory->create($targetEntity);
try {
$propertyMetadata = $associatedEntityDto->getPropertyMetadata($associatedPropertyName);
Copy link
Author

@gmorel gmorel Oct 20, 2020

Choose a reason for hiding this comment

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

Keep old behavior

@gmorel gmorel marked this pull request as ready for review October 20, 2020 17:14
@gmorel
Copy link
Author

gmorel commented Oct 20, 2020

I wanted to unit test it. But EntityRepository is not tested yet.

@gmorel gmorel changed the title [Search] Allow Searching in nested ORM\Embedded property [WIP] [Search] Allow Searching in nested ORM\Embedded property Oct 23, 2020
a.b.c where b is a ORM\Embedded
@gmorel gmorel force-pushed the feature/allow_search_in_nested_embeddable branch from 648d9c4 to ed9ef98 Compare October 23, 2020 15:53
Code was hard codded to fail if nested properties
But was failing with Doctrine ORM\Embedded too
@@ -21,7 +21,7 @@ public function supports(FieldDto $field, EntityDto $entityDto): bool
public function configure(FieldDto $field, EntityDto $entityDto, AdminContext $context): void
{
$isRenderedAsSwitch = true === $field->getCustomOption(BooleanField::OPTION_RENDER_AS_SWITCH);
if ($isRenderedAsSwitch && false !== strpos($field->getProperty(), '.')) {
if ($isRenderedAsSwitch && $this->isNestedProperty($field) && false === $this->isBoolean($field)) {
Copy link
Author

Choose a reason for hiding this comment

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

This was crashing the app during EDIT on boolean Doctrine Embedded properties

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this related to search? Seems like you are not fixing it but just ignoring it, or am I wrong?

I guess the AJAX request to change the value will not work with this change? Maybe remove this change and render the property as a normal boolean field, like the exception message says?

@gmorel gmorel changed the title [WIP] [Search] Allow Searching in nested ORM\Embedded property [Search] Allow Searching in nested ORM\Embedded property Oct 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants