Refactor/clean-up Routes.php

You said "no reviews" but I couldn't resist a quick scan 😅

1. +++ b/src/Routing/Routes.php
   @@ -82,113 +85,196 @@ class Routes implements ContainerInjectionInterface {
   +    // Add routes for every resource type.
   +    foreach ($this->resourceTypeRepository->all() as $resource_type) {
   +      $routes->addCollection(static::getRoutesForResourceType($resource_type, $path_prefix));
   +    }
   

   I sure like the look of this.

2. +++ b/src/Routing/Routes.php
   @@ -82,113 +85,196 @@ class Routes implements ContainerInjectionInterface {
   +    // Enable all available authentication providers.
   +    $routes->addOptions(['_auth' => $this->authProviderList()]);
   

   TIL you can add something to all routes in a collection 😲

Oh haha, and then you suddenly did want reviews! On it :)

1. Does this help #2953346: Define related/relationship routes per field, not dynamically (with route parameters that need validating)?
2. 194 insertions, 102 deletions
   I'd like to see this delete more than it inserts.

3. +++ b/src/ParamConverter/ResourceTypeConverter.php
   @@ -13,6 +13,11 @@ use Symfony\Component\Routing\Route;
   +   * The route parameter type to match.
   

   Nit: Missing @var typehint.

4. +++ b/src/Routing/Routes.php
   @@ -82,113 +85,196 @@ class Routes implements ContainerInjectionInterface {
   +  protected function getRoutesForResourceType(ResourceType $resource_type, $path_prefix) {
   ...
   +    $path_prefix = $path_prefix . '/' . $resource_type->getPath();
   ...
   +  protected function getEntryPointRoute($path_prefix) {
   +    $entry_point = new Route("/{$path_prefix}");
   

   What if we instead use RouteCollection::addPrefix()? Then both the ::getRoutesForResourceType() helper method and the "entry point" one can be blissfully unaware of the prefix!

5. +++ b/src/Routing/Routes.php
   @@ -82,113 +85,196 @@ class Routes implements ContainerInjectionInterface {
   +    // Collection route, like /jsonapi/node/article.
   ...
   +    // Individual routes like `/jsonapi/node/article/{uuid}` or
   

   Nit: Inconsistent comments.

6. +++ b/src/Routing/Routes.php
   @@ -82,113 +85,196 @@ class Routes implements ContainerInjectionInterface {
   +    // @todo: separate the collection route from the individual resource
   +    // creation route.
   

   Why?

7. +++ b/src/Routing/Routes.php
   @@ -82,113 +85,196 @@ class Routes implements ContainerInjectionInterface {
   +    $collection_route = new Route("/{$path_prefix}");
   +    $collection_route->setMethods(['GET', 'POST']);
   +    $collection_route->addDefaults(['serialization_class' => JsonApiDocumentTopLevel::class]);
   +    $collection_route->setRequirement('_csrf_request_header_token', 'TRUE');
   

   These calls can be chained.

8. +++ b/src/Routing/Routes.php
   @@ -82,113 +85,196 @@ class Routes implements ContainerInjectionInterface {
   +    // Add the resource type as a parameter to every resource route.
   +    static::addCollectionParameter($routes, static::RESOURCE_TYPE_KEY, ['type' => ResourceTypeConverter::PARAM_TYPE_ID]);
   +    $routes->addDefaults([static::RESOURCE_TYPE_KEY => $resource_type->getTypeName()]);
   ...
   +   * The Symfony Route class only has a method for adding options which
   +   * overrides any previous values. Therefore, it is tedious to add a single
   +   * parameter while keeping those that are already set.
   

   But … we control what parameters option gets set, and it's always the same. D'oh, for all routes except the "collection" one, we indeed need this.

   Still, this is actually more complex than what we have today.

   AFAICT getIndividualRoutesForResourceType() (which does the "individual", "related" and "relationship" routes) actually could just call RouteCollection::addOptions()?

9. +++ b/src/Routing/Routes.php
   @@ -82,113 +85,196 @@ class Routes implements ContainerInjectionInterface {
   +    $entry_point->addDefaults([RouteObjectInterface::CONTROLLER_NAME => EntryPoint::class . '::index']);
   +    $entry_point->setRequirement('_permission', 'access jsonapi resource list');
   +    $entry_point->setMethods(['GET']);
   

   Chaining.

But there's a bug in the Symfony router that ends up adding a trailing / to the collection and entry point routes, making them inaccessible.

🤦‍♂️

7.7/7.9: heh, fair :) Keep it as-is, definitely not a deal breaker, just a nit.

178 insertions, 102 deletions

Better, but still not net-negative.

Trying to simplify further.

But there's a bug in the Symfony router that ends up adding a trailing / to the collection and entry point routes, making them inaccessible.

It seems I can't reproduce this bug. Let's see.

So there's only a problem with the "entry point" route due to the Symfony bugs that @gabesullice mentioned. Because Symfony inconsistently trims trailing lead slashes. Basically a / route that gets a prefix such as /foobar ends up with /foobar/, and there's nothing you can do about it.

Still, we can remove some of the path prefix handling.

(Now down to 174 insertions, 102 deletions.)

Interestingly, it seems nobody has reported this bug at https://github.com/symfony/symfony/issues?q=is%3Aissue+routecollection+a....

OTOH it seems like this is intentional; the intent is to preserve any trailing slashes. So I can't simplify #14 further wrt path prefixing.

\Drupal\jsonapi\Routing\Routes::authProviderList() and the injected service seem like they could go away. Since that code was written, #2308745: Remove rest.settings.yml, use rest_resource config entities happened (introduced a useful container parameter), and e.g. #2825204: REST views: authentication is broken started using it.

Next, the ::create() implementation is pretty long and atypical. That can be simplified.

Finally, one of the reasons the diffstat is not looking too great is because this is adding several multi-line todo comments. I moved those into issues: #2971649: Make '_is_jsonapi' route option a route default + #2971652: Separate the collection route from the individual resource creation route. And because these are optional enhancements, not must-haves, and their need is pre-existing rather than being introduced here, I'm removing the @todo comments altogether. Plus, @todos are easily ignored, open issues are not.

New diffstat: 173 insertions, 134 deletions. Still net growth. Most of that is of course in the form of comments — Drupal's documentation standards/requirements are quite onerous.

I don't see how this patch can be simplified further.

I can see how this patch makes things more consistent, and how it helps ensure things will remain more consistent. That being said, I'm not sure this is really simpler. Basically: the "get route" helper methods make things more consistent, but then due to Symfony routing system weirdness, we need several unfortunate work-arounds (addRouteParameter(), passing path prefix down to the first level of helpers) that undo some of the gained simplicity.

I don't have a strong opinion, if others do, I'm fine with this.