Posted by xjm

Problem/Motivation

This issue is a followup for #1535868: Convert all blocks into plugins.

  • Plugins define a plugin ID that is different from their class name.
  • Some, but not all block plugins append the module name to the beginning of the block plugin ID.
  • The menu module prefixes its block plugin derivative IDs with menu- to avoid possible collisions with other blocks (e.g. the main menu vs. the main content block). This may or may not be necessary.

Proposed resolution

  • Explore how the plugin system responds to a possible plugin ID collision (e.g., two separate modules providing a plugin with the ID recent_comments).
  • If necessary, update core plugin IDs so that they are prefixed with the module name and recommend this as a best practice.

Comments

xjm’s picture

Component: node.module » block.module
Status: Postponed » Active
xjm’s picture

Issue tags: +Block plugins
xjm’s picture

salvis’s picture

I'm very much in favor of doing this. The change record already shows it that way:

/**
 * ...
 * @Plugin(
 *   id = "user_online_block",
 *   subject = @Translation("Who's online"),
 *   module = "user"
 * )
 */

If I saw a proposed machine name like "online_block", I'd naturally feel the need to prepend the module name, which would break any custom theming tied to id="block-online-block".

A standard of prepending the module name (and appending 'block') in the plugin definition would make it much less likely that the user changes the machine name.

This is not just a question of avoiding "block plugin ID collisions." Setting a standard will help to avoid a much larger confusion.

tim.plunkett’s picture

Title: Do we need to worry about block plugin ID collisions? » Do we need to worry about plugin ID collisions?
Component: block.module » plugin system
Issue tags: +VDC

This is not just about blocks, it's all plugins. Views plugins have the same problem.

We should consider (maybe not extensively but at least a bit) using a separator for this, user.online_block or user__online_block something, so that we can remove the module from the definition.

salvis’s picture

Yes, a separator would probably make sense, especially with module names that have underscores.

EDIT:
Right now core appends "block" for the id and prepends "block" for the class — "block_test" becomes

<div id="block-test-block" class="block block-block-test>

Maybe core should just make sure that the default machine name starts with the module name (and a suitable separator) and ends with "block" and then leave it at that?

This is what core really does:

<div id="block-machinename" class="block block-module">
<div id="block-test-block" class="block block-block-test">

The machine name is derived from the block title, not the plugin ID, so this is not really relevant here.

tim.plunkett’s picture

Plugin IDs are per plugin type, so they're ALL blocks. That part I'd love to see die.

pcambra’s picture

xjm’s picture

Clarifying #7, since it had me squinting: A Views user_online handler will not collide with a Block user_online definition plugin, so the _block is superfluous. This is out if the scope of the issue, but maybe:

  1. There's no risk of plugin collisions between different plugin types
  2. There is potentially the risk of plugin collisions within a plugin type (what happens?) and so we could namespace them by module.
pcambra’s picture

There is potentially the risk of plugin collisions within a plugin type (what happens?) and so we could namespace them by module.

That makes a lot of sense at least for contrib, i.e. devel has a user_switch block that could be potentially mismatched with a plugin provided by user module.

damiankloip’s picture

I was currently working under the assumption that the implementing module could just have 'my_plugin' but then any other modules implementing a plugin for that type would do 'MODULE_my_plugin', Atleast that is what we have been doing so far. Which I am OK with, So are we saying we should always prefix this id? even for a modules own plugins?

sun’s picture

effulgentsia’s picture

tim.plunkett’s picture

Facts and evidence persuaded me :)

tim.plunkett’s picture

Issue summary: View changes

Updated issue summary.

jibran’s picture

Are we going to have patch in this issue?

jibran’s picture

Issue summary: View changes

Removing myself from the author field so that I can unfollow the issue. --xjm

xjm’s picture

Issue summary: View changes
Issue tags: +Naming things is hard

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

tim.plunkett’s picture

Issue tags: -Blocks-Layouts

Not actively part of the Blocks-Layouts work.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

EclipseGc’s picture

Long standing issue hehe:

So all 4 of the discovery mechanism in core iterate over the results that were given to them and manually set something like:


$definitions[$plugin_id] = $metadata;

So in that sense, multiple plugins of the same id should not cause Drupal to blow up or anything, but one will silently overwrite the other. I could see introducing an exception of some sort in these cases but each discovery mechanic does this same work independently, so there's no single target for preventing that. Drupal developers are pretty good at namespacing things at this point, so I've never heard of this actually being a problem yet, but conceivably it could happen.

Do we want to prevent it?

Eclipse

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.