Views style plugins with the `attachTo` method should implement an interface

What about having a common interface for displays and styles?

1. +++ b/core/modules/views/src/Plugin/views/style/AttachableStyleInterface.php
   @@ -0,0 +1,26 @@
   +  /**
   +   * This style is being attached to the given display.
   +   *
   +   * @param array $build
   +   *   The build render array.
   

   It would be nice to explain what attaching mean, so that the method is meant to change the $build to add some additional render element.

2. +++ b/core/modules/views/src/Plugin/views/style/AttachableStyleInterface.php
   @@ -0,0 +1,26 @@
   +   * @param $display_id
   +   *   The display ID that this style is being attached to.
   ...
   +   * @param $title
   +   *   This display's title.
   

   Those two should be strings

1. +++ b/core/modules/views/src/Plugin/views/style/AttachableStyleInterface.php
   @@ -0,0 +1,43 @@
   +   * This style is being attached to the given display.
   

   Reads a bit wrong. Not sure what to suggest instead though.

   'Allows the style to react when attached by a display'?

2. +++ b/core/modules/views/src/Plugin/views/style/AttachableStyleInterface.php
   @@ -0,0 +1,43 @@
   +   *   `feed_icons` property of the view.
   

   Use single quotes, not backticks.

3. +++ b/core/modules/views/src/Plugin/views/style/AttachableStyleInterface.php
   @@ -0,0 +1,43 @@
   +   * @code
   +   *   if ($display->hasPath()) {
   +   *     if (empty($this->preview)) {
   +   *       $build['#attached']['feed'][] = array($url, $title);
   +   *     }
   +   *   }
   +   *   else {
   +   *     $this->view->feedIcons[] = array(
   +   *       '#theme' => 'feed_icon',
   +   *       '#url' => $url,
   +   *       '#title' => $title,
   +   *     );
   +   *   }
   +   * @endcode
   

   This sample code needs some sort of explanation of what it's showing.

Doing this makes sense to me, so +1. Also, patch still applies cleanly on 8.8.x so yay.

Took a look at the patch and it looks good, just a few minor things/nitpicks/questions:

1. +++ b/core/modules/views/src/Plugin/views/style/Opml.php
   @@ -3,6 +3,7 @@
   +use Drupal\views\Plugin\views\style\AttachableStyleInterface;
   
   +++ b/core/modules/views/src/Plugin/views/style/Rss.php
   @@ -4,6 +4,7 @@
   +use Drupal\views\Plugin\views\style\AttachableStyleInterface;
   

   Since they are in the same namespace, are the use statements necessary?

2. +++ b/core/modules/views/src/Plugin/views/style/AttachableStyleInterface.php
   @@ -0,0 +1,46 @@
   +   * @param array $build
   +   *   The build render array, passed by reference. Implementations can
   +   *   directly attach render elements here, or alternatively, manipulate the
   +   *   'feed_icons' property of the view.
   

   Should we be mentioning the feed icons on the interface?

3. +++ b/core/modules/views/src/Plugin/views/style/AttachableStyleInterface.php
   @@ -0,0 +1,46 @@
   +   *   if ($display->hasPath()) {
   +   *     if (empty($this->preview)) {
   +   *       $build['#attached']['feed'][] = array($url, $title);
   +   *     }
   +   *   }
   +   *   else {
   +   *     $this->view->feedIcons[] = array(
   +   *       '#theme' => 'feed_icon',
   +   *       '#url' => $url,
   +   *       '#title' => $title,
   +   *     );
   +   *   }
   

   Let's use short array syntax in the sample code.

Thanks @jhedstrom changes look good. And actually after thinking a bit more, mentioning the feedIcons does make sense in there so this looks good to me. Got nothing else to complain about so RTBC++.

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Re-roll patch for Drupal version 10.1.x

Fixed CCF for #29.

LGTM.

This change will break quite a few modules. See https://git.drupalcode.org/search?group_id=2&repository_ref=&scope=blobs...

For example: https://git.drupalcode.org/project/xmlfeedviews/-/blob/1.0.x/src/Plugin/...

I think we need to check if the plugin implements the new interface and if not issue a deprecation but still call the method. And then in Drupal 11 do something different. I'm also not sure if silence is the correct option here. At the moment if the style plugin does not implement the attachTo() method you'll get a helpful error saying that it does not. With this patch you won't get any information as a developer that you should think about implementing this method.

Based on our issue category definitions I don't think this should be considered a bug.