When viewing a revision, the Quick Edit, Edit, and Delete contextual link operations are available, but should not be

Great catch!

The proposed behaviour would be consistent with the behaviour in Drupal 7.

This patch addresses the removal of the Edit and Quick Edit links.

I think you want to use isDefaultRevision().

A test patch .

Failed testing because of missing ;

Tested code, looks good. However, no contextual links are shown at all. Is this an acceptable solution?

Goofed up, here is the correct patch.

That will teach me to work on two issues at the same time :)

+++ b/core/modules/node/src/NodeViewBuilder.php
@@ -169,7 +170,8 @@ protected static function buildLinks(NodeInterface $entity, $view_mode) {
+    $revision = entity_load($entity->getEntityTypeId(),$entity->id());
+    if ($entity->id() && ContentEntityBase::isDefaultRevision($revision)==TRUE) {

I think my request to use isDefaultRevision() confused you a bit :)

We already have an $entity object. Since this is NodeViewBuilder (which is responsible for building a render array to view a node), we now that we're dealing with Node entities.

In the code for Node you see this:

class Node extends ContentEntityBase implements NodeInterface

which leads us to NodeInterface, in whose code you see this:

interface NodeInterface extends ContentEntityInterface, EntityChangedInterface, EntityOwnerInterface

And this then leads us to ContentEntityInterface, whose code looks like this:

RevisionableInterface

Conclusion: all Node entities are guaranteed to implement RevisionableInterface.

It is RevisionableInterface that requires isDefaultRevision() to be implemented. Hence that method is available directly on all objects that implement RevisionableInterface, of which the Node class is one, and hence all Node entity objects also have it.

In other words, the $revision = entity_load(…) is unnecessary; $entity already is of a certain revision.

And calling on ContentEntityBase::isDefaultRevision() is definitely wrong: you can only call methods like that if they're static. Static methods are bound to the class, not objects (instances of the class), and hence cannot access $this.
It's much, much simpler, actually :) You can simply call $entity->isDefaultRevision() — that's it! :)

Tnx Wim,

wow, didn't thought this far. I have some good OOP knowledge, but it escaped my mind to go look for this specific method.

Will patch this.

In retrospect, do you have any tips for me for 'remembering' to check all methods an object has access to? I think I was misled because my IDE didn't have isDefaultRevision() as an autocomplete, so I did not remember to check all interfaces. Do you know of a technique or something so you can check this?

Tnx,

Patched previous patch, tnx to Wim Leers #14

In retrospect, do you have any tips for me for 'remembering' to check all methods an object has access to? I think I was misled because my IDE didn't have isDefaultRevision() as an autocomplete, so I did not remember to check all interfaces. Do you know of a technique or something so you can check this?

