-
Notifications
You must be signed in to change notification settings - Fork 49
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
chore(performance): use subresources & uriTemplate property #4968
Conversation
…large unnecessary join tables
⛔ Feature branch deployment currently inactive.If the PR is still open, you can add the |
@BacLuc: Now that we merged api-platform 3.3, I also enabled link security. Users without access to the parent object now receive a 404 instead of an empty list (similar as already implemented in #3610). Just in case, you want to have another look at it. This means, we now don't rely on |
$this->assertEquals("/periods/{$period2->getId()}/days", $responseArray['_embedded']['periods'][0]['_links']['days']['href']); | ||
$this->assertEquals("/periods/{$period1->getId()}/days", $responseArray['_embedded']['periods'][1]['_links']['days']['href']); |
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.
Is the order of these two periods in the response guaranteed? Or should you use $this->assertEqualsCanonicalizing
here?
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.
Yeah, should always be sorted by period start date:
https://github.com/ecamp/ecamp3/blob/devel/api/src/Entity/Camp.php#L98
period2 starts before period1:
https://github.com/ecamp/ecamp3/blob/devel/api/fixtures/periods.yml#L10
new GetCollection( | ||
security: 'is_authenticated()' | ||
), |
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.
As far as I see, you added the new GetCollections, but also kept the old, unfiltered ones everywhere. Shouldn't we switch to the new endpoints completely where possible? I think performance-wise we also found out that it doesn't make sense to fetch e.g. all schedule entries across all accessible camps, so this would enable us to disallow fetching the unfiltered collection, no?
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'd agree, that eventually, we should remove the unfiltered GetCollection (if everyone else agrees as well).
However, some additional works is necessary.
DayResponsibles
is also linked from
- CampCollaboration
- Period
ScheduleEntry
is also linked from
- Activity
- Day
Day
is not linked from any other entity in the backend, but in the frontend we use the /days
endpoint to load all days of a camp.
Overall, I'd vote for keeping this PR limited to introducing subresources as a concept. And further down the road, as we introduce more subresources we can (carefully) start to remove the main GetCollection endpoints.
Core Meeting DecisionWe want to replace our filters with sub resources where ever we can. |
Reopening #4940
in order to test the fix on api-platform api-platform/core#6313