Convert ViewsDisplayExtender plugin discovery to attributes

Ruturaj Chaubey → made their first commit to this issue’s fork.

Converted ViewsDisplayExtender plugin discovery to PHP attributes.
@larowlan is the approach of conversion correct ?

This looks good for the main conversion, but there is the annotation base class to deal with. 📌 Convert ViewsPluginAnnotationBase plugin discovery to attributes Closed: duplicate was closed in favour of doing it here, because the base class can't really be done on its own.

The base class contains a single property:

  /**
   * Whether or not to register a theme function automatically.
   *
   * This property is optional and it does not need to be declared.
   *
   * @var bool
   */
  public $register_theme = TRUE;

Theme functions don't even exist any more, so worth investigating what this even does - if we can remove it entirely then I don't think we need the base class. If we still need it we should consider whether it makes sense to have a base class or implement it a different way (interface etc)

This is still used as a killswitch in views_theme(). Firstly, it only affects certain Views plugin types, which to me means the base class is only needed for these at most (note also the comment not matching the list!):

  // Only display, pager, row, and style plugins can provide theme hooks.
  $plugin_types = [
    'display',
    'pager',
    'row',
    'style',
    'exposed_form',
  ];

Then, it acts as a killswitch:

      if (!isset($def['theme']) || empty($def['register_theme'])) {
        continue;
      }

However appears to be when a subclass implementation wants to share the template of a parent, e.g. the default RSS row plugin defines:

 *   theme = "views_view_row_rss",

and then the separate node and comment RSS row plugins share the template but do not register it separately:

 *   theme = "views_view_row_rss",
 *   register_theme = FALSE,

So it seems there is still some value in this, but it does look like it should only be applied to the five plugin types that are checked for templates in the first place.

Also even the core display plugins are inconsistent. Each display plugin claims to own views_view, but Block and EntityReference decide to abort by using register_theme = FALSE.

core/modules/views/src/Plugin/views/display/Attachment.php: *   theme = "views_view",
core/modules/views/src/Plugin/views/display/Block.php: *   register_theme = FALSE,
core/modules/views/src/Plugin/views/display/Block.php: *   theme = "views_view",
core/modules/views/src/Plugin/views/display/DefaultDisplay.php: *   theme = "views_view",
core/modules/views/src/Plugin/views/display/Embed.php: *   theme = "views_view",
core/modules/views/src/Plugin/views/display/EntityReference.php: *   register_theme = FALSE,
core/modules/views/src/Plugin/views/display/EntityReference.php: *   theme = "views_view",
core/modules/views/src/Plugin/views/display/Page.php: *   theme = "views_view",

Marking NR given the findings above mean to me that ViewsDisplayExtender does not need the base class, and we should move that to one of the other issues where it is actually required (pending further discussion at least)

ViewsPluginManager is shared by all Views plugins, we need to make it able to handle any attribute, probably by passing it in as an argument similar to how the annotations are handled.

Thanks for doing all that sleuthing @longwave

I think it would make sense to do it with exposed form then, as there's only a couple of those so the scope will be smaller.

I'll rewrangle all the issue links etc

I redid all the issue relationships/state so now just the 5 plugin-types that use the flag are postponed. I unpostponed exposed form and made that the one where we add the base class

MR update to address #10 📌 [PP-1] Convert ViewsDisplayExtender plugin discovery to attributes Postponed . It will be good to get the plugin manager changes in, since it's need by all the views plugins conversions. Until all the plugins have attributes, manager constructor code is a little ugly.

Based on the others believe this needs the deriver class.

Added deriver to attribute constructor.

Thanks for the quick reply.

Feedback appears to be addressed then. See all instances have been replaced.

Committed/pushed to 11.x and cherry-picked to 10.3.x, thanks!

I had to check whether this is used anywhere, but eventually found some implementations in contrib, so looks like it is.

Can someone with access please close the MR? Thanks!

catch → closed merge request !6596

Automatically closed - issue fixed for 2 weeks with no activity.