Skip to content

Conversation

mrossard
Copy link
Contributor

@mrossard mrossard commented Aug 19, 2023

Q A
Branch? 3.1
Tickets Closes 5735

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 mrossard force-pushed the search-nested-fix branch 4 times, most recently from 76139cc to 42f767d Compare August 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
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
Contributor Author

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 :

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) {
/*
Copy link
Contributor Author

@mrossard mrossard Aug 22, 2023

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
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
Member

soyuka commented Aug 22, 2023

could you rebase though? Thanks!

@mrossard
Copy link
Contributor Author

could you rebase though? Thanks!

Done!

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

soyuka commented Aug 23, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants