🇦🇺Australia @Sam152

benjifisher → credited sam152 → .

It provides an API, there are other use cases: ✨ Option to stop content from being unpublished Needs review

I think this was attempted and rejected in #2784201: Track the latest_revision and latest_revision_translation_affected ID in the entity data table for revisionable entity types → .

greggles → credited Sam152 → .

Sam152 → created an issue.

I had a crack at something similar in contrib, yonks ago: https://www.drupal.org/project/responsive_image_automatic →

+1 for a an active maintainer, I don't have perms or I would grant it. While someone is in there, please remove me.

Not a bad feature, but I'd imagine this either belongs in a sub module or another contrib so folks have the option of using this or not?

GH63210714HG

That's the thing though -- in some systems, Class::method means a static call, and in others, it means instantiate the class and call $object->method.

Pre PHP 8 is_callable would return true for public methods that required instantiation, but this changed: https://3v4l.org/d93fd

Hence the reflection is no longer required and false from is_callable can accurately signal the need for instantiation or the container.

Sam152 → created an issue.

Sam152 → made their first commit to this issue’s fork.

Hm, the access check should probably be false, but given true was the default, this PR isn't changing anything, just making the current behaviour compatible with D10. Merged, thanks.

Sam152 → made their first commit to this issue’s fork.

I don't remember getting a DM, but go for it. Added @heddn as a full maintainer.

I did this on a side project with some naive filtering of results.

Removing the tag. I'm more than fine with not having the final say on this one, if someone else feels strongly about pushing it forward.

More subjectively I don't think the example given is a great candidate for a unit either, the body of the function itself doesn't contain a lot of complex branches. There is a higher risk that the configuration may change in a some unexpected way, the ContentModerationState entity being MIA or the entity being passed in not being under moderation for whatever reason etc.

  protected function getModerationStatusLabel(EntityInterface $entity) {
    return \Drupal\content_moderation\Entity\ContentModerationState::loadFromModeratedEntity($entity)
      ->workflow
      ->entity
      ->get('type_settings')['states'][$entity->moderation_state->value]['label'];
  }

Don't take it personally though, in my experience Drupal projects are generally not unit testable. Most failure and risk comes from bootstrapping complex structures and behaviours out of the database, plugging-in and alter-ing as a design pattern, on top of a core product and suite of modules that are evolving underneath you. You sort of have to boot the whole thing up before you can be confident anything is actually working :)

Personally I would avoid adding deprecations, disrupting existing code and introducing a new public service in place of APIs marked @internal, to satisfy testing requirements outside of core. Custom code always has the option of adding a new interface that thinly wrap this functionality.

The only reason setSyncing is required in this case is because content moderation forces a new revision without it. If you disabled moderation, it would not be required and you would trigger the same bug without it.

cause the entity to be indexed for each occurrence (have to investigate how the search api id would work) It's horrid with the Content Entity Datasource item ID,

I built this feature, which can be found here: https://www.drupal.org/sandbox/sam/3200275 →

or it can be indexed as if it's a multiple field.

Rolling in the above feature would not prevent this feature from also being written. Both would need to be configured and added to SAPI indexes, so the user could decide how they want the integration to work.

I believe the issue reported in the original IS is being fixed in #2921280: Error on adding a Date range field to the search API → .

I haven't done a detailed line by line review but I wanted to check out the patch, this is really impressive work, nice one folks!

Just a few observations made while taking it for a spin:

1. +++ b/core/modules/layout_builder/layout_builder.links.contextual.yml
   @@ -27,3 +27,13 @@ layout_builder_block_remove:
   +layout_builder_block_visibility:
   +  title: 'Control visibility'
   +  route_name: 'layout_builder.visibility'
   +  group: 'layout_builder_block'
   +  options:
   +    attributes:
   ...
   +      data-dialog-type: dialog
   +      data-dialog-renderer: off_canvas
   

   Testing this was kind of confusing, because all your contextual links get cached in the browser. New blocks got the links and old ones don't.

   Looks like this might be worked on in 🐛 New contextual links are not available after a module is installed Needs work though.

