Problem/Motivation

If you create a pending revision, this is not loaded by EntityConverter. This is correct sometimes, but not all the time. The default revision makes sense for displaying content but for forms, without loading the latest revision, all the data in a pending revision is lost when the form is saved.

Proposed resolution

Content moderation and workbench moderation both rolled their own solution to this problem, but this a more generic problem to do with pending revisions. That is, this shouldn't be either of these modules concerns. This also has come up in a requirement for quickedit, which has to load the latest revision to update field values on pages displaying a pending revision, #2815221: Add ability to use Quick Edit to the latest_revision route.

Remaining tasks

Fix this problem and give modules utilising pending revisions a much easier time integrating with the rest core.

User interface changes

TBD.

API changes

TBD.

Data model changes

TBD.

CommentFileSizeAuthor
#84 2865616-84.patch14 KBSam152
#84 interdiff.txt2.94 KBSam152
#62 2865616-62.patch13.79 KBplach
#62 interdiff.txt1.38 KBplach
#50 ab-2865616-50.txt3.26 KBplach
#49 ab-2865616-49.txt3.71 KBplach
#47 2865616-46.patch14.02 KBSam152
#46 interdiff.txt747 bytesSam152
#43 2865616-43.patch13.98 KBSam152
#43 interdiff.txt22.66 KBSam152
#42 2865616-40.patch15.77 KBSam152
#40 interdiff.txt5.4 KBSam152
#40 2865616-40.patch5.4 KBSam152
#33 2865616-33.patch14.08 KBSam152
#33 interdiff.txt838 bytesSam152
#29 2865616-29.patch14.07 KBSam152
#29 interdiff.txt6.56 KBSam152
#25 2865616-25.patch11.29 KBSam152
#25 interdiff.txt2.47 KBSam152
#20 2865616-20.patch10.94 KBSam152
#20 interdiff.txt875 bytesSam152
#19 interdiff.txt4.87 KBSam152
#19 2865616-19.patch10.94 KBSam152
#18 2865616-18.patch6.77 KBSam152
#18 interdiff.txt3.99 KBSam152
#16 2865616-16-REPLACE-CM-CONVERTER.patch17.15 KBSam152
#16 2865616-16.patch7.05 KBSam152
#16 interdiff.txt4.72 KBSam152
#12 2865616-12.patch3.19 KBSam152
#12 interdiff.txt881 bytesSam152
#8 2865616-8_REPLACE_CM_CONVERTER.patch11.94 KBSam152
#8 2865616-8.patch3.19 KBSam152
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Sam152 created an issue. See original summary.

Sam152’s picture

Issue tags: +Workflow Initiative
timmillwood’s picture

Today I learnt: There are two EntityRevisionConverter classes, one in Workflows, one in ContentModeration.

Don't think we're ready yet to move any of this into Drupal\Core\ParamConverter yet.

Sam152’s picture

At this stage we couldn't without a way of query for latest revision in core, hence the followup to the linked issue.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Sam152’s picture

Title: Move EntityRevisionConverter to core » Add an option for EntityConverter to load the latest entity revision and fix all entity forms to use this option.
Component: content_moderation.module » entity system
Issue summary: View changes

Rejigging this a little bit and updating the IS. I think this could be framed as a generic entity API issue, because as we all know forward revisions are meant to be part of entity API.

Sam152’s picture

Issue summary: View changes
Sam152’s picture

Another issue that would be a lot easier with better latest revision handling, but copying in the query for now. Big cleanup in CM + a tool that other modules (quickedit/WBM/workspaces et al) can use without a duplicate implementation or soft dependency on content_moderation. Patch would get even smaller once getLatestRevision drops.

First patch just adds the feature and fixing the "bug", the second will test as a complete replacement to CM's converter.

Sam152’s picture

Issue summary: View changes

The last submitted patch, 8: 2865616-8.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Status: Needs review » Needs work

The last submitted patch, 8: 2865616-8_REPLACE_CM_CONVERTER.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Sam152’s picture

Status: Needs work » Needs review
FileSize
881 bytes
3.19 KB

Fixing some tests.

dawehner’s picture

Issue tags: +Needs tests

It feels like we should have some level of test coverage here :)

