Define related/relationship routes per field, not dynamically (with route parameters that need validating)

However, I'd like to slate this for after core's inclusion.

I don't think one should prevent the other. I think they can happen in parallel.

At this point I feel we ought to fix the remaining issues and move the bureaucracy forward.

I agree that it is time for us to push the bureaucracy forward and indeed, I think we are! #2836165-37: New experimental module: JSON API

I wonder if my name for this issue makes the task sound like a much bigger undertaking than it is. Perhaps that "static" part could have been taken to mean "in YAML" or config or something like that.

By "statically," I just meant that the field name shouldn't be a route parameter and that we should create a route for each possible field, rather than handle the field dynamically.

Having a param just adds edge cases for us to handle unecessarily, since we really don't need to check anything "at runtime"

This would be a completely non-breaking change AFAICT

Getting this started again.

This patch won't apply until #2973784: JSON API should check entity access during routing, not in controller, for the "individual" route lands.

Haven't seriously addressed the review in #17, this is still a WIP. Although I did take the isMethodSafe suggestion.

This primarily gets rid of the access checks that the route-level access check makes obsolete.

One notable change that this introduces is that the errors no longer contain a pointer. That makes sense, there's no document to point at. Pointers only make sense when PATCHing a resource. They "point" to errors in the submission.

#2973784: JSON API should check entity access during routing, not in controller, for the "individual" route is in. Kicking tests. Still a WIP.

Handles the low-hanging fruit in those failures.

More orchard harvesting.

+++ b/tests/src/Kernel/Controller/EntityResourceTest.php
@@ -767,86 +767,6 @@ class EntityResourceTest extends JsonapiKernelTestBase {
-  public function testGetRelatedInternal() {
...
-  public function testGetRelationshipInternal() {
...
-  public function testCreateRelationshipInternal() {
...
-  public function testPatchRelationshipInternal() {
...
-  public function testDeleteRelationshipInternal() {

These kernel tests can all be removed because EntityResource no longer needs to check for 404s. Those routes don't even exist anymore!

Code standards.

🤞should be green.

Even if green, still not done. The next step will be to convert/remove the related route default (mentioned in #17.4). I'm not sure that we should remove it entirely, it's still useful as a parameter, but maybe it should be a route parameter (under options) rather than a default.

So happy to finally be doing this :)

1. +++ b/tests/src/Functional/JsonApiFunctionalTest.php
   @@ -772,7 +772,7 @@ class JsonApiFunctionalTest extends JsonApiFunctionalTestBase {
   -    $this->assertEquals(403, $response->getStatusCode());
   +    $this->assertEquals(401, $response->getStatusCode());
   

   Now, we're actually getting a correct error code :)

2. +++ b/src/Controller/EntityResource.php
   @@ -393,12 +391,9 @@ class EntityResource {
   -    $related_field = $this->resourceType->getInternalName($related_field);
   -    $this->relationshipAccess($entity, 'view', $related_field);
   ...
   -    $this->validateReferencedResource($field_list, $related_field);
   
   @@ -458,41 +453,13 @@ class EntityResource {
   -    $related_field = $this->resourceType->getInternalName($related_field);
   -    $this->relationshipAccess($entity, 'view', $related_field);
   ...
   -    $this->validateReferencedResource($field_list, $related_field);
   ...
   -  protected function validateReferencedResource($field_list, $related_field) {
   
   @@ -518,7 +485,6 @@ class EntityResource {
   -    $this->relationshipAccess($entity, 'update', $related_field);
   
   @@ -619,7 +585,6 @@ class EntityResource {
   -    $this->relationshipAccess($entity, 'update', $related_field);
   
   @@ -708,8 +673,6 @@ class EntityResource {
   -    $this->relationshipAccess($entity, 'update', $related_field);
   
   @@ -879,9 +842,6 @@ class EntityResource {
   -    if (!($field_list = $entity->get($related_field)) || !$this->isRelationshipField($field_list)) {
   -      throw new NotFoundHttpException(sprintf('The relationship %s is not present in this resource.', $related_field));
   -    }
   
   @@ -965,25 +925,6 @@ class EntityResource {
   -  protected function isRelationshipField(FieldItemListInterface $entity_field) {
   

   All of the above "request-time" logic is replaced by:

   +++ b/src/Access/RelationshipFieldAccess.php
   @@ -0,0 +1,65 @@
   +/**
   + * Defines a class to check access to related and relationship routes.
   ...
   + */
   +class RelationshipFieldAccess implements AccessInterface {
   
   ...
   
   +++ b/src/Routing/Routes.php
   @@ -230,25 +231,33 @@ class Routes implements ContainerInjectionInterface {
   +    foreach ($resource_type->getRelatableResourceTypes() as $relationship_field_name => $target_resource_types) {
   ...
   +      $relationship_route->setRequirement(RelationshipFieldAccess::ROUTE_REQUIREMENT_KEY, $relationship_field_name);
   ...
   +      $routes->add(static::getRouteName($resource_type, "$relationship_field_name.relationship"), $relationship_route);
   ...
   +      if (static::hasNonInternalTargetResourceTypes($target_resource_types)) {
   ...
   +        $related_route->setRequirement(RelationshipFieldAccess::ROUTE_REQUIREMENT_KEY, $relationship_field_name);
   +        $routes->add(static::getRouteName($resource_type, "$relationship_field_name.related"), $related_route);
   +      }
   

🙂

#17.1: Done, changed to isMethodCacheable() because of a deprecation warning.
2. ✔️
3. I just tried to do this, but I don't think it makes sense to do. I think that todo shouldn't have been put there. I think it had more to do with the normalizer's use of the request object than that default being a bad one.
4. #16 discusses this. It's a correct and useful value, it doesn't need to be removed. Removing it would mean having to inspect the route object again, which we tried hard to remove.

Addresses #29.