Field node_field_revision.title needs to use an entity-aware formatter in Views

In order to support this properly we have to allow the String formatter to support linking to any revision.

That sounds like something other of these issues will need, such as the node revision title?

Well, I think every entity type supporting revisions somehow would need that, so I went with solving the generic problem, not just the specific usecase of a single special case.

• Fixed the schema issue
• Added a test for the revision link.

The remaining test is caused by a too long table name due to the new table name. tricky.

1. +++ b/core/lib/Drupal/Core/Entity/Entity.php
   @@ -163,6 +163,10 @@ public function urlInfo($rel = 'canonical', array $options = []) {
   +    if ($rel === 'revision' && $this instanceof RevisionableInterface && $this->isDefaultRevision()) {
   +      $rel = 'canonical';
   +    }
   

   This is a nice trick! But it should probably have some comment.
   But, how useful is this? If you want to link to a specific vid you cannot, and urlRouteParameters() always calls $this->getRevisionId(), when is that not the default vid?

2. +++ b/core/modules/node/src/Plugin/views/field/Revision.php
   @@ -69,7 +69,7 @@ protected function renderLink($data, ResultRow $values) {
   +      $this->options['alter']['url'] = Url::fromRoute('entity.node.revision', ['node' => $nid, 'node_revision' => $vid]);
   
   +++ b/core/modules/node/src/Plugin/views/field/RevisionLink.php
   @@ -58,7 +58,7 @@ protected function renderLink($data, ResultRow $values) {
   +      $url = Url::fromRoute('entity.node.revision', ['node' => $node->nid, 'node_revision' => $vid]);
   

   This won't take advantage of the new $rel === revision hack though, right?

Thanks a lot for your review!

But, how useful is this? If you want to link to a specific vid you cannot, and urlRouteParameters() always calls $this->getRevisionId(), when is that not the default vid?

Well, I wanted to try to avoid that code adding a link to a revision have to deal with that. As we still have the ugly
access checking, catch agreed on IRC to link to the entity itself, if its the current revision.

See core/modules/node/src/Access/NodeRevisionAccessCheck.php:141 in HEAD.

I'm really not sure what exactly to do, honestly

This won't take advantage of the new $rel === revision hack though, right?

Yeah, but I guess it could when we replace the usages of that handler and all the child classes?

We have to fix the database prefix, as its too long now.

berdir had a great suggestion: Just use a different test, which doesn't add an entity type.

Per #2393339-57: [META] Make sure Views base fields are using Field API for formatting, and do not lose functionality, tagging for critical office hours, but if there's a reason to not have this particular child issue in that list, please untag it.

Couple of questions

Also - should we be removing the old plugin here?

1. +++ b/core/lib/Drupal/Core/Entity/Entity.php
   @@ -163,6 +163,10 @@ public function urlInfo($rel = 'canonical', array $options = []) {
   +    if ($rel === 'revision' && $this instanceof RevisionableInterface && $this->isDefaultRevision()) {
   

   As with #8 - a comment here would be great

2. +++ b/core/modules/node/src/Access/NodeRevisionAccessCheck.php
   @@ -138,7 +138,7 @@ public function checkAccess(NodeInterface $node, AccountInterface $account, $op
   -      if ($node->isDefaultRevision() && ($this->nodeStorage->countDefaultLanguageRevisions($node) == 1 || $op == 'update' || $op == 'delete')) {
   +      if ($node->isDefaultRevision() && ($op == 'update' || $op == 'delete')) {
   

   Can you elaborate on why this change was needed?

3. +++ b/core/modules/node/src/Entity/NodeRouteProvider.php
   @@ -45,6 +45,7 @@ public function getRoutes( EntityTypeInterface $entity_type) {
   +
   

   Unneeded

4. +++ b/core/modules/simpletest/src/Tests/SimpleTestBrowserTest.php
   @@ -130,8 +130,8 @@ public function testTestingThroughUI() {
   -      // A KernalTestBase test.
   -      'tests[Drupal\field\Tests\String\StringFormatterTest]' => TRUE,
   +      // A KernelTestBase test.
   +      'tests[Drupal\system\Tests\DrupalKernel\DrupalKernelTest]' => TRUE,
   

   Can you elaborate why this change is needed?

As with #8 - a comment here would be great

Well, we first should discuss whether this the right way of solving it ... any opinion?

Can you elaborate why this change is needed?

The SimpleTestBrowserTest test runs the StringFormatterTest. By doing that you result in simpletest123456simpletest12345entity_test__field_test as table name.
This patch replaces the entity_test entity type usage with entity_test_rev which then results in a total tablename size of over 64. Given that the SimpleTestBrowserTest just
tests any kernel tests, we can choose a different one.

16.1 - I have no issues with that approach

Its great that you don't have any issues with it! Let's put up a new patch.

Can you elaborate on why this change was needed?

With the change in Entity.phpitself, this change is not needed anymore.

Discussed with @webchick, @effulgentsia, and @alexpott. We agreed that this issue should be critical because creating lists of node revision titles that respect access expectations is a very common usecase.

Slightly tweaked the comment.

Manual testing screenshots:

Let's get this one in

$data['node_field_revision']['title']['field']['id'] = 'node_revision';

In NodeViewData - looks like we're still using Drupal\node\Plugin\views\field\Revision. Have we fixed what the issue summary claims to have fixed?

Re-rolled and fixed #21

I'm also working on this =). There is another issue here, the UI for the StringFormatter can be better. At the moment you have to check both options to make the item link to the revision.

I change it to have radios for no link, link to entity & link to revision.

@k4v
If you need any information / help, feel free to ask me.

I changed the boolean field in the backend to a string to hold the link_option

Now it looks like this. the formatter dropdown does not belong there, it should be some side effect in another issue.

Discussing with @k4v at dev days.

Do we really need to complexify the UI with a new, explicit setting in StringFormatter for "link to the revision" ?
Couldn't we just keep "link / no link", where "link" simply always links to the correct revision (the one of the entity that contains the field) ?

I mean, the patch adds this code :

+++ b/core/lib/Drupal/Core/Entity/Entity.php
@@ -163,6 +163,12 @@ public function urlInfo($rel = 'canonical', array $options = []) {
+    // Links pointing to the current revision point to the actual entity. So
+    // instead of using the 'revision' link, use the 'canonical' link.
+    if ($rel === 'revision' && $this instanceof RevisionableInterface && $this->isDefaultRevision()) {
+      $rel = 'canonical';
+    }

So StringFormatter could just do :

if ($this->getSetting('link_to_entity')) {
  $url = $entity->urlInfo($entity instanceof RevisionableInterface ? 'revision' : 'canonical');
}

and just always link to the right revision ?

@dawehner said he could be okay with dropping the option to link to the current entity and always link to the revision, although this was possible before in the views formatter UI.

let's see...

I looked into the errors with @xano. Turns out there is a schema missing for \Drupal\views\Plugin\views\field\Field which is the default handler views uses now that I removed the line from NodeViewsData.php.

go, testbot!

1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/StringFormatter.php
   @@ -93,11 +94,18 @@ public function settingsForm(array $form, FormStateInterface $form_state) {
   +    // if the entity is a revision, just change the UI, we're using the same option and link
   +    // to the revision in this case.
   +    if ($entity_type->isRevisionable() && $entity_type->hasLinkTemplate('revision')) {
   +      $form['link_to_entity']['#title'] = $this->t('Link to the revision');
   +    }
   

   Not sure we want to do that. This doesn't check if the entity is a revision, but if the entity type is revisionable.
   So do we really want the option to read "Link to the revision" instead of "Link to the Content" whenever the formatter is used for nodes ?

   I'd think "Link to the @entity_label" is good enough (and it silently happens to "do the right thing", i.e link to the revision of its parent entity) ?

2. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/StringFormatter.php
   @@ -109,7 +117,11 @@ public function settingsSummary() {
   +    if ($this instanceof RevisionableInterface) {
   +      $summary = array($this->t('Link to the revision'));
   

   Likewise

@yched: sounds reasonable i change it like that.

Hi @k4v, you don't need to wrap the test class in a test[] because it's already done later in the code :

$edit = array(
        "tests[$test]" => TRUE,
      );

Hope it will be ok now :)

+1 for the interdiff in #56

I would see that patch as RTBC, but I worked quite a lot on it.

Nice ! @k4v++

A couple nitpicks below, other than that this looks RTBC to me :-)

1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/StringFormatter.php
   @@ -93,11 +94,12 @@ public function settingsForm(array $form, FormStateInterface $form_state) {
   -    $form['link_to_entity'] = [
   +
   +    $form['link_to_entity'] = array(
          '#type' => 'checkbox',
          '#title' => $this->t('Link to the @entity_label', ['@entity_label' => $entity_type->getLabel()]),
          '#default_value' => $this->getSetting('link_to_entity'),
   -    ];
   +    );
   

   Needless hunk, we should keep the [ ] style

2. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/StringFormatter.php
   @@ -109,7 +111,7 @@ public function settingsSummary() {
   -      $summary[] = $this->t('Linked to the @entity_label', ['@entity_label' => $entity_type->getLabel()]);
   +      $summary = array($this->t('Linked to the @entity_label', ['@entity_label' => $entity_type->getLabel()]));
   

   Likewise

3. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/StringFormatter.php
   @@ -119,16 +121,17 @@ public function settingsSummary() {
        $url = NULL;
   -    // Add support to link to the entity itself.
   -    if ($this->getSetting('link_to_entity') && ($entity = $items->getEntity()) && $entity->hasLinkTemplate('canonical')) {
   -      $url = $entity->urlInfo();
   +
   +    if ($this->getSetting('link_to_entity')) {
   

   Minor, code grouping : let's keep the $url = NULL; line with the code block that's below, they go hand in hand.

4. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/StringFormatter.php
   @@ -119,16 +121,17 @@ public function settingsSummary() {
   +      $entity = $items->getEntity();
   +
   +      // For the default revision this falls back to 'canonical'
   +      $url = $entity->urlInfo('revision');
   

   Nitpick : we could inline the $entity var directly in the $url = ... line

5. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/StringFormatter.php
   @@ -158,5 +160,4 @@ protected function viewValue(FieldItemInterface $item) {
      }
   -
    }
   

   that empty line needs to stay, according to our coding standards :-)

6. +++ b/core/modules/field/src/Tests/String/StringFormatterTest.php
   @@ -134,13 +138,38 @@ public function testStringFormatter() {
   +    $old_revision_id = $entity->getRevisionId();
   +    $entity->setNewRevision(TRUE);
   +    $value2 = $this->randomMachineName();
   +    $entity->{$this->fieldName}->value = $value2;
   +    $entity->save();
   +    $entity_new_revision = \Drupal::entityManager()->getStorage('entity_test_rev')->loadRevision($old_revision_id);
   +
   +    $this->renderEntityFields($entity, $this->display);
   +    $this->assertLink($value2, 0);
   +    $this->assertLinkByHref($entity->url('revision'));
   +
   +    $this->renderEntityFields($entity_new_revision, $this->display);
   +    $this->assertLink($value, 0);
   +    $this->assertLinkByHref('/entity_test_rev/' . $entity_new_revision->id() . '/revision/' . $entity_new_revision->getRevisionId() . '/view');
   

   Minor : it's nicer to add a small comment that explains what you are checking (helps people having to debug a test fail in a later patch ;-)) See the beginning of the mathod.

nits massaged.

@k4v: thanks ! Let's get this in !

Thanks @k4v! Embedding the screenshot in the summary for review. Do you think you could also add a "before" screenshot of what it looks like in HEAD?

Or never mind, just grabbed a screenshot myself. :)

Nobody will see that the screenshots are out of date :P

I took out the screenshots, as the UI is the same as in head now.

Looks great.

Committed and pushed to 8.0.x. Thanks!

Restoring tag.

Great job @k4v!