Allow to specify a route prefix per resource #1685
Conversation
Toflar
force-pushed
the
resource-route-prefix
branch
from
eaed80b to
481ccd8
Compare
src/Annotation/ApiResource.php
Outdated
| /** | ||
| * @var string | ||
| */ | ||
| public $routePrefix; |
There was a problem hiding this comment.
Do we really need a new attribute for that? Can't we use an attribute?
There was a problem hiding this comment.
Speaking about that, maybe can we just forward any top-level attribute passed to the annotation to attributes? This way it will be easier for the user. Anyway if we create a new attribute, the XML and the YAML loader must be extracted too (it's why I'm 👎).
There was a problem hiding this comment.
Well to be honest, I have no idea what qualifies for a top-level attribute and what can be part of attributes :-D
There was a problem hiding this comment.
@dunglas we should totally freeze current top-level stuff and only allow updates with optional attributes IMO
There was a problem hiding this comment.
Yeah I found it strange that I need to update all the factories. So I'm moving this to attributes?
There was a problem hiding this comment.
Yes please, I also think it's better to use attributes.
@soyuka what do you think about the fallback mechanism I was mentioning earlier?
| @@ -197,8 +198,11 @@ private function addRoute(RouteCollection $routeCollection, string $resourceClas | |||
| } | |||
| } | |||
|
|
|||
| $path = $resourceMetadata->getRoutePrefix() ?: ''; | |||
Toflar
force-pushed
the
resource-route-prefix
branch
from
ddf9734 to
c042ec7
Compare
|
Okay, comments addressed. Makes the PR a lot smaller 😄 Will do a docs PR once this is approved. |
| * @param string $operationType | ||
| * | ||
| * @throws RuntimeException | ||
| * @param RouteCollection $routeCollection |
There was a problem hiding this comment.
You can drop this block, we now don't add PHPDoc when not adding information compared to the typehints. Just keep the @throw if appropriate.
|
Docs PR linked. PR ready. |
| @@ -197,8 +193,11 @@ private function addRoute(RouteCollection $routeCollection, string $resourceClas | |||
| } | |||
| } | |||
|
|
|||
| $path = $resourceMetadata->getAttribute('routePrefix', ''); | |||
There was a problem hiding this comment.
What do you think about sanitizing the prefix with '/'.trim(trim($prefix, '/')) if it's defined (!== '')?
There was a problem hiding this comment.
I thought about that too but then my idea was that in Symfony, you always have to start with / both, for prefixes as well as routes. And if you define them incorrectly, you see an invalid path so that's a quickfix. Not sure we should do this :)
There was a problem hiding this comment.
My idea was just to follow the same logic as RouteCollection::addPrefix().
IMO it can improve the UX.
|
Just asking but should this option be applied even if |
|
To me, I think chances are higher that you would want to "namespace" the whole entity. So in fact, also when you use |
|
@Toflar I have seen several cases of entites with 10+ operations and only 2 routes with different namespace. If you for instance need several permissions + extensions to be applied for automatic filtering of the same collection based on user "type" |
|
The problem is, it's separated logic. The |
Toflar
force-pushed
the
resource-route-prefix
branch
from
24db267 to
aa4ecdb
Compare
|
Thanks @Toflar |
Sometimes it's useful to be able to specify a prefix for all operations of a given resource. In that case right now you have to override the paths of all operations individually.
The goal of this PR is to allow this: