-
-
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
feat(doctrine): searchfilter using id instead of full IRI when it's not an int #5771
base: main
Are you sure you want to change the base?
Conversation
7d50d3f
to
6cd4adf
Compare
6cd4adf
to
3c51edf
Compare
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.
A new feature should target main
@@ -1048,7 +1048,7 @@ Feature: Search filter on collections | |||
Scenario: Filters can use UUIDs | |||
Given there is a group object with uuid "61817181-0ecc-42fb-a6e7-d97f2ddcb344" and 2 users | |||
And there is a group object with uuid "32510d53-f737-4e70-8d9d-58e292c871f8" and 1 users | |||
When I send a "GET" request to "/issue5735/issue5735_users?groups[]=/issue5735/groups/61817181-0ecc-42fb-a6e7-d97f2ddcb344&groups[]=/issue5735/groups/32510d53-f737-4e70-8d9d-58e292c871f8" | |||
When I send a "GET" request to "/issue5735/issue5735_users?groups[]=61817181-0ecc-42fb-a6e7-d97f2ddcb344&groups[]=/issue5735/groups/32510d53-f737-4e70-8d9d-58e292c871f8" |
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 change a test please
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.
sorry, i was doing that at the same time as #5760 , i'll make separate tests.
parent::__construct($managerRegistry, $logger, $properties, $nameConverter); | ||
|
||
$this->iriConverter = $iriConverter; | ||
$this->identifiersExtractor = $identifiersExtractor; | ||
$this->propertyAccessor = $propertyAccessor ?: PropertyAccess::createPropertyAccessor(); | ||
$this->resourceMetadataFactory = $resourceMetadataFactory; | ||
$this->resourceClassResolver = $resourceClassResolver; |
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.
lots of new dependencies I wish we wouldn't have...
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.
yes, that's mostly why i didn't include this part in the previous PR. And that's kind of the point I was making in #5687 - some of the metadata is not easy to get to at times.
// dig into the subResource metadata to find out which field we're looking at | ||
if ($this->isResourceClass($associationResourceClass) && $this->resourceMetadataFactory) { | ||
$associationApiMetadata = $this->resourceMetadataFactory->create($associationResourceClass); | ||
$variables = $associationApiMetadata->getIterator()->current()->getUriVariables(); |
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.
you should call getOperation
instead of getIterator()->current()
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'll check that, thanks.
$variables = $associationApiMetadata->getIterator()->current()->getUriVariables(); | ||
if (1 === \count($variables)) { // otherwise let's just give up at this point, too complicated | ||
$varName = array_key_first($variables); | ||
$associationResourceApiIdField = $variables[$varName]->getIdentifiers()[0]; // this will be needed to get the correct value |
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.
agreed this is too complicated and deserves its own search filter...
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.
Yes, with multiple identifiers this becomes even clumsier to handle, and i'm not even sure what the request should look like anyway.
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.
Best would be to make some mix with #5732 to handle complicated stuff not sure how it'd apply on filters though
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.
A similar handlelinks option in filters?
That's a really interesting idea, but every filter would need to be reworked, that's quite an impact. I'd keep that idea in mind with #2400 for a major overhaul of filters, maybe?
3c51edf
to
d050fa0
Compare
Since this is based on a commit from 3.1 I wasn't sure this was the correct move...I'd have to rebase and make sure it still works, which feels awkward... |
d050fa0
to
2db8cc6
Compare
Finally found some time to rebase onto the updated main...would it be ok this way? |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
just stopping this from automatically closing...no time to work on it right now but i still want to... |
my bad :) |
This brings support for searches likes
/entities/?subEntity=subEntityId
when subEntity id is notint $id
.This is a follow up on #5760