Broken/missing handler (Module: node) …

Add a new node revision view, and you will see it.

CommentFileSizeAuthor
#16 2229167-12.patch8.35 KBdawehner
#14 2229167-14.patch8.29 KBmarthinal
#12 interdiff-2229167-9-12.txt946 bytesmarthinal
#12 2229167-12.patch8.35 KBmarthinal
#9 interdiff.txt2.06 KBdawehner
#9 vdc-2229167-9.patch8.35 KBdawehner
#7 interdiff.txt6.9 KBdawehner
#7 vdc-2229167-7.patch8.31 KBdawehner
#1 2229167.patch3.62 KBBojhan
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Bojhan’s picture

Bojhan CreditAttribution: Bojhan commented
FileSize
2229167.patch3.62 KB

I am not sure if "$display_options['fields']['title']['link_to_node_field_revision'] = 1;" is needed but, the remaining patch seems to resolve the issue. The modal for selecting fields seems broken, but I am not sure if thats related to this.

Bojhan’s picture

Bojhan CreditAttribution: Bojhan commented
Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 1: 2229167.patch, failed testing.

Bojhan’s picture

Bojhan CreditAttribution: Bojhan commented
Status: Needs work » Needs review

1: 2229167.patch queued for re-testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

Nice work! I will review it properly tomorrow and probably also write a test for it.

+++ b/core/modules/node/lib/Drupal/node/Plugin/views/wizard/NodeRevision.php
@@ -127,7 +127,7 @@ protected function defaultDisplayOptions() {
-    $display_options['fields']['title']['link_to_node_revision'] = 1;
+    $display_options['fields']['title']['link_to_node_field_revision'] = 1;

That change is not needed.

Bojhan’s picture

Bojhan CreditAttribution: Bojhan commented

Yhea, kinda figured it wasn't needed. I am not sure how to test this :) I thought we had tests for it already but I guess that only tests whether we actually get results from this.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
FileSize
vdc-2229167-7.patch8.31 KB
interdiff.txt6.9 KB

Here is a test.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented 
  1. +++ b/core/modules/node/lib/Drupal/node/Plugin/views/wizard/NodeRevision.php
@@ -95,20 +95,20 @@ protected function defaultDisplayOptions() {
+    $display_options['fields']['changed']['alter']['alter_text'] = 0;
+    $display_options['fields']['changed']['alter']['make_link'] = 0;
+    $display_options['fields']['changed']['alter']['absolute'] = 0;
+    $display_options['fields']['changed']['alter']['trim'] = 0;
+    $display_options['fields']['changed']['alter']['word_boundary'] = 0;
+    $display_options['fields']['changed']['alter']['ellipsis'] = 0;
+    $display_options['fields']['changed']['alter']['strip_tags'] = 0;
+    $display_options['fields']['changed']['alter']['html'] = 0;
+    $display_options['fields']['changed']['hide_empty'] = 0;
+    $display_options['fields']['changed']['empty_zero'] = 0;

    While we are changing these, shall we make them real booleans? 

  2. +++ b/core/modules/node/lib/Drupal/node/Tests/Views/NodeRevisionWizardTest.php
@@ -0,0 +1,77 @@
+    $node = $node->createDuplicate();
+    $node->isNewRevision();
+    $node->created->value = REQUEST_TIME + 20;
+    $node->save();

    Didn't know you had to do that, I would have just called isNewRevision() and saved again. Does it need to be a new object? Sorry, haven't looked at the entity system changes for quite a while! More of a question than anything.

Also, how was anything working before without the join to node_revision from node_field_revision??

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
FileSize
vdc-2229167-9.patch8.35 KB
interdiff.txt2.06 KB

Also, how was anything working before without the join to node_revision from node_field_revision??

Nothing did, seriously!

Didn't know you had to do that, I would have just called isNewRevision() and saved again. Does it need to be a new object? Sorry, haven't looked at the entity system changes for quite a while! More of a question than anything.

I tried without the duplication and it simply did not worked at all.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented 
+++ b/core/modules/node/lib/Drupal/node/Tests/Views/NodeRevisionWizardTest.php
@@ -0,0 +1,77 @@
+    $node->isNewRevision();
...
+    $node->isNewRevision();

Was this supposed to be setNewRevision()? isNewRevision just returns a boolean, it doesn't *do* anything.

Otherwise I think this is fine.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs review » Needs work

UPs yeah

marthinal’s picture

marthinal CreditAttribution: marthinal commented
Status: Needs work » Needs review
FileSize
2229167-12.patch8.35 KB
interdiff-2229167-9-12.txt946 bytes

Reviewing the patch :)

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented

Should the tests have passed anyway when the nodes were not being set as new revisions as per Tim's comment above?

marthinal’s picture

marthinal CreditAttribution: marthinal commented
FileSize
2229167-14.patch8.29 KB

By default, when we are creating a new node, the new revision id is added to the node_revision table. So, should not fail and afaik we don't need to force a new revision in this test. Removing this method from the test...

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented

Sorry, do you have an interdiff for that? :)

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
FileSize
2229167-12.patch8.35 KB

Sorry but I really think #14 is wrong as it does not contain revisions anymore. We actively want to test that specfic scenario: nodes with multiple revisions.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Needs review » Reviewed & tested by the community

The last hunk is out of scope, but it's not too bad.
Nice test, thanks!

  • Commit c47466e on 8.x by catch: 
    Issue #2229167 by dawehner, marthinal, Bojhan: Broken/missing handler (...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

Status: Fixed » Closed (fixed)

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