Two relations from content revisions to content look identical

Here is one suggestion for the wording:

I feel pretty strongly that this is better than nothing, but there is room for improvement.

I want to keep the text short. I think any site builder who gets this far (adding a relationship to a view of content revisions) should already be pretty familiar with Drupal, so I think it is right to favor accuracy at the expense of some simplicity. But I did not go all the way and refer to "content ID" and "revision ID".

The patch affects code in the node module, so maybe I am wrong to assign this issue to the views.module component. On the other hand, I see the option "node system" but not "node.module". Maybe I should just classify it under "user interface text" (done).

I am adding #2844820: Improve Views revision integration for all revisionable entities as a related issue, in case this functionality ever gets extended to all revisionable entities.

I am adding #1832858: Revamp descriptions of items in handler listings as a related issue. Maybe we should make that the parent of the current issue.

Thanks for the review!

I will add a little more information to the issue description.

As I pointed out on #2769289: Can't show 'revision author' on views of type 'Content', one way to get a view of current revisions is to join on the vid and make it an inner join by checking "Require this relationship".

@xjm points out that this needs tests. I am also going to look (again) for a duplicate issue.

I am happy to get help with wordsmithing the descriptions, but I do not think that every single string needs attention from the UX team, so I am removing the "Needs usability review" tag that I added when I updated the IS.

Test added, so I am removing that tag.

This is my first time writing a JS test from scratch, so I am not sure what the testbot will say.

As I said, I need practice writing these tests.

Try again.

Making progress.

and again

Try a different selector to open the "Advanced" section of the Views edit page.

I recently switched to Lando/Docker for my local sites. I was not able to run the test locally until I figured out how to get pahntomjs installed on the container.

In a javascript test, I expected the machine name to be filled in automatically when I enter the human-readable name. This did not happen, so I had to do this:

    $this->drupalGet('admin/structure/views/add');
    $name = $this->randomMachineName();
    $machine_name = strtolower($name);
    $page->fillField('label', $name);
    $page->fillField('id', $machine_name);

Exactly: failure to wait seems to be the cause of intermittent failures. For example, I worked on #2934997: Intermittent failure in MediaUiJavascriptTest, where that was the problem.

@dawehner:

The point of the patch is to make useful interface text show up, so I think we really need a functional test for this. Besides, that is the sort of test that @xjm requested (off line).

Thanks for suggesting a non-JS test. I found NodeRevisionWizardTest.php, which already sets up a test view of node revisions. There are a few options:

1. Add three lines to the existing test method, even though they are not related to the name of the class nor method.
2. Add another test method in the class (and refactor the existing test into a setUp() and a test).
3. Add an entirely new test class.

The attached patch implements the middle option. (My local git repo has a commit implementing the first option, so I can easily produce a patch for that.) I wonder whether I should rename the test class, since it now tests more than the wizard.

While refactoring the tests, I made a few changes that might be considered out of scope:

• The setUp() method calls parent::setUp(FALSE); so that the test views from ViewsTestBase are not added.
• I rewrote the array definition as a single, multi-line assignment instead of $view = []; followed by separate assignments for each key.

I have not attached an interdiff. The testing code is completely different, and there are only two lines (no change) outside the tests.

Kernel tests are harder to write, but I guess I need the experience. Maybe I should have said "integration test" instead of "functional test" in my previous comment.

Thanks to @tim.plunkett and @mglaman on Slack for helping me convert this to a Kernel test.

This time I am adding a test to core/modules/node/tests/src/Kernel/Views/RevisionRelationshipsTest.php. The existing test in that class does not use Views UI, but I think the name of the class is general enough that the new test "fits".

Again, I do not think that an interdiff would be helpful.

Here is a slight simplification of the test, thanks to @dawehner's comment on #2947814: Use KernelTestBase::render() in CacheTest and IconBuilderTest. This time, an interdiff is appropriate.

I am living dangerously: I have not run the test locally.

@ifrik: Thanks for offering to help with the wording!

Short version: by "current revision" I mean the value of vid from the node table. By "this revision" I mean the value of vid from the node_revision table. The term "current revision" means the same thing it does on the Revisions page, /node/{nid}/revisions.

Suppose the node with node ID nid = 1 has several revisions, with revision IDs vid = 1, 2, 3.

Typically, the most recent revision ID is the current one. In this case, the node table will have one row for this node, with nid = 1 and vid = 3. The node_revision table will have three rows for this node:

nid | vid
  1 | 1
  1 | 2
  1 | 3

When we create a view of "Content revisions", we get all three rows from the node_revision table. When we add a relation to the node table, we can either match on the nid column or on the vid column.

Matching on nid is what I label "Content associated to this revision". Using the word "Content" instead of "node" is my weak attempt at being user-friendly. When we choose this option, we get all the information from the node table added to each row of our view.

Matching on vid is what I label "Associated content for the current revision, NULL otherwise". When we choose this option, we get all the information from the node table added to the third row of the view. The other rows get a bunch of NULL values by default (using an outer join). If we check "Require this relationship" when adding the relation (meaning we tell Views to use an inner join) then those rows get removed.

With content moderation, the value of vid in the node table is the currently published revision, which may not be the most recent one.

We talked about this at the weekly UX meeting today. A few suggestions came up:

1. Change the Title column as well as the Description column.
2. Perhaps change the Title column to "Content (nid)" and "Content (vid)", similar to the "Image (field_image:target_id)" line in the screenshots.
3. Use "results" rather than "rows" to describe what-it-is-that-views-produces.
4. Use "empty" rather than "NULL".
5. Give a longer description of at least one of the options. Something like, "Add this relationship and make it required to get just one result for each node." Of course, we try to avoid "node" in the admin UI, so not exactly that.

I promised to generate some screenshots showing the results of each option, and we may bring it up again at the next UX meeting.

@jrockowitz: Style point

+          $option['title'] .= ' ('. $field_name .')';

I prefer " ($field_name)". The coding standards suggest this usage, but do not require it.

I ran across this old issue while looking for something else.

I am closing this issue as outdated. In #2652652: Provide a relationship from the revision table to the main table, along with replacing the node-specific relationship with a more generic one, the labels were updated. Here is a screenshot:

The description does not explain exactly what the difference is, but it is clear that these are two different options. With a little experimentation, anyone building a view should be able to figure out which one to use.