To ensure that an IDE *can* correctly autocomplete, we add /** @var \namespace\interface $variable_name comments in the code. This is necessary when typehints are set to a generic interface (like EntityInterface), which allows implementations of that interface to be passed in… but an implementation is not limited to a single interface, and therefore doesn't know if e.g. RevisionableInterface is also implemented.

That's the general explanation. Hence the general advice would be: look whether the object you're receiving implements additional interfaces.

However, in this case, such a comment does exist! So it looks like your IDE is kind of broken, because that comment does exist and does have the expected effect for me: $entity->is autocompletes to isDefaultRevision().
Not sure what's wrong in your IDE.

Manually tested the patch. Works correctly :)

Unfortunately, there is one small problem in the patch preventing me from RTBC'ing this. Please fix it and then this is RTBC :)

+++ b/core/modules/node/src/NodeViewBuilder.php
@@ -10,6 +10,7 @@
+use Drupal\Core\Entity\ContentEntityBase;

This use statement is no longer necessary.

Tnx Wim Leers, indeed strange that the IDE (Netbeans in this case) is not able to 'autocomplete' these methods. Not that I'm so dependant on that feature, but it is nice just to be able to look at all the methods on any given object. I've noticed some other oddities with Netbeans lately, so I'm going to take a look under the hood, so to speak.

Your patch is coming up :)

Here you go.

Took a bit longer than expected : had to reroll the patch.

One conflict (duh) : the use line was removed for the patch to be able to be applied.

Hope all is wel from here on :)

Perfect, thanks!

$entity->isDefaultRevision() can return TRUE even though $entity->id() will return FALSE. So it seems wrong to remove the $entity->id() check. I might be missing something...

#21: I thought the same: I thought that ->id() would still be necessary. To prevent contextual links from showing up in previews, right? Turns out contextual links already show up in previews in HEAD. This diff doesn't affect that. To fix that, we need to check for $node->in_preview, but that's out of scope.
That's what we get for ever using if ($entity->id()) {…} in the first place: it makes very little sense…

Here is a patch for the entire proposed solution.
Note that though I have not seen any case where id() returns FALSE I have kept the test.

@sdelbosc: is it possible that you're using an amazingly ancient version of Drupal 8? "Quick Edit" module was still called "Edit" in your patch, which happened more than half a year ago, in April: https://www.drupal.org/node/2242711.

Could you reroll against the latest Drupal 8, or shall I do that for you?

Oh, and wait, you're proposing to add "Revert to revision" and "Delete revision" contextual links! That's fine, but that belongs in a new issue. This means that comments #23—#26 should be ignored.

Reuploading #19 which IMHO is still RTBC worthy. Pinging tstoeckler for feedback.

Yes, I used 8.x branch and realised the mistake when my patch failed testing.

Regarding the Revert to revision and Delete revision links, I think we should update the description of the issue as it is confusing.
Otherwise, from what I have seen the patch from #19 is fine for me in the sense that I have not found any use case where id() is FALSE (on 8.x branch at least).

By the way, if you could confirm that 8.0.x is the branch I should use it would be great.

Yes, I used 8.x branch and realised the mistake when my patch failed testing.

Aha, heh :)

Regarding the Revert to revision and Delete revision links, I think we should update the description of the issue as it is confusing.

Clarified the title.

By the way, if you could confirm that 8.0.x is the branch I should use it would be great.

Yes, 8.0.x is the one you want. And just to make sure: please do open a new issue for what you want to do :)

Do you know the issue number of the other issue you are talking about?
The one supposed to show Revert and Delete contextual links on revisions.

@sdelbosc: there isn't one, please create one :)

Sorry misunderstanding on my side. However, when reading the proposed solution in the description of this ticket, it seems to me that adding these 2 links is part of this ticket and not a new one.

#32: it's not. I already clarified the title in #29.

Can we add some quick test for this?

sorry added wrong tag.

For info. I have created [2389341] according to the previous comments.
@Wim, would be great if you could check that this is what you expected.

It still seems strange to me to remove the id() check. Even if it's incorrect, IMO we should fix that in a separate issue including dedicated tests for that.

In any case marking needs work due to the "Needs tests" tag.

Writing tests for this issue...

Here is what I have so far for the tests. I added the new test code to an existing function in NodeRevisionsTest.php, as that seemed to be the most relevant existing test class. This allowed me to take advantage of some existing test set-up functionality in that class.

In the process of creating these tests, however, I found that I had some concerns with the patch from #27:

1) I agree with @tstoeckler that the if( $entity->id() ) check should be left in. I see no justification for removing it. It seems quite likely that removing it will create problems elsewhere.

2) If I'm understanding things correctly, the patch in #27 removes the quickedit, edit, and delete contextual links when viewing revisions by completely disabling the creation of the contextual links "popup menu". This will create problems if other contextual links need to be displayed when viewing a revision, and thus seems like a poor solution.

Interestingly, the patch from the related issue #2389341 removes the quickedit, edit, and delete contextual links (in addition to adding new links for reverting and deleting revisions). My brain is pretty fried at this point, so it's not clear to me exactly how it accomplishes that, but perhaps that patch can be adapted to be a solution for this issue?

Here is an alternative patch for this issue, derived from the patch by @sdelbosc in related issue #2389341. Instead of completely disabling contextual links for revisions, this patch fixes the issue by giving revisions their own separate contextual links, which are currently empty.

I also updated my test code to account for the return of the contextual links menu for revisions.

Re-rolled

Isn't this supposed to be tested against 8.0.x-dev? Or was it correct to test it against 8.0.0-beta2? Please correct me if I'm wrong.

After further examination, I realized that I needed to include a bit more of @sdelbosc's patch in order to prevent the "Quick Edit" link from appearing under certain circumstances. Sorry for the confusion on this.

I applied when_viewing_a-2362435-48.patch using a clean install of D8 and the patch correctly removed all three contextual links when displaying the older revision. Tested with Yosemite and Chrome.

+++ b/core/modules/node/src/NodeViewBuilder.php
@@ -183,6 +183,9 @@
+          'metadata' => array(
+            'quickEdit' => '0'
+          ),

+++ b/core/modules/quickedit/js/quickedit.js
@@ -514,6 +514,13 @@
+        // Check if quick edit feature is explicitly disabled.
+        var dataContextualId = $links.closest('.contextual-region').find('[data-contextual-id]').attr('data-contextual-id');
+        if (dataContextualId.indexOf('quickEdit=0') > -1) {
+          return false;
+        }

I don't understand why this is necessary. It feels very hacky. Can you please explain it?

In response to #51, this code's purpose is to explicitly disable the addition of a "Quick Edit" contextual link. Without this code, the quick edit link will be incorrectly added to the context menu for node revisions in the case where there are other contextual links (for example, those proposed to be added by #2389341: When viewing a revision the Reverted or Deleted links should be available in contextual links). There might be other ways of doing the same thing that would feel less hack-ish. I'm certainly open to ideas.

I originally adapted this code from @sdelbosc's patch for that issue (as mentioned in #40) because it seemed to be a good solution to this issue, without breaking contextual links entirely for revisions.

Looking at the code again, the JS code could be cleaned up quite a bit, to something more like:

        // Check if quick edit feature is explicitly disabled.
        var dataContextualId = $(contextualLink.el).attr('data-contextual-id');
        if (dataContextualId.indexOf('quickEdit=0') > -1) {
          return true;
        }

Let me help, this is the patch on #48 with #52 fix.

#53 removes the 3 links and test passed.

Screen shot before on revision page:

Screen shot after on revision page:

1. +++ b/core/modules/node/src/NodeViewBuilder.php
   @@ -171,10 +171,23 @@ protected function alterBuild(array &$build, EntityInterface $entity, EntityView
   +          'route_parameters' => array('node' => $entity->id()),
   +          'metadata' => array('changed' => $entity->getChangedTime()),
   ...
   +          'route_parameters' => array(
   +            'node' => $entity->id(),
   +            'node_revision' => $entity->getRevisionId(),
   +          ),
   +          'metadata' => array(
   +            'quickEdit' => '0'
   +          ),
   

   These aren't formatted consistently.

2. +++ b/core/modules/node/src/Tests/NodeRevisionsTest.php
   @@ -19,6 +20,13 @@ class NodeRevisionsTest extends NodeTestBase {
   +  public static $modules = array('node', 'datetime', 'contextual');
   

   Just "contextual" would be sufficient, the parent class' modules are also enabled automatically (IIRC).

3. +++ b/core/modules/node/src/Tests/NodeRevisionsTest.php
   @@ -93,6 +102,18 @@ function testRevisions() {
   +    $this->assertTrue(strstr($json[$ids[0]], '<li class="entitynodeedit-form"><a href="' . base_path() . 'node/' . $node->id() . '/edit">Edit</a></li>'),
   +      'The "Edit" contextual link is shown for the default revision.');
   +    $this->assertTrue(strstr($json[$ids[0]], '<li class="entitynodedelete-form"><a href="' . base_path() . 'node/' . $node->id() . '/delete">Delete</a></li>'),
   +      'The "Delete" contextual link is shown for the default revision.');
   
   @@ -106,6 +127,18 @@ function testRevisions() {
   +    $this->assertFalse(strstr($json[$ids[0]], '<li class="entitynodeedit-form">'),
   +      'The "Edit" contextual link is not shown for a non-default revision.');
   +    $this->assertFalse(strstr($json[$ids[0]], '<li class="entitynodedelete-form">'),
   +      'The "Delete" contextual link is not shown for a non-default revision.');
   

   We put the assertion messages on the same line.

4. +++ b/core/modules/node/src/Tests/NodeRevisionsTest.php
   @@ -203,4 +236,23 @@ function testNodeRevisionWithoutLogMessage() {
   +  }
    }
   

   Nit: newline after the last method.

Also, still not convinced by this approach — sorry.

+++ b/core/modules/quickedit/js/quickedit.js
@@ -514,6 +514,13 @@
+        // Check if quick edit feature is explicitly disabled.
+        var dataContextualId = $(contextualLink.el).attr('data-contextual-id');
+        if (dataContextualId.indexOf('quickEdit=0') > -1) {
+          return false;
+        }

This is too late in the process.

Also, why do we even need quickEdit=0? Zero contextual links are defined on that route! AFAICT this also works, and is much, much simpler.

Lets get the screenshots added to the issue summary above.

I am at DrupalCon LA. This patch no longer applies. It will need to be re-rolled. I will review the patch and see if I can re-roll it.

re-saved comment

I can't see how I would re-roll it, (newbie issues).

If this needs a reroll, it needs to be marked as "needs work".

@Wim @sher1 I have re-rolled #58 . Please check it.
Attaching interdiff also.

Testing the patch alone, to see if it fails as expected.

Re-rolled the patch

Only a review of the code, I haven't looked at the functionality.

1. +++ b/core/modules/node/src/Tests/NodeRevisionsTest.php
   @@ -110,6 +112,18 @@ function testRevisions() {
   +    $ids = array('node:node=' . $node->id() . ':changed=' . $node->getChangedTime());
   +    $response = $this->renderContextualLinks($ids, "node/" . $node->id());
   
   @@ -123,6 +137,18 @@ function testRevisions() {
   +    $ids = array('node_revision::node=' . $node->id() . '&node_revision=' . $node->getRevisionId() . ':');
   +    $response = $this->renderContextualLinks($ids, "node/" . $node->id() . '/revisions/' . $node->getRevisionId() . '/view');
   
   @@ -222,6 +248,25 @@ function testNodeRevisionWithoutLogMessage() {
   +  protected function renderContextualLinks($ids, $current_path) {
   

   renderContextualLinks could just take the node as an argument to reduce code duplication.

2. +++ b/core/modules/node/src/Tests/NodeRevisionsTest.php
   @@ -110,6 +112,18 @@ function testRevisions() {
   +    $json = Json::decode($response);
   
   @@ -123,6 +137,18 @@ function testRevisions() {
   +    $json = Json::decode($response);
   
   @@ -222,6 +248,25 @@ function testNodeRevisionWithoutLogMessage() {
   +    return $this->drupalPost('contextual/render', 'application/json', $post, array('query' => array('destination' => $current_path)));
   

   Instead of returning the raw response, renderContextualLinks could return the json object to reduce code duplication.

3. +++ b/core/modules/node/src/Tests/NodeRevisionsTest.php
   @@ -110,6 +112,18 @@ function testRevisions() {
   +    $this->assertTrue(strstr($json[$ids[0]], '<li class="entitynodeedit-form"><a href="' . base_path() . 'node/' . $node->id() . '/edit">Edit</a></li>'),
   +      'The "Edit" contextual link is shown for the default revision.');
   +    $this->assertTrue(strstr($json[$ids[0]], '<li class="entitynodedelete-form"><a href="' . base_path() . 'node/' . $node->id() . '/delete">Delete</a></li>'),
   +      'The "Delete" contextual link is shown for the default revision.');
   
   @@ -123,6 +137,18 @@ function testRevisions() {
   +    $this->assertFalse(strstr($json[$ids[0]], '<li class="entitynodeedit-form">'),
   +      'The "Edit" contextual link is not shown for a non-default revision.');
   +    $this->assertFalse(strstr($json[$ids[0]], '<li class="entitynodedelete-form">'),
   +      'The "Delete" contextual link is not shown for a non-default revision.');
   

   These should be on one line, not two.

Assigning to myself, working for this on DrupalCon Barcelona.

+++ b/core/modules/node/src/NodeViewBuilder.php
@@ -153,10 +153,20 @@ protected function alterBuild(array &$build, EntityInterface $entity, EntityView
+      if ($entity->isDefaultRevision()) {
+        $build['#contextual_links']['node'] = array(
+          'route_parameters' => array('node' => $entity->id()),
+          'metadata' => array('changed' => $entity->getChangedTime()),
+        );
+      }
+      else {
+        $build['#contextual_links']['node_revision'] = array(
+          'route_parameters' => array(
+            'node' => $entity->id(),
+            'node_revision' => $entity->getRevisionId(),
+          ),
+        );
+      }
     }

Added a PR for entity module to generalize that bit: https://github.com/fago/entity/pull/23

<3

We ran into a closely related issue with Workbench Moderation a while back. Our solution was here: #2635890: Quick Edit integration. Basically, we disable quick edit entirely when showing any revision other than the latest revision, as doing otherwise leads to unexpected branching and unmanaged weirdness. Dropping a link here for context.

#69
1.) Yes, but the input varies: "node:node=" vs . "node_revision::node="
2, 3.) done.

remove leftover parenthesis.

@lokapujya, I am able to reproduce the issue. For the revision when click on contextual menu, Drop down is not reflecting.

Hmm, I guess this never worked completely as shown by the missing "Delete" in the screenshots in comment #54.

Can anyone confirm that the delete link should show up. At least, the contextual links probably shouldn't be empty like they do in the #54 screenshot, right?

Rerolled patch from #77.

@lokapujya, the Delete link should not be showing up, as it will delete the entire node rather than the revision that is being viewed. This would most likely be unexpected behavior for users. #2389341: When viewing a revision the Reverted or Deleted links should be available in contextual links is intended to add back a "Delete revision" contextual link.

I've changed the title to match the intent of this issue.

The contextual links are still empty though. What should happen in this case?

• Resolved PHPCS warning in NodeRevisionsTest RE: $modules array declaration: "If the line declaring an array spans longer than 80 characters, each element should be broken into its own line."
• No functional changes.
• Patch worked as expected.

The links are hidden for me as intended, but as #86 noted, it now has the odd empty contextual links behavior.

The empty contextual links is the result of a separate Drupal Core bug: #2650910: Contextual links button is always rendered even when no links are available (with warm client-side cache) Once that is fixed, the empty contextual links visible in #88 will go away.

Discussed with @catch, @xjm, @dixon, @amateescu, @berdir, @effulgentsia, @timmillwood - we decided to promote this to critical as the user is not editing what they think they are editing when quick editing a revision - this could result in data lose and is certainly unexpected and confusing.

1. +++ b/core/modules/node/src/NodeViewBuilder.php
   @@ -146,10 +146,20 @@ protected function alterBuild(array &$build, EntityInterface $entity, EntityView
   +      if ($entity->isDefaultRevision()) {
   +        $build['#contextual_links']['node'] = array(
   +          'route_parameters' => array('node' => $entity->id()),
   +          'metadata' => array('changed' => $entity->getChangedTime()),
   +        );
   +      }
   +      else {
   +        $build['#contextual_links']['node_revision'] = array(
   +          'route_parameters' => array(
   +            'node' => $entity->id(),
   +            'node_revision' => $entity->getRevisionId(),
   +          ),
   +        );
   +      }
   

   The first case has changed in its metadata, the revision one should too, because revisions can also be edited without creating a new revision.

2. +++ b/core/modules/node/src/Tests/NodeRevisionsTest.php
   @@ -165,6 +184,17 @@ function testRevisions() {
   +    $this->assertFalse(strstr($json[$ids[0]], '<li class="entitynodeedit-form">'),
   +      'The "Edit" contextual link is not shown for a non-default revision.');
   +    $this->assertFalse(strstr($json[$ids[0]], '<li class="entitynodedelete-form">'),
   +      'The "Delete" contextual link is not shown for a non-default revision.');
   

   Nit: strange wrapping here.

3. +++ b/core/modules/node/src/Tests/NodeRevisionsTest.php
   @@ -318,6 +348,27 @@ function testNodeRevisionWithoutLogMessage() {
   +   * @param array $ids
   

   Nit: string[]

4. +++ b/core/modules/node/src/Tests/NodeRevisionsTest.php
   @@ -318,6 +348,27 @@ function testNodeRevisionWithoutLogMessage() {
   +   *   An array of contextual link ids.
   

   Nit: s/ids/IDs/

5. +++ b/core/modules/node/src/Tests/NodeRevisionsTest.php
   @@ -318,6 +348,27 @@ function testNodeRevisionWithoutLogMessage() {
   +   *   The decoded Json response body.
   

   Nit: s/Json/JSON/

6. +++ b/core/modules/node/src/Tests/NodeRevisionsTest.php
   @@ -318,6 +348,27 @@ function testNodeRevisionWithoutLogMessage() {
   +    $post = array();
   ...
   +    $response = $this->drupalPost('contextual/render', 'application/json', $post, array('query' => array('destination' => $current_path)));
   

   Nit: s/array()/[]/

Making changes from review in #93...

Suggestions from #93 implemented as well as some additional array syntax changes for new code. Patch and interdiff attached.

I'm afraid #95 introduced a new array syntax case:

+++ b/core/modules/node/src/NodeViewBuilder.php
@@ -147,18 +147,19 @@ protected function alterBuild(array &$build, EntityInterface $entity, EntityView
+          'metadata' => array('changed' => $entity->getChangedTime()),

s/array()/[]/

a little fix.

Rerolled it.

+++ b/core/modules/node/src/NodeViewBuilder.php
@@ -146,10 +146,21 @@ protected function alterBuild(array &$build, EntityInterface $entity, EntityView
+          'metadata' => array('changed' => $entity->getChangedTime()),

Updated array syntax.

This issue is also critical for phase A of the Workflow Initiative.

Updated remaining array syntax. I am having trouble parsing the failing test results.

The most recent test failure was caused by #2749955: Random fails in UpdatePathTestBase tests so not the result of this patch.

There were a few code style issues left, otherwise this patch looks ready to me.

RTBC++

+++ b/core/modules/node/src/Tests/NodeRevisionsTest.php
@@ -34,7 +35,13 @@ class NodeRevisionsTest extends NodeTestBase {
-  public static $modules = array('node', 'datetime', 'language', 'content_translation');
+  public static $modules = [
+    'node',
+    'contextual',
+    'datetime',
+    'language',
+    'content_translation',
+  ];

In general I prefer that we not reformat arrays like this when changing lines for other reasons, because it makes word diffs unusable. Not going to block a critical on that, though. ;)

I tested this patch manually and confirmed there are no more dangerous edit/delete links on past revisions! Yay!

However, it introduces a different bug. With the patch, the contextual links "friendly pencil" is displayed, but expands to an empty dropdown:

That's a better bug to have, but it is still a bug I think.

However, it introduces a different bug. With the patch, the contextual links "friendly pencil" is displayed, but expands to an empty dropdown:

@xjm The issue you are describing is being handled in #2650910: Contextual links button is always rendered even when no links are available (with warm client-side cache).

Back to RTBC per #111.

Attached patch undos reformatting of $modules array as requested by @xjm in #110.

Looks good, thanks @markdorison :)

I guess the question now is should #2650910: Contextual links button is always rendered even when no links are available (with warm client-side cache) block this. I don't think so but it'd be good to get more opinions on this.

I vetted the patch at #2650910: Contextual links button is always rendered even when no links are available (with warm client-side cache), see #2650910-23: Contextual links button is always rendered even when no links are available (with warm client-side cache). In short: it's ready, it just needs JS test coverage. I've also written a comprehensive IS.

tried to understand the patch and updated the change record for that. I hope that makes a bit more sense to more than just me.

Aha! Thanks for the issue link.

I think that that bug is less bad than this bug, so I do not think it should block this issue. That bug is a "huh wtf?" but this one is a "huh wtf and lol I deleted my node". :)

Committed 4668a26 and pushed to 8.2.x and 8.1.x. Thanks!

#120 Nice summary :D