Add a views form integration for diff

+++ b/src/Plugin/views/field/DiffFrom.php
@@ -0,0 +1,97 @@
+class DiffFrom extends DiffPluginBase {

Form*

OHHH AHH AHAHAA

@dawehner Thanks for your work ! It's really nice that we can use view for this one.
Still, it's not very clear for me yet how are we going to proceed with integration between diff and moderation_state module. Which module is going to provide the view ? Are both modules going to provide their own separate view and when used together the diff view is disabled and the moderation_state one is configured to include the diff form ?

As for waiting on the generic diff controller I think we can work on this first since there are tasks postponed on it and anyway the order doesn't really matter for diff anyway.

Looks ready to me.

1. +++ b/src/Controller/GenericRevisionController.php
   @@ -0,0 +1,331 @@
   +class GenericRevisionController extends EntityComparisonBase {
   ...
   +  protected function getVids(EntityStorageInterface $storage, $entity_id) {
   

   Doc block missing.

2. +++ b/src/Controller/GenericRevisionController.php
   @@ -0,0 +1,331 @@
   +  protected function buildMarkdownNavigation(EntityInterface $entity, $left_vid, $right_vid, $active_filter) {
   

   @param part of the doc block is missing.

3. +++ b/tests/src/Kernel/DiffControllerTest.php
   @@ -0,0 +1,89 @@
   +  public function testController() {
   

   Doc block missing.

Re-rolled.

This is an improved version of patch #11 (https://www.drupal.org/files/issues/2636406-11.patch) where the path to test files is changed (/modules -> /tests/modules).
I chose #11 as according to @dawehner this is the actual patch for this issue.
As I understand it #13 and #22 belong to https://www.drupal.org/node/2634212.
Please correct me, if I'm wrong!

@dawehner
Yes, it works and helps us very much. Thank you!

Re-testing #27.

I open my view within a modal dialog. By clicking on 'Compare' I want it to stay within the modal, but by default a new page is loaded.

Try 1: Enable ajax for view (within Views API). -> This doesn't seem to work for the Compare button (equal to bulk actions).

Try 2: Use a form_alter hook to add an ajax callback. -> This only works, if I have no exposed filter added to the view. But I need to filter by author. In this case (with exposed filter and added ajax callback for compare button) first the filter form is loaded which then expects an ajax callback too. As I won't define an ajax callback for the filter, nothing happens and i get the following Ajax Error:

ResponseText: {"message":"A fatal error occurred: The specified #ajax callback is empty or not callable."}

Does anybody know this problem?

Flagged #641480: Expose diff functionality to views as a duplicate, since there are no patches in that older thread.

Hey, how can I do it on Drupal 7.

I have created a set of pages and I'll update them from time to time. On the home page I want to show the updated things (only updated content) and not the complete page. People shall see what new content is available as form of posts like we do from 'promote to front page'. I have created a view for content revision but can't integrate the diff module with it. Also there is no other way round to solve my problem.

Thanks
Shubham
(Technical Writer)

Rerollèd.

1. +++ b/diff.views.inc
   @@ -0,0 +1,36 @@
   +  $entity_types = \Drupal::entityTypeManager()->getDefinitions();
   +  $revisionable_entity_types = array_filter($entity_types, function (EntityTypeInterface $entity_type) {
   +    return $entity_type->isRevisionable();
   +  });
   

   This can be condensed a bit: array_filter(get_entity_types(), function () { ... })

2. +++ b/src/Plugin/views/field/DiffFrom.php
   @@ -0,0 +1,98 @@
   +  protected function getToFieldId() {
   

   This is missing a doc comment.

3. +++ b/src/Plugin/views/field/DiffFrom.php
   @@ -0,0 +1,98 @@
   +    $to_fields = array_filter($this->view->field, function (FieldPluginBase $field) {
   +      return $field instanceof DiffTo;
   +    });
   +    return array_keys($to_fields)[0];
   

   As a micro-optimization, we could just foreach through the fields and return ASAP.

4. +++ b/src/Plugin/views/field/DiffFrom.php
   @@ -0,0 +1,98 @@
   +      $diff_from = $form_state->getValue($this->options['id']);
   +      $diff_from_entity = $this->loadEntityFromDiffFormKey($diff_from);
   +
   +      $diff_to = $form_state->getValue($this->getToFieldId());
   +      $diff_to_entity = $this->loadEntityFromDiffFormKey($diff_to);
   

   If the entities can't be loaded for any reason, things are going to blow up. Can we add a couple of sanity checks here?

5. +++ b/src/Plugin/views/field/DiffFrom.php
   @@ -0,0 +1,98 @@
   +      if ($diff_from_entity instanceof NodeInterface) {
   +        $form_state->setRedirect('diff.revisions_diff', [$entity_type_id => $diff_from_entity->id(),'left_vid' => $diff_from_entity->getRevisionId(), 'right_vid' => $diff_to_entity->getRevisionId()], $options);
   +      }
   +      else {
   +        $route_name = 'entity.' . $entity_type_id . '.revisions_diff';
   +        $form_state->setRedirect($route_name, [$entity_type_id => $diff_from_entity->id(), 'left_revision' => $diff_from_entity->getRevisionId(), 'right_revision' => $diff_to_entity->getRevisionId()], $options);
   +      }
   

   Can there be a comment explaining why nodes need different handling from other entity types?

6. +++ b/src/Plugin/views/field/DiffPluginBase.php
   @@ -0,0 +1,145 @@
   +  public function getCacheMaxAge() {
   

   No doc comment?

7. +++ b/src/Plugin/views/field/DiffPluginBase.php
   @@ -0,0 +1,145 @@
   +  public function getValue(ResultRow $row, $field = NULL) {
   +    return '<!--form-item-' . $this->options['id'] . '--' . $row->index . '-->';
   +  }
   

   Will getCacheMaxAge() prevent this placeholder from being render-cached? I ask so that we don't run into an issue like the one over in #2758767: [Needs tests] EB views that sort in descending order are broken by render caching.

8. +++ b/src/Plugin/views/field/DiffPluginBase.php
   @@ -0,0 +1,145 @@
   +    $key = json_encode($key_parts);
   

   Should we use Drupal's JSON serializer here? I think it wraps around json_encode(), but using Drupal's components is probably best practice.

9. +++ b/src/Plugin/views/field/DiffPluginBase.php
   @@ -0,0 +1,145 @@
   +    $key_parts = json_decode($key);
   

   Ditto about using Drupal's serializer.

10. +++ b/src/Plugin/views/field/DiffTo.php
    @@ -0,0 +1,48 @@
    +  public function viewsForm(&$form, FormStateInterface $form_state) {
    +    $use_revision = array_key_exists('revision', $this->view->getQuery()->getEntityTableInfo());
    +
    +    if (!empty($this->view->result)) {
    +      $form[$this->options['id']]['#tree'] = TRUE;
    +      foreach ($this->view->result as $row_index => $row) {
    +        $entity = $row->_entity;
    +        $form[$this->options['id']][$row_index] = [
    +          '#type' => 'radio',
    +          '#parents' => [$this->options['id']],
    +          '#title' => $this->t('Compare this item'),
    +          '#title_display' => 'invisible',
    +          '#return_value' => $this->calculateEntityDiffFormKey($entity, $use_revision),
    +        ];
    +      }
    +    }
    

    I haven't done a line-by-line review, but this look awfully similar to what's in the DiffFrom plugin. Can they both inherit this code from their base class? Or better yet, do they even need to be two different plugins? Can't we simply use the same plugin for both the diff__from and diff__to fields?

11. +++ b/src/Tests/DiffViewsTest.php
    @@ -0,0 +1,65 @@
    +  public function testDiffView() {
    

    Doc comment.

The failing test should still fail, but this addresses some of my comments from #42.

This oughta fix the failures!

1. PHPStorm didn't seem to be able to detect it on its own, but it certainly is documented. Will remove.
2. What do you mean by form validation? I agree it's a bit kludgey, but the point is to avoid fatal errors. Happy to take any suggestions on how to improve that bail-out code.
3. If you don't care, then I guess I don't need to change this :)

How's this?

Looks pretty fine.
Nit: some comments lack a final ".".

Also..

1. +++ b/src/Plugin/views/field/DiffPluginBase.php
   @@ -0,0 +1,178 @@
   +    return '<!--form-item-' . $this->options['id'] . '--' . $row->index . '-->';
   

   Is this comment stuff needed? Seems like no real help to me.

2. +++ b/src/Plugin/views/field/DiffPluginBase.php
   @@ -0,0 +1,178 @@
   +          '#title' => $this->t('Compare this item'),
   

   I thought about accessibility here and if this is enough to cover ARIA requirements...
   First thought if we should offer different title for "from" and "to"...

   Then compared with the default node diff tab to see the radio has no title / alt at all... and that the operations is abused for from and neither to nor the row action button has a table heading. Needs a major revisit there...
   I guess we should create followups to investigate the accessibility situation.

Committed, works nicely.
Also, i added some help text. The emptyness was confusing in the add dialog.
Pity / sorry, not yet in diff alpha3, but we should go beta soon anyway after working through the cleanup issues.

Interesting things happen when you end up creating a view that is not limited to a node.
Comparing two revisions of different nodes results in a 403 link that tries to compare node a in context of node a with a revision id of node b.
No idea if we could somehow avoid this misconfiguration. ;-)

The 7.x branch is not our business. Feel free to mark for backport. :-)

Would a maintainer please reopen this issue and move it to the 7.x queue for backport?