+++ b/core/lib/Drupal/Core/ParamConverter/EntityConverter.php
@@ -62,6 +64,25 @@ public function convert($value, $definition, $name, array $defaults) {
+      // If the entity type is revisionable, load the latest revision.
+      if (!empty($definition['_load_latest_revision']) && $entity->getEntityType()->isRevisionable()) {
+        // @todo, replace this with query with a standardised way of getting the
+        // latest revision in https://www.drupal.org/node/2784201.
+        $entity_revisions = $storage
+          ->getQuery()
+          ->allRevisions()
+          ->condition($entity->getEntityType()->getKey('id'), $entity->id())
+          ->sort($entity->getEntityType()->getKey('revision'), 'DESC')
+          ->range(0, 1)
+          ->execute();
+        $revision_ids = array_keys($entity_revisions);
+        $latest_revision_id = array_shift($revision_ids);
+        if ($entity->getRevisionId() != $latest_revision_id) {
+          $entity = $storage->loadRevision($latest_revision_id);
+        }

Can we add a loadLatestRevision method to Entity storage or \Drupal\Core\Entity\EntityRepositoryInterface ?

Sam152’s picture

Tests incoming! I was curious to see if works as a complete drop-in for what content_moderation has. Based on #8 having the same amount of fails, I think it does.

Re: loadLatestRevision, we absolutely need something like that. If you have input, please add it to #2784201: Track the latest_revision and latest_revision_translation_affected ID in the entity data table for revisionable entity types. I haven't seen EntityRepositoryInterface suggested yet.

Sam152’s picture

Status: Needs review » Needs work

NW for tests.

Sam152’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
4.72 KB
7.05 KB
17.15 KB

Adding some tests. Also another version of the patch which should proves content_moderation should no longer be concerned with entity forms and forward revisions.

amateescu’s picture

This looks awesome! Especially the 'replace-cm-converter' patch. Here are a few points that I found:

  1. +++ b/core/lib/Drupal/Core/EventSubscriber/EntityRouteAlterSubscriber.php
    @@ -50,10 +50,30 @@ public function onRoutingRouteAlterSetType(RouteBuildEvent $event) {
    +        $parameter['_load_latest_revision'] = TRUE;
    

    As far as I've seen in other core routing files, custom parameters are not prefixed with an underscore, so I think we can drop it as well. Some examples are in views_ui.routing.yml (the 'tempstore' stuff) and node.routing.yml (the 'with_config_overrides' stuff) :)

    Although, looking into this made we wonder if this should be a route option instead of a parameter, in which case it should be prefixed with an underscore..

  2. +++ b/core/lib/Drupal/Core/EventSubscriber/EntityRouteAlterSubscriber.php
    @@ -50,10 +50,30 @@ public function onRoutingRouteAlterSetType(RouteBuildEvent $event) {
    +    $events[RoutingEvents::ALTER][] = ['onRoutingRouteAlterSetLatestRevision', -150];
    

    Should we have a different priority for the new event? I don't think that equal priorities is the norm, so we can at least have some minimal difference between them.

  3. +++ b/core/lib/Drupal/Core/ParamConverter/EntityConverter.php
    @@ -4,7 +4,9 @@
    +use Drupal\Core\Entity\RevisionableInterface;
    ...
    +use Drupal\node\Entity\Node;
    

    These doesn't seem to be used by the new code added in this file.

  4. +++ b/core/tests/Drupal/KernelTests/Core/ParamConverter/EntityConverterLatestRevisionTest.php
    @@ -0,0 +1,125 @@
    + * Test the entity converter when the _load_latest_revision flag is set.
    

    We need some quotes around the name of the option/parameter.

Sam152’s picture

Thanks for the review, notes below.

1. I wasn't sure what the rules were here, updated. Converters, since they are only dealing with individual parameters in a route are only given the parameter as context, so a route option would be tricky. Additionally for an entity form route with multiple entity parameters, we'd force the latest revision to be loaded for each param even if it wasn't the form for the entity in question. I think that behaviour is probably correct, but we should allow people to override it by first checking if someone has possibly specified "load_latest_revision" => FALSE.
2. Done.
3. Good catch.
4. Done.

Sam152’s picture

Sam152’s picture

+++ b/core/tests/Drupal/Tests/Core/EventSubscriber/EntityRouteAlterSubscriberTest.php
@@ -0,0 +1,140 @@
+      'Multiple entity parameters on an entity form' => [
+        [
+          '_entity_form' => 'entity_test.edit'
+        ],
+        [
+          'entity_test' => [
+            'type' => 'entity:entity_test',
+          ],
+          'node' => [
+            'type' => 'entity:node',
+          ],
+        ],
+        [
+          'entity_test' => [
+            'type' => 'entity:entity_test',
+            'load_latest_revision' => TRUE,
+          ],
+          'node' => [
+            'type' => 'entity:node',
+            'load_latest_revision' => TRUE,
+          ],
+        ],
+      ],

I'm happy with this patch now. This is the last bit that is contentious that would be good to get some feedback on. If the route is an entity_test.edit form, do we load the latest revision of 'node'? It's an edge case, not 100% sure.

Sam152’s picture

Component: entity system » routing system

Maybe routing is the more-correct component for this issue?

Sam152’s picture

Issue summary: View changes
Sam152’s picture

Issue summary: View changes
timmillwood’s picture

One thought, this ends up loading the entity twice. The initial load gets passed to the EntityConverter where it's switched for the latest revision. Why don't we just load the latest revision initially rather than loading twice?

Also, two other queries:

  1. +++ b/core/lib/Drupal/Core/EventSubscriber/EntityRouteAlterSubscriber.php
    @@ -50,10 +50,32 @@ public function onRoutingRouteAlterSetType(RouteBuildEvent $event) {
    +    $events[RoutingEvents::ALTER][] = ['onRoutingRouteAlterSetLatestRevision', -149];
    

    Does this need to be a lower weight?

  2. +++ b/core/lib/Drupal/Core/ParamConverter/EntityConverter.php
    @@ -62,6 +62,25 @@ public function convert($value, $definition, $name, array $defaults) {
    +        $entity_revisions = $storage
    +          ->getQuery()
    +          ->allRevisions()
    +          ->condition($entity->getEntityType()->getKey('id'), $entity->id())
    +          ->sort($entity->getEntityType()->getKey('revision'), 'DESC')
    +          ->range(0, 1)
    +          ->execute();
    

    Does this query need accessCheck(FALSE)?

Sam152’s picture

Thanks for the review!

0. Good catch, replacing with a version which only loads the entity once.
1. The -150 event sets all the entity type information in the params. -149 runs after and uses that information, so I think the order is correct.
2. Good point! I don't think converters are concerned with access.

timmillwood’s picture

Status: Needs review » Reviewed & tested by the community

As long as testbot agrees, looks good to me.

dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/EventSubscriber/EntityRouteAlterSubscriber.php
    @@ -50,10 +50,32 @@ public function onRoutingRouteAlterSetType(RouteBuildEvent $event) {
    +        if (!isset($parameter['load_latest_revision'])) {
    

    Should we add this flag just for revisionable entity types?

  2. +++ b/core/lib/Drupal/Core/ParamConverter/EntityConverter.php
    @@ -60,8 +60,34 @@ public function __construct(EntityManagerInterface $entity_manager) {
    +        // @todo, replace this query with a standardised way of getting the
    

    IMHO We use american english aka. the version with z.

timmillwood’s picture

Status: Reviewed & tested by the community » Needs review

Back to "Needs review" based on #27.

Sam152’s picture

  1. +++ b/core/lib/Drupal/Core/EventSubscriber/EntityRouteAlterSubscriber.php
    @@ -57,12 +66,17 @@ public function onRoutingRouteAlterSetType(RouteBuildEvent $event) {
    +        if ($parameter['type'] === 'entity:' . $entity_type && !isset($parameter['load_latest_revision'])) {
    
    +++ b/core/tests/Drupal/Tests/Core/EventSubscriber/EntityRouteAlterSubscriberTest.php
    @@ -95,7 +107,6 @@ public function latestRevisionAlterTestCases() {
               'node' => [
                 'type' => 'entity:node',
    -            'load_latest_revision' => TRUE,
               ],
    

    So now that we're looking at the entity type anyway, I think it makes sense to only set the flag for params of the type of entity in question.

  2. +++ b/core/lib/Drupal/Core/ParamConverter/EntityConverter.php
    @@ -68,7 +68,7 @@ public function convert($value, $definition, $name, array $defaults) {
    -        // @todo, replace this query with a standardised way of getting the
    +        // @todo, replace this query with a standardized way of getting the
    

    Fixed :)

  3. +++ b/core/tests/Drupal/Tests/Core/EventSubscriber/EntityRouteAlterSubscriberTest.php
    @@ -110,6 +121,18 @@ public function latestRevisionAlterTestCases() {
    +      'Non-revisionable entity type will not change' => [
    +        [
    +          '_entity_form' => 'entity_test.edit'
    +        ],
    +        [
    +          'entity_test' => [
    +            'type' => 'entity:entity_test',
    +          ],
    +        ],
    +        FALSE,
    +        FALSE,
    +      ],
    

    Test for a non-revisionable entity type.

timmillwood’s picture

Status: Needs review » Reviewed & tested by the community

Looks good, back to RTBC.

Sam152’s picture

Thanks for the reviews!

larowlan’s picture

  1. +++ b/core/lib/Drupal/Core/ParamConverter/EntityConverter.php
    @@ -60,8 +60,34 @@ public function __construct(EntityManagerInterface $entity_manager) {
    +      // "load_latest_revision" flag load the latest revision.
    

    nit: could use a comma after 'flag' - can be fixed on commit

  2. +++ b/core/tests/Drupal/KernelTests/Core/ParamConverter/EntityConverterLatestRevisionTest.php
    @@ -0,0 +1,125 @@
    +class EntityConverterLatestRevisionTest extends KernelTestBase {
    

    Do we need a test for a non-revisionable entity? Happy for the answer to be 'no we already have tests for that in {some class}'

Sam152’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
838 bytes
14.08 KB

1. Fixed (https://twitter.com/xjmdrupal/status/922090356077932544).
2. EntityConverterTest seems to have this covered pretty well. I wanted to Kernel test this and use real entities, hence the two test classes, but I suppose a Unit test could have been made to work as well. The original EntityConverterTest doesn't seem to cover the \Drupal\Core\Entity\EntityRepositoryInterface::getTranslationFromContext code path (which the kernel test does), so not sure if that's missing coverage, possibly out of scope for this issue?

Edit: We'd have lots of implicit coverage for it however.

timmillwood’s picture

Status: Needs review » Reviewed & tested by the community

I agree with #33.2.

All looking good, so back to RTBC.

larowlan’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs change notice

Fair nuff.

Can we get a change record pls.

Thanks

Sam152’s picture

Status: Needs review » Reviewed & tested by the community
Sam152’s picture

Issue tags: -Needs change notice
plach’s picture

Status: Reviewed & tested by the community » Needs work

I was manually testing this because I need it in #2860097: Ensure that content translations can be moderated independently and I was not able to get node edit forms to load the latest revision. The following diff seemed to fix the issue:

diff --git a/core/lib/Drupal/Core/EventSubscriber/EntityRouteAlterSubscriber.php b/core/lib/Drupal/Core/EventSubscriber/EntityRouteAlterSubscriber.php
index 721eb7c..ae4a6e2 100644
--- a/core/lib/Drupal/Core/EventSubscriber/EntityRouteAlterSubscriber.php
+++ b/core/lib/Drupal/Core/EventSubscriber/EntityRouteAlterSubscriber.php
@@ -72,7 +72,7 @@ public function onRoutingRouteAlterSetLatestRevision(RouteBuildEvent $event) {
       // Only set the flag on entity types which are revisionable.
       list($entity_type) = explode('.', $entity_form);
       if (!$this->entityTypeManager->getDefinition($entity_type)->isRevisionable()) {
-        return;
+        continue;
       }
       $parameters = $route->getOption('parameters') ?: [];
       foreach ($parameters as &$parameter) {
@@ -89,7 +89,7 @@ public function onRoutingRouteAlterSetLatestRevision(RouteBuildEvent $event) {
    */
   public static function getSubscribedEvents() {
     $events[RoutingEvents::ALTER][] = ['onRoutingRouteAlterSetType', -150];
-    $events[RoutingEvents::ALTER][] = ['onRoutingRouteAlterSetLatestRevision', -149];
+    $events[RoutingEvents::ALTER][] = ['onRoutingRouteAlterSetLatestRevision', -151];
     return $events;
   }
 
tim.plunkett’s picture

Component: routing system » entity system

One key piece of feedback, and some nits.

  1. +++ b/core/lib/Drupal/Core/EventSubscriber/EntityRouteAlterSubscriber.php
    @@ -50,10 +59,37 @@ public function onRoutingRouteAlterSetType(RouteBuildEvent $event) {
    +      list($entity_type) = explode('.', $entity_form);
    

    If this code continues to exist, please add 2 as a 3rd param to explode

  2. +++ b/core/lib/Drupal/Core/EventSubscriber/EntityRouteAlterSubscriber.php
    @@ -50,10 +59,37 @@ public function onRoutingRouteAlterSetType(RouteBuildEvent $event) {
    +      $parameters = $route->getOption('parameters') ?: [];
    +      foreach ($parameters as &$parameter) {
    +        if ($parameter['type'] === 'entity:' . $entity_type && !isset($parameter['load_latest_revision'])) {
    ...
         $events[RoutingEvents::ALTER][] = ['onRoutingRouteAlterSetType', -150];
    

    Now that I read this more...

    It reminded me more and more of \Drupal\Core\Entity\EntityResolverManager::setParametersFromEntityInformation()

    Only to see that it is this subscriber that triggers that code.

    Which begs the question, why punt this to another subscriber? Revisionability is a core enough concept that I don't know that it needs to be kept separate.

  3. +++ b/core/lib/Drupal/Core/ParamConverter/EntityConverter.php
    @@ -60,8 +60,34 @@ public function __construct(EntityManagerInterface $entity_manager) {
    +    if ($storage && $definition) {
    

    $storage cannot ever be falsy, it will throw an exception or return a valid storage class.
    I realize that this is kept from the original patch, but since you're changing the lines anyway might as well not put it in the conditional.

    Also, in what case would !$definition be true?

  4. +++ b/core/lib/Drupal/Core/ParamConverter/EntityConverter.php
    @@ -60,8 +60,34 @@ public function __construct(EntityManagerInterface $entity_manager) {
    +        // @todo, replace this query with a standardized way of getting the
    +        // latest revision in https://www.drupal.org/node/2784201.
    

    Nit: following lines of @todo should be indented an additional 2 spaces.

  5. +++ b/core/lib/Drupal/Core/ParamConverter/EntityConverter.php
    @@ -60,8 +60,34 @@ public function __construct(EntityManagerInterface $entity_manager) {
    +        $entity_revisions = $storage
    +          ->getQuery()
    +          ->allRevisions()
    +          ->condition($entity_definition->getKey('id'), $value)
    +          ->sort($entity_definition->getKey('revision'), 'DESC')
    +          ->range(0, 1)
    +          ->accessCheck(FALSE)
    +          ->execute();
    +        if (!empty($entity_revisions)) {
    +          $revision_ids = array_keys($entity_revisions);
    +          $latest_revision_id = array_shift($revision_ids);
    

    I think this would be best in a protected method that returns the latest revision ID. Then it would be much clearer the contrast between $entity = $storage->loadRevision($value) and $entity = $storage->load($value)

    Or it could return the entity itself.

  6. +++ b/core/lib/Drupal/Core/ParamConverter/EntityConverter.php
    @@ -60,8 +60,34 @@ public function __construct(EntityManagerInterface $entity_manager) {
    +          ->accessCheck(FALSE)
    

    IMO this sort of line should always have a comment asserting that yes, we do want to turn off access checking. Could just be me.

  7. +++ b/core/tests/Drupal/KernelTests/Core/ParamConverter/EntityConverterLatestRevisionTest.php
    @@ -0,0 +1,125 @@
    +   * Test with a translated pending revision.
    

    Nit: this and others in this class should be "Tests" not "Test"

Sam152’s picture

Status: Needs work » Needs review
FileSize
5.4 KB
5.4 KB

Thanks everyone for reviewing! I'm going to fix the bug and add a test for the feedback in #38 first, then see what #39.2 looks like in a subsequent patch, in case there is a good reason to back-out of that approach.

Sam152’s picture

Status: Needs review » Needs work
Sam152’s picture

Correct patch for #40.

Sam152’s picture

Status: Needs work » Needs review
FileSize
22.66 KB
13.98 KB

Thanks for the review @tim.plunkett, implementing #39.

1. Fixed, look like this is in line with the rest of EntityResolverManager, so good catch.
2. Good point. The control flow of setParametersFromEntityInformation was such that it looked a bit messy to put this stuff inside that particular function, but another method running after it I think suited the purpose quite nicely. Updated moved the coverage into EntityResolverManagerTest. I think as you point out, setting this flag is very much in the spirit of the existing class, so happy with this.
3. Sounds like this is being needlessly defensive. Removed both checks, lets see what the bot says.
4. Done.
5. Done, looks much better.
6. Added.
7. Done.

The interdiff is noisy enough that I think reviewing the patch as a whole is probably easier, but one provided for posterity.

Sam152’s picture

@plach also pointed out on IRC that it would be good to make sure we aren't introducing any performance regressions. It's possible the linked @todo as well as a static cache for revisions would mitigate any such regressions, but I'm hoping they are minimal enough that the architectural win is worth the penalty.

#2620980: ContentEntityStorageBase::loadRevision() should use the static and the persistent entity cache like ContentEntityStorageBase::load()
#2784201: Track the latest_revision and latest_revision_translation_affected ID in the entity data table for revisionable entity types

Status: Needs review » Needs work

The last submitted patch, 43: 2865616-43.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Sam152’s picture

Status: Needs work » Needs review
FileSize
747 bytes
14.02 KB

Test fixes.

Sam152’s picture

FileSize
14.02 KB

Test fixes.

Edit: Double post, one day I'll learn to use drupal.org...

plach’s picture

Changes look good to me and work well here, but I'll let Tim put this back to RTBC, if he's happy too :)

plach’s picture

FileSize
3.71 KB

Ok, I performed some quick benchmarking with a DB with roughly 13500 generated nodes (1 revision per node). According to my findings in that case this introduces a ~5% performance regression with cold page/render caches. Details attached.

plach’s picture

FileSize
3.26 KB

With warm render caches (and page cache disabled) the difference is of course higher: around ~12% in the same situation. With page cache enabled there's no significant difference obviously.

plach’s picture

Btw, I agree that probably the tickets mentioned in #44 would alleviate or even cancel the performance penalty introduced here.

dawehner’s picture

I'm profiling a similar scenario at the moment to figure out why this causes such a high regression.

dawehner’s picture

I can reproduce similar bad numbers: https://blackfire.io/profiles/compare/2950eebf-e4fe-400e-9806-370537ce06...

This is all coming from checking edit access on the link relationships.

amateescu’s picture

So why do we even try to do something with link relationships in an entity form? That doesn't make any sense to me..

dawehner’s picture

I agree, I would be personally fine with skipping the edit and delete form from within \Drupal\node\Controller\NodeViewController::view. I'm not sure why this would be helpful, ever.

plach’s picture

Status: Needs review » Reviewed & tested by the community

Let's move this back to RTBC, we need to hear from committers about the performance issue anyway.

The last submitted patch, 42: 2865616-40.patch, failed testing. View results

catch’s picture

We should open a follow-up for #53-55, and either fix that before this goes in or decide to eat the temporary regression here, better to avoid the checks altogether than rely on revision caching - still same hit with cold revision cache.

amateescu’s picture

  1. +++ b/core/lib/Drupal/Core/ParamConverter/EntityConverter.php
    @@ -60,14 +62,59 @@ public function __construct(EntityManagerInterface $entity_manager) {
    +      ->allRevisions()
    ...
    +      ->sort($entity_definition->getKey('revision'), 'DESC')
    +      ->range(0, 1)
    

    We need to use the new latestRevision() method added to entity query here :)

  2. +++ b/core/lib/Drupal/Core/ParamConverter/EntityConverter.php
    @@ -60,14 +62,59 @@ public function __construct(EntityManagerInterface $entity_manager) {
    +      $revision_ids = array_keys($entity_revisions);
    +      $latest_revision_id = array_shift($revision_ids);
    +      return $storage->loadRevision($latest_revision_id);
    

    After we fix the point above, this will simply become return $storage->load(key($entity_revisions));

Also, I tried setting a breakpoint in \Drupal\node\Controller\NodeViewController::view() and then loaded an entity form and the breakpoint was never reached, which sounds like the correct behavior, because I don't see any way of reaching that code through an entity form. Which makes me wonder.. how did the profiling from #53 arrived to a code path which was checking access on the link relationships?

xjm’s picture

Status: Reviewed & tested by the community » Needs review

For #58 and ##59.

plach’s picture

I just created #2922570: The node view page triggers a lot of expensive access checks for relationship links to address #58.

@amateescu:

Also, I tried setting a breakpoint in \Drupal\node\Controller\NodeViewController::view() and then loaded an entity form and the breakpoint was never reached, which sounds like the correct behavior, because I don't see any way of reaching that code through an entity form. Which makes me wonder.. how did the profiling from #53 arrived to a code path which was checking access on the link relationships?

In #49 - #50 I had benchmarked the node view route, because I was seeing the latest revision being loaded in my testing. I don't have access to the profile above, but I suspect @dawehner did the same. No entity form involved here, just edit access checks.

plach’s picture

And this should address #59.

amateescu’s picture

In #49 - #50 I had benchmarked the node view route, because I was seeing the latest revision being loaded in my testing. I don't have access to the profile above, but I suspect @dawehner did the same. No entity form involved here, just edit access checks.

Then this is the problem that we need to address/answer: why is the latest revision loaded on a regular node view route?

The patch should only do that for entity forms by default.

dawehner’s picture

Then this is the problem that we need to address/answer: why is the latest revision loaded on a regular node view route?

The patch should only do that for entity forms by default.

Wait, I thought we know the answer for that already. Due to #2922570: The node view page triggers a lot of expensive access checks for relationship links we generate also a link to the edit form, which with this patch triggers a loading to the latest revision, so access checking can be applied.

plach’s picture

@amateescu:

Any other concern or can we move this back to RTBC?

amateescu’s picture

No other concerns from me, but I think #2922570: The node view page triggers a lot of expensive access checks for relationship links needs to get in first, no?

dawehner’s picture

Won't we need to fix the performance regression first?

plach’s picture

#58 does not specify which should be the preferred way, so I'd like to send this back to committers for a final word :)

amateescu’s picture

@plach, wouldn't that be an infinite loop now that you are a committer as well? :D

plach’s picture

I'm provisional and I couldn't make decisions on issues I've actively worked on anyway :)

plach’s picture

@Sam152:

I just realized that, unless I'm missing something, we no longer need EntityRevisionConverter (see #2496337-164: [plach] Testing issue). I guess it could be removed altogether unless we have BC concerns, in which case it could be kept as an empty extension of EntityConverter.

Thoughts?

plach’s picture

@amateescu @dawehner @catch:

Btw, I did some additional research in #2865616: Add an option for EntityConverter to load the latest entity revision and fix all entity forms to use this option., I'd appreciate your feedback over there :)

dawehner’s picture

@plach
You just caused an actual infinite redirect loop :P

plach’s picture

Sam152’s picture

Re: #71: we tested that in #16, so we're on the same wavelength :). Just a matter of scope, given there are BC questions I was going to push that to another issue, but happy to bring it in here too.

plach’s picture

Ok, a follow-up works for me, but can we make sure we create it before getting back to RTBC?

Since CM is beta I'd go with a (deprecated) empty subclass.

Sam152’s picture

As per https://www.drupal.org/core/d8-bc-policy:

Paramconverters, access checkers, event subscribers and similar services which are never expected to be used directly either as services or value objects, are not considered part of the API.

Of course we can argue such things in the follow up :)

#2924812: Deprecate EntityRevisionConverter in content_moderation.

plach’s picture

Status: Needs review » Reviewed & tested by the community
plach’s picture

Assigned: Unassigned » catch
catch’s picture

+++ b/core/lib/Drupal/Core/ParamConverter/EntityConverter.php
@@ -60,15 +62,55 @@ public function __construct(EntityManagerInterface $entity_manager) {
+
+  /**
+   * Load the latest revision of an entity.
+   *
+   * @param \Drupal\Core\Entity\EntityStorageInterface $storage
+   *   The entity storage.
+   * @param \Drupal\Core\Entity\EntityTypeInterface $entity_definition
+   *   The entity definition.
+   * @param mixed $value
+   *   The raw value.
+   *
+   * @return \Drupal\Core\Entity\EntityInterface|null
+   *   The loaded entity if it exists, NULL otherwise.
+   */
+  protected function loadLatestRevision(EntityStorageInterface $storage, EntityTypeInterface $entity_definition, $value) {
+    // @todo, replace this query with a standardized way of getting the
...
+    $result = $storage
+      ->getQuery()
+      ->latestRevision()
+      ->condition($entity_definition->getKey('id'), $value)
+      // The entity converter is not concerned with access checking, skip the
+      // access check when looking up the latest revision.
+      ->accessCheck(FALSE)
+      ->execute();
+
+    return !empty($result) ? $storage->loadRevision(key($result)) : NULL;
   }

Caught up on this and #2922570: The node view page triggers a lot of expensive access checks for relationship links.

Statically/persistently caching the revision load would help a bit, but I think we're missing an opportunity to avoid loading the revision (as a revision) altogether.

i.e. if the latest revision is also the default revision, we can just use the regular entity load. This will then re-use both the persistent and static cache keyed on entity ID as normal.

With the code as it is currently, we'll end up writing/fetching two cache entries for what is exactly the same entity revision.

Should be able to load the entity via a regular load, query for the latest revision ID as this does. Then loadRevision() if it differs, but just return the entity if they're the same.

edit: this still leaves the entity query on node view pages, it's a shame we have to do that just for an access check on an edit link, but don't see a way around that.

plach’s picture

Priority: Normal » Major
Status: Reviewed & tested by the community » Needs work

Should be able to load the entity via a regular load, query for the latest revision ID as this does. Then loadRevision() if it differs, but just return the entity if they're the same.

Good point, let's do that!

edit: this still leaves the entity query on node view pages, it's a shame we have to do that just for an access check on an edit link, but don't see a way around that.

No, not for now. #2784201: Track the latest_revision and latest_revision_translation_affected ID in the entity data table for revisionable entity types should let us rip out the query though. Would you prefer if we added a @todo mentioning that issue?

plach’s picture

Assigned: catch » Unassigned
catch’s picture

@todo sounds good.

Sam152’s picture

Status: Needs work » Needs review
FileSize
2.94 KB
14 KB

Here is an implementation of the approach in #80.

We also already have a @todo pointing to that issue, to replace the method that does the query, so should be all good there.

plach’s picture

Status: Needs review » Reviewed & tested by the community

Looks good!

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed d0518c2 and pushed to 8.5.x. Thanks!

  • catch committed d0518c2 on 8.5.x
    Issue #2865616 by Sam152, plach, dawehner, timmillwood, amateescu, tim....

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

Wim Leers’s picture

@hchonov never reviewed this issue, and in an issue where we're making the logic here compatible with entity translations, he discovered the negative consequences of this patch. See #2860097-123: Ensure that content translations can be moderated independently for a summary.

Wim Leers’s picture

Wim Leers’s picture

It's possible that this bit of this issue's CR:

This option has been applied automatically to all revisionable entity forms, so data in pending revisions is retained when entities are edited and saved. Modules using pending revisions now don't have to provide additional converters to ensure no data is lost.

is going to be reverted.

effulgentsia’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityResolverManager.php
@@ -197,6 +197,30 @@ protected function setParametersFromEntityInformation(Route $route) {
+        $parameter['load_latest_revision'] = TRUE;

Per #91, does anyone here object to #2940899: Regression: _entity_form routes should not get load_latest_revision_flag by default: changes default behavior (= regression) while only Content Moderation needs it being committed, which changes that line to only being applied to entity types that are moderated by content_moderation module? Leaving it to modules that manage pending revisions their own way to need to explicitly set that flag themselves if that's the behavior they want.