Convert default theme to a LayoutPlugin

Not sure how much sense it makes to commit this separately. Without the plugins, without actually doing anything.

So the granularity of small issues still need the full scope of
- offering some functionality
- including test coverage

Whileas you should trimm down the meta issue for the layout plugins to the absolute minimum for a first commit and add more functionality in follow-ups.
So the first sub-issue will still be quite some monster...

Some first review here...

1. +++ b/src/Controller/PluginRevisionController.php
   @@ -0,0 +1,162 @@
   +   * @param string $filter
   +   *   If $filter == 'raw' raw text is compared (including html tags)
   +   *   If filter == 'raw-plain' markdown function is applied to the text before comparison.
   ...
   +      $plugin = $this->diffLayoutManager->createInstance('classic_diff_layout');
   

   The $filter is what should specify the plugin to load.

2. +++ b/src/DiffLayoutBase.php
   @@ -0,0 +1,118 @@
   +  public function buildConfigurationForm(array $form, FormStateInterface $form_state) {
   

   Yeah, for now simply an enable checkbox. And yeah, some label for the overview.

3. +++ b/src/DiffLayoutInterface.php
   @@ -0,0 +1,45 @@
   +   * @param \Drupal\Core\Routing\RouteMatchInterface $route_match
   +   *   The route match.
   ...
   +  public function build(array $build, EntityInterface $left_revision, EntityInterface $right_revision, RouteMatchInterface $route_match);
   

   I'm not 100% sure we should pass in $route_match that deep.

4. +++ b/src/Plugin/Layout/ClassicDiffLayout.php
   @@ -0,0 +1,393 @@
   +  public function build(array $build, EntityInterface $left_revision, EntityInterface $right_revision, RouteMatchInterface $route_match) {
   +    $entity_type_id = $left_revision->getEntityTypeId();
   +    $entity = $route_match->getParameter($entity_type_id);
   

   I think it should be $entity instead of $route_match in the plugin.

5. +++ b/src/Plugin/Layout/ClassicDiffLayout.php
   @@ -0,0 +1,393 @@
   +    $build['#attached']['library'][] = 'diff/diff.general';
   

   I think this should be added by the caller already to have the basic CSS for the header also ready independent of plugin.

6. +++ b/src/Plugin/Layout/ClassicDiffLayout.php
   @@ -0,0 +1,393 @@
   +  protected function buildMarkdownNavigation(EntityInterface $entity, $left_vid, $right_vid, $active_filter) {
   ...
   +  protected function diffRoute(EntityInterface $entity, $left_vid, $right_vid, $filter = NULL) {
   

   This (navigation and route handling) would not go into the layout plugin.

7. +++ b/src/Plugin/Layout/ClassicDiffLayout.php
   @@ -0,0 +1,393 @@
   +  protected function getVids(EntityStorageInterface $storage, $entity_id) {
   

   Also i would keep this at the Generic/PluginRevisionController

Markdown is a second item in the dropdown. Thus i would make it a separate plugin.

This looks really awesome!

Yeah sure we need a weight to order appearance in the drop down button.

I guess the highest prio will then simply be the default.. or should we stay with current definition?

1. +++ b/src/Controller/PluginRevisionController.php
   @@ -0,0 +1,252 @@
   +    $this->config = $this->config('diff.settings');
   

   Is this common in controllers? Can't remember this is widely done.

   Hmm, i think the idea was to have a config (file) per plugin. So modules could easily provide new diffs incl default config.
   But no more sure. ;-)

I like how you convert the markdown special cases!

Finally, i can't see the settings used.

The setting need not only weight, but also "enabled" or "status" key tho disable / enable a plugin.

EDIT: Lots of statements and questions removed as i found out they have been wrong or i found the answer. ;-)

Also promoting this. Hope we can get this in ASAP and then do remaining things in follow-ups if needed.

Providing a reroll.

So yeah, i guess the most important thing to decide now is if we stay with the proposed config storage, or if we do one config per plugin.

I'm not sure about config entities or not. introducing a config entity is going to make this patch quite a bit bigger, although you could postpoine the UI to a later phase, and just require the config entity to be created by the module. core does that in a similar way with action plugins.. and it is rather confusing there, since plugins without config entity are not picked up.

So the question, as discussed with Miro already, is there a possible use case where the same plugin would be used multiple times, with different config? If so, use config entities, if not, then don't. it would also make updating a lot more complicated, only alpha, but you'd probably have to add a update function, as it would be completely broken without the config entities.

one more thing is the config schema, apparently some markdown setting is in the base schema, that's wrong, should be in a markdown specific one.

1. +++ b/src/Controller/PluginRevisionController.php
   @@ -0,0 +1,252 @@
   +  public function getVids(EntityStorageInterface $storage, $entity_id) {
   +    $result = $storage->getQuery()
   +      ->allRevisions()
   +      ->condition($storage->getEntityType()->getKey('id'), $entity_id)
   +      ->execute();
   

   if we move everything around, should we clean this up a bit and use proper naming like getRevisionIds() or so?

2. +++ b/src/Controller/PluginRevisionController.php
   @@ -0,0 +1,252 @@
   +      $plugin = $this->diffLayoutManager->createInstance($filter);
   

   should we check if this filter exists before using it, to avoid exceptions?

3. +++ b/src/Controller/PluginRevisionController.php
   @@ -0,0 +1,252 @@
   +    $build['#attached']['library'][] = 'diff/diff.general';
   

   does this maybe belong in the plugin? other plugins might want different libraries?

4. +++ b/src/Controller/PluginRevisionController.php
   @@ -0,0 +1,252 @@
   +  /**
   +   * Builds a table row with navigation between raw and raw-plain formats.
   +   */
   +  protected function buildMarkdownNavigation(EntityInterface $entity, $left_vid, $right_vid, $active_filter) {
   ...
   +
   +  protected function buildRevisionsNavigation(EntityInterface $entity, $vids, $left_vid, $right_vid) {
   

   missing/incomplete docs and more $vid stuff (vid is node specific)

   are we sure that this is not something that a plugin might want to do differently?

   if not, then why do we need to pass the $build array to the plugins? can't we just merge things together?

5. +++ b/src/DiffLayoutBase.php
   @@ -0,0 +1,124 @@
   +  public function __construct(array $configuration, $plugin_id, $plugin_definition, ConfigFactoryInterface $config, EntityTypeManagerInterface $entity_type_manager, DiffEntityParser $entityParser) {
   +    $this->configFactory = $config;
   +    $this->entityTypeManager = $entity_type_manager;
   +    $this->entityParser = $entityParser;
   +    parent::__construct($configuration, $plugin_id, $plugin_definition);
   +    $this->configuration += $this->defaultConfiguration();
   

   order seems strange, with parent construct in the middle, that is usually first.

6. +++ b/src/DiffLayoutBase.php
   @@ -0,0 +1,124 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function buildConfigurationForm(array $form, FormStateInterface $form_state) {
   +    // @todo Implement buildConfigurationForm() method.
   +  }
   +
   

   no need for a todo, either keep it empty, if you consider that having configuration is a special case and most need it, or remove some/all methods to force plugins to implement it.

7. +++ b/src/DiffLayoutBase.php
   @@ -0,0 +1,124 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function getConfiguration() {
   +    // @todo Implement getConfiguration() method.
   +  }
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function setConfiguration(array $configuration) {
   +    // @todo Implement setConfiguration() method.
   +  }
   

   those should be implemented, just set/return property?

8. +++ b/src/DiffLayoutManager.php
   @@ -0,0 +1,61 @@
   +    parent::__construct('Plugin/Layout', $namespaces, $module_handler, '\Drupal\diff\DiffLayoutInterface', 'Drupal\diff\Annotation\DiffLayoutBuilder');
   +
   +    $this->entityTypeManager = $entity_type_manager;
   +    $this->config = $configFactory->get('diff.settings');
   +    $this->layoutPluginsConfig =  $configFactory->get('diff.layout_plugins');
   +  }
   

   why no cache and alter hook like 99% of the other plugin managers?

   ah, I see, because the existing one also doesn't have caching/alter. Bad example, that should be fixed. look at core examples.

   Plugin/Layout is also a really bad namespace. That's guaranteed to conflict on core or some other contrib module.

   Usually, namespaces are two folders, Diff is already a really bad example again. Standard is module_name/something, so diff/layout and diff/builder or something like that would be much better.

9. +++ b/src/Plugin/Layout/ClassicDiffLayout.php
   @@ -0,0 +1,200 @@
   +    $build['diff'] = [
   +      '#type' => 'table',
   +      '#header' => $diff_header,
   

   if plugins don't have to set anything except the diff key, then I would let them return just this part, this can also have attached. and no $build argument then as mentioned above.

10. +++ b/src/Plugin/Layout/MarkdownDiffLayout.php
    @@ -0,0 +1,244 @@
    +  /**
    +   * Build the revision link for a revision.
    +   *
    +   * @param \Drupal\Core\Entity\EntityInterface $revision
    +   *   A revision where to add a link.
    +   *
    +   * @return \Drupal\Core\GeneratedLink
    +   *   Header link for a revision in the table.
    +   */
    +  protected function buildRevisionLink(EntityInterface $revision) {
    +    $entity_type_id = $revision->getEntityTypeId();
    +    if ($revision instanceof EntityRevisionLogInterface || $revision instanceof NodeInterface) {
    

    is this really plugin specific? shouldn't it go the baes class at least?

One more thing. It looks like the new plugin controller class replaces the previous generic one, but the old one is not removed, which means it is all new code and the new one is also only used for nodes I think as that extends from that now.

Thx for review, Berdir!

I was about to commit this as a first step, but i think many of the review items should be implemented here.
Let's still keep the issue minimal and push larger things into followups - create them as issues. I'm pretty sure we will progress faster.

Almost ready for commit... Still some last minute feedback.

1. +++ b/diff.routing.yml
   @@ -3,7 +3,7 @@ diff.revisions_diff:
   +    filter: 'classic_layout'
   

   'classic' OK, but why repeat "layout" if all are?
   Same for markdown_layout.

2. +++ b/src/DiffLayoutManager.php
   @@ -0,0 +1,60 @@
   +    parent::__construct('Plugin/Layout', $namespaces, $module_handler, '\Drupal\diff\DiffLayoutInterface', 'Drupal\diff\Annotation\DiffLayoutBuilder');
   

   The problem here is we already have a plugin namespace "Plugin/Diff" that is for Diff field plugins.
   Don't forget, this is a global discovery. There is a layout module and this namespace reads like we are globally introducing a layout system.
   No it's about diff, thus we should name plugins better:
   - Plugin/diff/Layout
   - Plugin/diff/Field

   The problem with the second rename is we break existing plugins such as ERR... But we didn't commit to a stable API yet and i don't know about others that picked this up. Still we need the related issues ready to commit.

   If we want to rename the Plugin/Diff namespace, we should do it as a separate issue.
   #2791943: Rename Plugin/Diff to Plugin/diff/Field

Yeah that's awesome.

Tested a bit manually and i'm satisfied.

Committing to allow us move forward in smaller steps with follow-ups.
Still back to needs work until all follow-ups are created.