DRUP-624 Add revisions to API Docs #161
Conversation
|
Added ApiDoc settings for configuring default revision options. |
| } | ||
|
|
||
| $map = [ | ||
| 'view' => "view all {$this->entityTypeId} revisions", |
There was a problem hiding this comment.
I think you are unintentionally mixing what is the scope that an entity access handler should handle and what is the scope of a route access handler.
https://github.com/fago/entity/blob/85f76b56f25d7c3f5bc466f94db30a9b63d97fc5/src/Access/EntityRevisionRouteAccessChecker.php#L86 This is a route access handler and you can see the similarity between your implementation and this class.
There was a problem hiding this comment.
So probably this is the right place to ask again, can we get rid of the custom entity access and entity route handlers that we implemented and rely on what the Entity module provides?
I would feel much safer if access handlers from a well-tested framework would ensure the entity access in our system.
There was a problem hiding this comment.
Yet an other reason why the custom implementation should go:
https://github.com/apigee/apigee-edge-drupal/pull/161/files/901a1e5c817a9447528c55d1cfe1524151f7e7ad#r274771293
modules/apigee_edge_apidocs/tests/src/Kernel/ApidocEntityTest.php
Outdated
Show resolved
Hide resolved
modules/apigee_edge_apidocs/tests/src/Kernel/ApidocEntityTest.php
Outdated
Show resolved
Hide resolved
modules/apigee_edge_apidocs/tests/src/Kernel/ApidocEntityRevisionsAccessTest.php
Outdated
Show resolved
Hide resolved
modules/apigee_edge_apidocs/tests/src/Kernel/ApidocEntityRevisionsAccessTest.php
Outdated
Show resolved
Hide resolved
arlina-espinoza
force-pushed
the
8.x-1.x-DRUP-624
branch
from
3f0f120 to
e88bc36
Compare
| } | ||
|
|
||
| $map = [ | ||
| 'view' => "view all {$this->entityTypeId} revisions", |
|
|
||
| // If no revision author has been set explicitly, make the current user | ||
| // the revision author. | ||
| if (!$this->getRevisionUser()) { |
There was a problem hiding this comment.
In Node, the owner id is the uid field, which we don't have in the ApiDoc entity.
Should not we have an owner on ApiDoc entities? :) This is kinda an OOTB feature on content entities (nodes) that now we are intentionally removing from API doc entities - although it could be useful in the content moderation workflows, ex.: the owner (creator) of an API documentation cannot publish the API documentation only an other evaluated user.
There was a problem hiding this comment.
As @arunz6161 requested I approved this PR, but please create follow up tasks for all remaining, unresolved comments in this PR, especially cleaning up the entity/route access checks on the API documentation entity type.
|
|
||
| // If no revision author has been set explicitly, make the current user | ||
| // the revision author. | ||
| if (!$this->getRevisionUser()) { |
There was a problem hiding this comment.
Since there is no BC guaranteed for experimental modules, we can revisit this later - together with replacing the custom entity access on this entity type with what Entity module provides.
| $default_revision = $entity_storage->load($entity->id()); | ||
| $entity_access_default = $default_revision->access($operation, $account); | ||
| if (!$entity_access_default) { | ||
| return AccessResult::forbidden(); |
There was a problem hiding this comment.
If you extend this class then yes it is cached, but this comment is in connection with my next comment
https://github.com/apigee/apigee-edge-drupal/pull/161/files#r271145040 in which I described that I think you are mixing what an entity access handler and an entity route access handler should do. Consequently, this class should not extend this EntityAccessControlHandler base class.
If you do not believe the Entity (contrib) module, this is how it is implemented for the Node entity in Drupal core:
This is the route access handler for node revision access - which cares about revision elated permissions.
https://github.com/drupal/core/blob/17557c97a816bc78e70989a86064c562bc27027f/modules/node/node.services.yml#L11
https://github.com/drupal/core/blob/17557c97a816bc78e70989a86064c562bc27027f/modules/node/src/Access/NodeRevisionAccessCheck.php#L93-L103
and this is the entity access handler on the Node entity that does not care about revisioning related permissions...
https://github.com/drupal/core/blob/17557c97a816bc78e70989a86064c562bc27027f/modules/node/src/Entity/Node.php#L31
https://github.com/drupal/core/blob/c784c6a8e416505fdbd7f8ab1a6608ad58d37dd9/modules/node/src/NodeAccessControlHandler.php
Also, there is this major difference between entity and route access handlers: https://www.drupal.org/node/2122195#multiple
Please create a follow-up task about cleaning up the access check on the API documentation entity type.
|
This PR has been moved to the API Catalog module: https://github.com/cnovak/apigee-api-catalog-drupal/pull/7 |
This will add revisioning to the API Docs entities: