Allow per-field diff configuration

Here there is a very early patch, the list is build and the config can be updated but not saved. Need help to solve the schema for storing the config.

I started from the beginning again. This is a dirty patch but at least it works. In the patch I am storing the plugin config with fields.entityTypeId_FieldName.plugin but I think (not sure though) it should be changed to be EntityTypeId.FieldName.plugin.
This will break tests. :)

Changed some code, it seems now is stored in a better way, still need to fix tests. but the code works, tested manually with some other contrib modules plugins.

Cleaned the code a bit, fixed the labels. Still need to fix most of the test, failing due we don't have defaults yet.

@miro_dietiker

1. Still working on that
2. Changed the icon.
4. Not sure either, I think it's Ok if we hide them based on a common name.
3. & 5. Fixed the labels.
6. Will be a followup

@miro_dietiker

1. Now fixed.

Still need to fix tests, but in general it works as expected.

Made some other changes, fixed code, added a service to exclude some unneeded fields.

First human smell code analysis...

1. ...
   +   * @var \Drupal\Core\Entity\EntityManagerInterface
   ...
   +  protected $entityManager;
   ...
   +  public function __construct(PluginManagerInterface $plugin_manager, PluginManagerInterface $diffBuilderManager, EntityManagerInterface $entityManager) {
   ...
   +    $this->entityManager = $entityManager;
   ...
   +      $container->get('entity.manager')
   

   since this is deprecated, shouldn't you already add the non-deprecated entity then ? use entity_type.manager

2. +++ b/src/Form/FieldsSettingsForm.php
   @@ -45,10 +53,11 @@ class FieldTypesSettingsForm extends ConfigFormBase {
       * @param \Drupal\Component\Plugin\PluginManagerInterface $plugin_manager
       * @param \Drupal\Component\Plugin\PluginManagerInterface $diffBuilderManager
   

   ... also missing its argument doc ...

3. +++ b/src/Form/FieldsSettingsForm.php
   @@ -107,11 +120,26 @@ class FieldTypesSettingsForm extends ConfigFormBase {
   +    foreach (\Drupal::entityTypeManager()->getDefinitions() as $entity_type_name => $entity_type) {
   

   ... and then use it here.

4. +++ b/src/DiffFields.php
   @@ -0,0 +1,28 @@
   +  /**
   +   * @param \Drupal\Core\Field\FieldStorageDefinitionInterface $field_storage_definition
   +   *
   +   * @return bool
   +   */
   +  public function showDiff(FieldStorageDefinitionInterface $field_storage_definition) {
   

   missing documentation.

5. +++ b/src/Form/FieldsSettingsForm.php
   @@ -26,6 +22,18 @@ class FieldTypesSettingsForm extends ConfigFormBase {
   +   * Wrapper object for writing/reading configuration from diff.plugins.yml
   

   needs better documentation.

6. +++ b/src/Form/FieldsSettingsForm.php
   @@ -282,24 +315,25 @@ class FieldTypesSettingsForm extends ConfigFormBase {
   -   * Form submission handler for multi-step buttons.
   +   * @param $form
   +   * @param \Drupal\Core\Form\FormStateInterface $form_state
   

   documentation. and many other places...

7. +++ b/src/Form/FieldsSettingsForm.php
   @@ -239,42 +272,42 @@ class FieldTypesSettingsForm extends ConfigFormBase {
   +              '#type' => 'submit',
   ...
   +              '#type' => 'submit',
   ...
   +            '#type' => 'image_button',
   

   some odd indentations here...

8. +++ b/diff.services.yml
   @@ -21,3 +21,6 @@ services:
   +      class: Drupal\diff\DiffFields
   

   indentation.

9. diff --git a/config/install/diff.plugins.yml b/config/install/diff.plugins.yml
   

   why have you just removed the lines and not just delete this file ?

10. +++ b/src/Form/FieldsSettingsForm.php
    @@ -1,12 +1,8 @@
    -/**
    - * @file
    - * Contains \Drupal\diff\Form\FieldTypesSettingsForm.
    - */
    -
    

    unrelated. Let's create a refactor issue for diff later (could then be another candidate's issue (?)).

11. +++ b/src/Form/FieldsSettingsForm.php
    @@ -347,43 +378,41 @@ class FieldTypesSettingsForm extends ConfigFormBase {
    +          //$state->set('field_type', $field_type);
    

    leftover ?

Didn't check about the logic yet...

Did some other changes, cleaned up the code a bit, test should pass now. :)

Fixed tests, also fixed hook_help.

Nice! I've reviewed the code a bit. Here, some comment improvement suggestions and other nitpicks:

1. diff --git a/config/install/diff.plugins.yml b/config/install/diff.plugins.yml
   

   Again, deleted lines but file still there ? ;)

2. +++ b/diff.module
   @@ -82,7 +82,28 @@ function diff_help($route_name, RouteMatchInterface $route_match) {
   +function _diff_field_label($entity_type, $field_name) {
   

   Needs documentation.

3. +++ b/diff.module
   @@ -82,7 +82,28 @@ function diff_help($route_name, RouteMatchInterface $route_match) {
   +  // Count the amount of instances per label per field.
   

   "// Count the amount of instances per field label." (?)

4. +++ b/diff.module
   @@ -82,7 +82,28 @@ function diff_help($route_name, RouteMatchInterface $route_match) {
   +  foreach (array_keys(\Drupal::service('entity_type.bundle.info')->getBundleInfo($entity_type)) as $bundle) {
   +    $bundle_instances = \Drupal::service('entity_field.manager')->getFieldDefinitions($entity_type, $bundle);
   

   Better to define the services once, before the loop.

5. +++ b/diff.module
   @@ -82,7 +82,28 @@ function diff_help($route_name, RouteMatchInterface $route_match) {
   +  if (empty($labels)) {
   +    return [$field_name];
   

   Better to explain what you return here.

6. +++ b/diff.module
   @@ -82,7 +82,28 @@ function diff_help($route_name, RouteMatchInterface $route_match) {
   +  // Sort the field labels by it most used label and return the labels.
   

   "// Order the field labels by the most used label, and return them."

7. +++ b/src/Controller/NodeRevisionController.php
   @@ -83,7 +83,8 @@ class NodeRevisionController extends EntityComparisonBase {
   +        $visible = entity_get_display('node', $node->getType(), $view_mode)->getComponent($field_machine_name);
   

   Deprecated function used, but this can be fixed in a refactoring issue (possible new MDS student's issue (?))

8. +++ b/src/DiffBuilderManager.php
   @@ -36,4 +37,29 @@ class DiffBuilderManager extends DefaultPluginManager {
   +   * Check if the field is not a key and revisionable to define if it should be
   +   * displayed in the diff comparison.
   

   "To define if a field should be displayed in the diff comparison, check if it is revisionable and its entity key is not a bundle or a revision." (?)
   not sure how to make it clear without going too much in details...

9. +++ b/src/DiffBuilderManager.php
   @@ -36,4 +37,29 @@ class DiffBuilderManager extends DefaultPluginManager {
   +      // Check if the field is bundle or revision, if so don't dislay it.
   

   "// Do not display the field, if it is a bundle or a revision."

10. +++ b/src/DiffBuilderManager.php
    @@ -36,4 +37,29 @@ class DiffBuilderManager extends DefaultPluginManager {
    +      $entity_type = \Drupal::entityTypeManager()->getDefinition($field_storage_definition->getTargetEntityTypeId());
    

    Better to inject entityTypeManager.

11. +++ b/src/DiffEntityParser.php
    @@ -36,20 +36,20 @@ class DiffEntityParser {
    +  protected $entityTypeManager;
    ...
        *   Entity Manager service.
    

    Doc is wrong. Should be "The entity type manager.", "EntityTypeManagerInterface", and "The entity type manager service."

12. +++ b/src/DiffEntityParser.php
    @@ -36,20 +36,20 @@ class DiffEntityParser {
    +  public function __construct(DiffBuilderManager $diffBuilderManager, EntityTypeManagerInterface $entityTypeManager, ConfigFactoryInterface $configFactory) {
    +    $this->entityManager = $entityTypeManager;
    

    Should be $this->entityTypeManager = $entity_type_manager;

    Actually also the other parameters need to be written as snake_case ...

    btw, i don't see the usage of $this->entityTypeManager and $this->config anywhere in this class (?)

13. +++ b/src/DiffEntityParser.php
    @@ -77,14 +77,50 @@ class DiffEntityParser {
    +        // Iterate through all the field types this plugin supports
    +        // and for every such field type add the id of the plugin.
    

    "// Add the field type's ID for each field type this plugin supports."

14. +++ b/src/DiffEntityParser.php
    @@ -77,14 +77,50 @@ class DiffEntityParser {
    +      // Get the plugin configuration set for this field.
    

    "// Get the field plugin configuration."

15. +++ b/src/DiffEntityParser.php
    @@ -77,14 +77,50 @@ class DiffEntityParser {
    +      $plugin_options = [];
    +      if (isset($plugins[$field_items->getFieldDefinition()->getType()])) {
    +        foreach ($plugins[$field_items->getFieldDefinition()->getType()] as $id) {
    +          $plugin_options[$id] = $diff_plugin_definitions[$id]['label'];
    +        }
    +      }
    

    Since $plugin_options is used only in if (!$plugin_config), shouldn't this block be inside that check ?

16. +++ b/src/DiffEntityParser.php
    @@ -77,14 +77,50 @@ class DiffEntityParser {
    +      // If there is no a plugin defined take the first available.
    

    "// If there is no plugin defined, take the first available." (?)

17. +++ b/src/EntityComparisonBase.php
    @@ -118,35 +118,37 @@ class EntityComparisonBase extends ControllerBase {
    +      $result[$right_key] += $this->combineFields(array(), $right_values[$field_name]);
    
    +++ b/src/Form/FieldsSettingsForm.php
    @@ -210,14 +244,14 @@ class FieldTypesSettingsForm extends ConfigFormBase {
    +          'callback' => array($this, 'multiStepAjax'),
    
    @@ -227,54 +261,54 @@ class FieldTypesSettingsForm extends ConfigFormBase {
    +            '#limit_validation_errors' => [['fields', $field_key, 'plugin', 'type']],
    ...
    +            '#attributes' => array('class' => array('field-plugin-settings-edit'), 'alt' => $this->t('Edit')),
    ...
    +            '#limit_validation_errors' => array(array('fields', $field_key, 'plugin', 'type')),
    
    @@ -282,24 +316,25 @@ class FieldTypesSettingsForm extends ConfigFormBase {
    +        if ($plugin_settings = $form_state->getValue(array('fields', $field_key, 'settings_edit_form', 'settings'))) {
    
    @@ -347,43 +379,40 @@ class FieldTypesSettingsForm extends ConfigFormBase {
    +          $plugin = $this->diffBuilderManager->createInstance($field_values['plugin']['type'], array());
    

    array() --> []
    And better to break the array elements in new lines.
    Basically i would change the array syntax if I've edited that line, but yeah, again this could be moved to a refactoring issue.

18. +++ b/src/Form/FieldsSettingsForm.php
    @@ -40,15 +51,19 @@ class FieldTypesSettingsForm extends ConfigFormBase {
    +   * @param \Drupal\Core\Entity\EntityTypeManagerInterface $entityTypeManager
    +   * @param \Drupal\Core\Entity\EntityFieldManagerInterface $entityFieldManager
    ...
    +  public function __construct(PluginManagerInterface $plugin_manager, PluginManagerInterface $diffBuilderManager, EntityTypeManagerInterface $entityTypeManager, EntityFieldManagerInterface $entityFieldManager) {
    ...
    +    $this->entityTypeManager = $entityTypeManager;
    +    $this->entityFieldManager = $entityFieldManager;
    

    Use snake_case for these parameters.

19. +++ b/src/Form/FieldsSettingsForm.php
    @@ -107,11 +126,26 @@ class FieldTypesSettingsForm extends ConfigFormBase {
    +    // Build a row in the table for every field of every entity type.
    

    "// Build a row for each field of each entity type."

20. +++ b/src/Form/FieldsSettingsForm.php
    @@ -107,11 +126,26 @@ class FieldTypesSettingsForm extends ConfigFormBase {
    +        // Build a row in the table for every field type.
    

    "// Build the row for this field." (?)

21. +++ b/src/Form/FieldsSettingsForm.php
    @@ -121,40 +155,21 @@ class FieldTypesSettingsForm extends ConfigFormBase {
    -   * Builds a row for the table. Each row corresponds to a field type.
    ...
    +  protected function buildFieldRow($entity_type, $field_definition, $plugins, $diff_plugin_definitions, FormStateInterface $form_state) {
    

    Why is this method's documentation gone ? You just need to update the first parameter doc.

22. +++ b/src/Form/FieldsSettingsForm.php
    @@ -282,24 +316,25 @@ class FieldTypesSettingsForm extends ConfigFormBase {
    -   * Form submission handler for multi-step buttons.
    
    @@ -313,10 +348,7 @@ class FieldTypesSettingsForm extends ConfigFormBase {
    -   * Ajax handler for multi-step buttons.
    
    @@ -347,43 +379,40 @@ class FieldTypesSettingsForm extends ConfigFormBase {
    -   * {@inheritdoc}
    

    Also here, why are they removed ?

23. +++ b/src/Form/FieldsSettingsForm.php
    @@ -396,34 +425,30 @@ class FieldTypesSettingsForm extends ConfigFormBase {
    +    // Set the plugin type as hidden of the fields which have no plugin
    +    // selected.
    

    "// Set the plugin type as hidden, for fields which have no plugin selected."

24. +++ b/src/Form/FieldsSettingsForm.php
    @@ -396,34 +425,30 @@ class FieldTypesSettingsForm extends ConfigFormBase {
    +    // For fields that have a plugin selected save the settings.
    

    I would say "// Save the settings, for fields which have a plugin selected."

Probably I still miss some placed. Just start with these points and discuss about comments/documentations.

And also provide UI screenshots ;)

Changes made. Here is the new fields configuration tab.

Forgot to change this.

Added update function for deleting old no-longer-used configuration.

Deprecated method mentioned by tduong in #23 .7 will be removed in #2759627: Remove redundant NodeRevisionController
Core issue about opening the settings form when changing the diff formatter: #2766031: Open settings form when changing field formatter
Choose default plugins: #2766035: Define default plugins

This introduced a regression for fields where a plugin cannot be found. The attached patch appears to resolve it.

The specific field that failed was an og membership reference field.

The resulting exception looks like this:

Drupal\Component\Plugin\Exception\PluginNotFoundException: The "" plugin does not exist. in Drupal\Core\Plugin\DefaultPluginManager->doGetDefinition() (line 52 of core/lib/Drupal/Component/Plugin/Discovery/DiscoveryTrait.php).
Drupal\Core\Plugin\DefaultPluginManager->getDefinition(NULL) (Line: 16)
Drupal\Core\Plugin\Factory\ContainerFactory->createInstance(NULL, Array) (Line: 84)
Drupal\Component\Plugin\PluginManagerBase->createInstance(NULL, Array) (Line: 108)
Drupal\diff\DiffEntityParser->parseEntity(Object) (Line: 116)
Drupal\diff\EntityComparisonBase->compareRevisions(Object, Object) (Line: 84)
Drupal\diff\Controller\NodeRevisionController->compareNodeRevisions(Object, '179365', '180181', 'raw')
call_user_func_array(Array, Array) (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 574)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 124)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext(Array, Array) (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}()
call_user_func_array(Object, Array) (Line: 139)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 62)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 57)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 98)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1) (Line: 77)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 50)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 23)
Stack\StackedHttpKernel->handle(Object, 1, 1) (Line: 628)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)

Opened #2768911: Regression: Exception doing a diff on some field types with the regression, marking this fixed again. Thanks for all the work on this @miro_dietiker!