Skip to content

fix: searchfilter with nested custom identifiers - #5760

Merged
soyuka merged 1 commit into
api-platform:3.1from
mrossard:search-nested-fix
Aug 23, 2023
Merged

fix: searchfilter with nested custom identifiers#5760
soyuka merged 1 commit into
api-platform:3.1from
mrossard:search-nested-fix

Conversation

@mrossard

@mrossardmrossard commented Aug 19, 2023

Copy link
Copy Markdown
Contributor
QA
Branch?3.1
TicketsCloses5735

This undoes the revert of #5623 , but (as far as i can see) fixes the part that was causing #5735

@mrossard
mrossard marked this pull request as draft August 19, 2023 19:03
@mrossard
mrossardforce-pushed the search-nested-fix branch 4 times, most recently from 76139cc to 42f767dCompareAugust 20, 2023 07:32
@mrossard
mrossard marked this pull request as ready for review August 20, 2023 07:40
$item = $this->getIriConverter()->getResourceFromIri($value, ['fetch_data' => false]);

return $this->propertyAccessor->getValue($item, $associationFieldIdentifier);
} catch (InvalidArgumentException) {

Copy link
Copy Markdown
Member

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?

Copy link
Copy Markdown
ContributorAuthor

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 :

protectedfunctiongetIdFromValue(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) {
/*

@mrossardmrossardAug 22, 2023

Copy link
Copy Markdown
ContributorAuthor

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.

Copy link
Copy Markdown
Member

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!

@soyuka

Copy link
Copy Markdown
Member

could you rebase though? Thanks!

@mrossard

Copy link
Copy Markdown
ContributorAuthor

could you rebase though? Thanks!

Done!

@soyuka
soyuka merged commit a774f4c into api-platform:3.1Aug 23, 2023
@soyuka

Copy link
Copy Markdown
Member

perfect thanks and @dannyvw tried on his project so fingers crossed :p

Sign up for freeto 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

@mrossard@soyuka