2. +++ b/core/modules/layout_builder/src/Form/BlockVisibilityForm.php
   @@ -0,0 +1,358 @@
   +    foreach ($this->conditionManager->getDefinitionsForContexts($this->getAvailableContexts($section_storage)) as $plugin_id => $definition) {
   +      $conditions_available_to_block[$plugin_id] = $definition['label'];
   +    }
   

   The condition manager already implements FilteredPluginManagerInterface, does it make sense to use getFilteredDefinitions here instead, so contribs like layout_builder_restrictions can choose to provide users a way of restricting this list.

   Also as a dev, if I decided I didn't want admins adding visibility conditions, could I use the same hook to NULL the list out and expect to see the contextual link disappear? This is perhaps a bit too much for a single issue, but perhaps worth a follow-up.

3. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/BlockVisibilityTest.php
   @@ -0,0 +1,315 @@
   +    $this->clickContextualLink(static::BODY_FIELDBLOCK_SELECTOR, 'Control visibility');
   

   Is "Control" the right label? Seems to use a mixture of "Control" and "Configure":

Re: #45.1, the strikethough tells me that has already landed 🙂

Thanks everyone for working on this, it would be a great step towards cutting out the work required to get to a node-like custom entity type.

My review is as follows. I think the biggest outstanding thing to resolve was mentioned back in #10, that is we shouldn't lock in and assume a set of magically named permissions for each entity type.

1. +++ b/core/lib/Drupal/Core/Entity/Controller/RevisionControllerTrait.php
   @@ -0,0 +1,200 @@
   +    // @todo Expand the entity storage to load multiple revisions.
   

   We should link to #1730874: Add support for loading multiple revisions at once → here.

