-
Notifications
You must be signed in to change notification settings - Fork 18
IBX-9266: Added REST endpoint loading available site accesses for location #1759
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
base: 4.6
Are you sure you want to change the base?
Conversation
alongosz
left a comment
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.
Conceptually I think this is the proper placement for the endpoint.
Other remarks:
| */ | ||
| public function visit(Visitor $visitor, Generator $generator, $data): void | ||
| { | ||
| $generator->startObjectElement('SiteAccessesList'); |
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.
My preference here would be SiteAccessList similarly as we refer to it on other places afair and the same as ContentInfoList, LocationList, etc.
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.
The issue is that we also use SiteAccessesList in a lot of places in directly related services, e.g.: https://github.com/ibexa/admin-ui/blob/main/src/lib/Siteaccess/NonAdminSiteaccessResolver.php#L33 (a lot of other places in admin-ui), therefore I wanted to stay consistent with it as I'm directly using this service. But obviously I can change it if you prefer it your way.
|
|
||
| use Ibexa\Rest\Value as RestValue; | ||
|
|
||
| final class SiteAccessesList extends RestValue |
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.
and the same remark for the object itself as in the previous comment.
src/lib/REST/Output/ValueObjectVisitor/SiteAccess/SiteAccessesListVisitor.php
Outdated
Show resolved
Hide resolved
1860d6c to
0f54bf7
Compare
0f54bf7 to
0c0cfa9
Compare
| # | ||
|
|
||
| ibexa.rest.site_access.load_for_location: | ||
| path: /site-access/load-non-admin-for-location/{locationId} |
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 gonna lie, this URI terrifies me 😅
load-non-admin-for-location? Seems like extremely specific. It definitely violates the REST principle by putting some "action" (load) as part of the URI.
I'd suggest at the very least changing the part I mentioned.
If possible, I'd prefer this endpoint to be more generic. In my opinion ideal URI would look like this:
/site-access/by-location/{locationId}?resolver_type=non_admin
I would suggest to allow picking a specific type of resolver we're looking for by using a query parameter instead of creating separate links for each and every one (if/when they become needed).
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.
Resolved in 28dee5e
konradoboza
left a comment
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.
Paweł remark seems valid.
src/lib/REST/Output/ValueObjectVisitor/SiteAccess/SiteAccessesListVisitor.php
Outdated
Show resolved
Hide resolved
…ation # Conflicts: # src/bundle/Resources/config/routing_rest.yaml # src/bundle/Resources/config/services/controllers.yaml # src/bundle/Resources/config/services/rest.yaml
28dee5e to
558b550
Compare
| switch ($resolverType) { | ||
| default: $siteAccesses = $this->nonAdminSiteAccessResolver->getSiteAccessesListForLocation($location); | ||
| } |
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.
Shouldn't this be done as a part of resolving strategy that will be easily extended?
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 there really a point in creating a dedicated strategy if we handle just one resolver for now? We won't probably need another one is a long time - if ever.
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.
switch statement doesn't look good. An alternative could be a map but whitelisting isn't nice either maintenance-wise. @Steveb-p what did you have in mind in regards to shape of resolver determination based on query parameter?
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.
tagged_locator would be my choice for resolution, with current non-admin service being tagged.
EDIT:
Index the locator using keys:
$converters: !tagged_locator
tag: 'ibexa.<tag>'
index_by: typeAnd mark services accordingly:
tags:
- { name: 'ibexa.<tag>', type: 'non-admin' }For non-existent types, throw an exception. You can use the ServiceLocator type (which is injected when tagged_locator is used) to give user the info what types are available (through a dedicated method to acquire locator keys).
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.
please see 480f1d1
|



Related PRs:
ibexa/fieldtype-richtext#267
Description:
A new endpoint is exposed as
/api/ibexa/v2/siteaccess/load-non-admin-for-location/{locationId}that utilizes the existingSiteaccessResolverInterfacethat fetches eligible site accesses for a given location.Example response
{ "SiteAccessesList": { "_media-type": "application/vnd.ibexa.api.SiteAccessesList+json", "values": [ { "_media-type": "application/vnd.ibexa.api.SiteAccess+json", "name": "__default_site_access__", "provider": "Ibexa\\Core\\MVC\\Symfony\\SiteAccess\\Provider\\StaticSiteAccessProvider" }, { "_media-type": "application/vnd.ibexa.api.SiteAccess+json", "name": "__second_site_access__", "provider": "Ibexa\\Core\\MVC\\Symfony\\SiteAccess\\Provider\\StaticSiteAccessProvider" }, { "_media-type": "application/vnd.ibexa.api.SiteAccess+json", "name": "ger", "provider": "Ibexa\\Core\\MVC\\Symfony\\SiteAccess\\Provider\\StaticSiteAccessProvider" }, { "_media-type": "application/vnd.ibexa.api.SiteAccess+json", "name": "eng", "provider": "Ibexa\\Core\\MVC\\Symfony\\SiteAccess\\Provider\\StaticSiteAccessProvider" }, { "_media-type": "application/vnd.ibexa.api.SiteAccess+json", "name": "ku6\"H", "provider": "Ibexa\\Core\\MVC\\Symfony\\SiteAccess\\Provider\\StaticSiteAccessProvider" } ] } }For QA:
Documentation: