-
-
Notifications
You must be signed in to change notification settings - Fork 868
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
fix: searchfilter with nested custom identifiers #5760
Conversation
76139cc
to
42f767d
Compare
$item = $this->getIriConverter()->getResourceFromIri($value, ['fetch_data' => false]); | ||
|
||
return $this->propertyAccessor->getValue($item, $associationFieldIdentifier); | ||
} catch (InvalidArgumentException) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not good for performances, isn't there a way to prevent this exception?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, that's not great for perfs but i can't see another way to check whether what i have is a valid IRI beforehand. That should not change much performance-wise though; I actually copied this logic from SearchFilterTrait::getIdFromValue, which was called before :
core/src/Doctrine/Common/Filter/SearchFilterTrait.php
Lines 121 to 132 in b58ec12
protected function getIdFromValue(string $value): mixed | |
{ | |
try { | |
$iriConverter = $this->getIriConverter(); | |
$item = $iriConverter->getResourceFromIri($value, ['fetch_data' => false]); | |
return $this->getPropertyAccessor()->getValue($item, 'id'); | |
} catch (InvalidArgumentException) { | |
// Do nothing, return the raw value | |
} | |
return $value; |
|
||
return $this->propertyAccessor->getValue($item, $associationFieldIdentifier); | ||
} catch (InvalidArgumentException) { | ||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually have another version that handle that case better, but i'll leave it for another PR since it needs quite a lot more to work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't worry let's go for this I think it's just find and maybe we can do better when working on #2400 next year!
could you rebase though? Thanks! |
42f767d
to
b7044d8
Compare
b7044d8
to
5f09a6a
Compare
Done! |
perfect thanks and @dannyvw tried on his project so fingers crossed :p |
This undoes the revert of #5623 , but (as far as i can see) fixes the part that was causing #5735