2. +++ b/core/lib/Drupal/Core/Entity/Controller/RevisionControllerTrait.php
   @@ -0,0 +1,200 @@
   +      /** @var \Drupal\Core\Entity\ContentEntityInterface $revision */
   +      if ($revision->hasTranslation($langcode) && $revision->getTranslation($langcode)
   +          ->isRevisionTranslationAffected()
   +      ) {
   

   Not sure splitting this on to multiple lines makes it more readable.

3. +++ b/core/lib/Drupal/Core/Entity/Controller/RevisionControllerTrait.php
   @@ -0,0 +1,200 @@
   +    $build[$entity->getEntityTypeId() . '_revisions_table'] = [
   +      '#theme' => 'table',
   +      '#rows' => $rows,
   +      '#header' => $header,
   +    ];
   

   Is adding the entity type ID into render array keys just making this harder to override?

4. +++ b/core/lib/Drupal/Core/Entity/Controller/RevisionControllerTrait.php
   @@ -0,0 +1,200 @@
   +    // We have no clue about caching yet.
   +    $build['#cache']['max-age'] = 0;
   

   We should add a follow up here at least.

5. +++ b/core/lib/Drupal/Core/Entity/Controller/RevisionControllerTrait.php
   @@ -0,0 +1,200 @@
   +    if ($this->hasDeleteRevisionAccess($entity_revision)) {
   +      $links['delete'] = $this->buildDeleteRevisionLink($entity_revision);
   +    }
   
   +++ b/core/lib/Drupal/Core/Entity/Controller/RevisionOverviewController.php
   @@ -0,0 +1,160 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  protected function hasDeleteRevisionAccess(EntityInterface $entity) {
   +    return $this->currentUser()->hasPermission("delete all {$entity->id()} revisions");
   +  }
   

   We should probably add access controls to the delete route and then simply base visibility/access of the links themselves on those same access controls. If I deny access to a route outside of just the permission, that should propagate to all links to that route.

6. +++ b/core/lib/Drupal/Core/Entity/Controller/RevisionOverviewController.php
   @@ -0,0 +1,160 @@
   +  /**
   +   * Creates a new RevisionOverviewController instance.
   +   *
   +   * @param \Drupal\Core\Datetime\DateFormatterInterface $date_formatter
   +   *   The date formatter.
   +   */
   +  public function __construct(DateFormatterInterface $date_formatter, RendererInterface $renderer) {
   

   I disagree we even need to document these params, but if we document one we should probably document the other.

7. +++ b/core/lib/Drupal/Core/Entity/Controller/RevisionOverviewController.php
   @@ -0,0 +1,160 @@
   +  public function revisionOverviewController(RouteMatchInterface $route_match) {
   +    return $this->revisionOverview($route_match->getParameter($route_match->getRouteObject()->getOption('entity_type_id')));
   +  }
   

   I wonder why we need this method at all. Can't "revisionOverview" be the entry point?

8. +++ b/core/lib/Drupal/Core/Entity/Controller/RevisionOverviewController.php
   @@ -0,0 +1,160 @@
   +          'message' => ['#markup' => $markup, '#allowed_tags' => Xss::getHtmlTagList()],
   

   getRevisionLogMessage is the "value" column from a "string_long" field. I think that'd already be considered unsafe markup and would be escaped? Is "allowed_tags" making this more or less permissive?

9. +++ b/core/lib/Drupal/Core/Entity/Controller/RevisionOverviewController.php
   @@ -0,0 +1,160 @@
   +  protected function hasRevertRevisionAccess(EntityInterface $entity) {
   +    return AccessResult::allowedIfHasPermission($this->currentUser(), "revert all {$entity->getEntityTypeId()} revisions")->orIf(
   +      AccessResult::allowedIfHasPermission($this->currentUser(), "revert {$entity->bundle()} {$entity->getEntityTypeId()} revisions")
   +    );
   +  }
   

   I do agree with the previous comments, why isn't this $entity->access('revert'), with access control handlers deciding if such controls are based on a permission or not.

10. +++ b/core/lib/Drupal/Core/Entity/Entity.php
    @@ -322,7 +322,7 @@ protected function urlRouteParameters($rel) {
    -    if ($rel === 'revision' && $this instanceof RevisionableInterface) {
    +    if (in_array($rel, ['revision', 'revision-revert-form'], TRUE) && $this instanceof RevisionableInterface) {
           $uri_route_parameters[$this->getEntityTypeId() . '_revision'] = $this->getRevisionId();
         }
    

    We could possibly do #2927077: $entity->toUrl('revision-*') should fill revision parameter on all applicable routes. → ahead of this issue if we wanted to reduce the scope here.

11. +++ b/core/lib/Drupal/Core/Entity/EntityRevisionAccessCheck.php
    @@ -0,0 +1,162 @@
    +      $_entity = $route_match->getParameter($entity_type_id);
    ...
    +      $_entity_revision = $route_match->getParameter($entity_type_id . '_revision');
    

    Not sure why these are prefixed with "_"?

12. +++ b/core/lib/Drupal/Core/Entity/EntityRevisionAccessCheck.php
    @@ -0,0 +1,162 @@
    +  protected function checkAccess(ContentEntityInterface $entity, AccountInterface $account, $operation = 'view') {
    

    This method is a good example of why delegating to an access control handler might be a good idea. This hard codes bundle based granularity and also seperate permissions for update/delete/revert. What if my entity type is simple and I just want a single "administer revisions" permission to cover the whole lot?

13. +++ b/core/modules/system/tests/modules/entity_test/src/Entity/EntityTestRev.php
    @@ -24,6 +24,7 @@
    + *       "revision" = "Drupal\Core\Entity\Routing\RevisionRouteProvider",
    

    Given these are also providing HTML routes, maybe this should be RevisionHtmlRouteProvider for consistency?

14. +++ b/core/tests/Drupal/FunctionalTests/Routing/RevisionRouteAccessTest.php
    @@ -0,0 +1,84 @@
    + * @runTestsInSeparateProcesses
    + *
    + * @preserveGlobalState disabled
    

    What are these annotations for?