Problem/Motivation

Annotated classes will never have a leading slash, but putting class: '\Drupal\mymodule\MyClass' in a YAML-based definition is also valid.
Ideally the class itself wouldn't be compared by any code, but it is done in contrib, and should work consistently.

When using the MyClass::class constant, no leading slash is included.

Proposed resolution

Ensure plugin class names do not have leading slashes everywhere.

Remaining tasks

N/A

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#24 2824655-plugin-24.patch5.28 KBtim.plunkett
#24 2824655-plugin-24-interdiff.txt1.77 KBtim.plunkett
#22 2824655-plugin-22.patch4.44 KBtim.plunkett
#11 2824655-plugin-11.patch4.78 KBtim.plunkett
#11 2824655-plugin-11-interdiff.txt875 bytestim.plunkett
#10 2824655-plugin-9.patch3.92 KBtim.plunkett
#10 2824655-plugin-9-interdiff.txt1.69 KBtim.plunkett
#6 2824655-plugin-6.patch2.23 KBtim.plunkett
#6 2824655-plugin-6-interdiff.txt758 bytestim.plunkett
#5 2824655-plugin-5.patch1.49 KBtim.plunkett
#5 2824655-plugin-5-interdiff.txt671 bytestim.plunkett
#4 2824655-plugin-4.patch1.46 KBtim.plunkett
#4 2824655-plugin-4-interdiff.txt1.28 KBtim.plunkett
#2 2824655-plugin-2.patch1.08 KBtim.plunkett
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett created an issue. See original summary.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
Status: Active » Needs review
FileSize
2824655-plugin-2.patch1.08 KB
tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
Status: Needs work » Needs review
FileSize
2824655-plugin-4-interdiff.txt1.28 KB
2824655-plugin-4.patch1.46 KB

Defaults need to be merged in first.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
FileSize
2824655-plugin-5-interdiff.txt671 bytes
2824655-plugin-5.patch1.49 KB

Okay okay, I get it, typed config is weird.
Also this is why array-based definitions are bad.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
FileSize
2824655-plugin-6-interdiff.txt758 bytes
2824655-plugin-6.patch2.23 KB

So typed config has the same problem in the opposite direction: they are comparing class names as strings and assuming the leading slash:

  public function hasConfigSchema($name) {
    // The schema system falls back on the Undefined class for unknown types.
    $definition = $this->getDefinition($name);
    return is_array($definition) && ($definition['class'] != '\Drupal\Core\Config\Schema\Undefined');
  }

If that was changed to the more modern style:

  use Drupal\Core\Config\Schema\Undefined;
  public function hasConfigSchema($name) {
    // The schema system falls back on the Undefined class for unknown types.
    $definition = $this->getDefinition($name);
    return is_array($definition) && ($definition['class'] != Undefined::class);
  }

then the code would fail, since that has no leading slash.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
Status: Needs work » Needs review
FileSize
2824655-plugin-9-interdiff.txt1.69 KB
2824655-plugin-9.patch3.92 KB

Fifth time's the charm?

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
Issue tags: +Blocks-Layouts
FileSize
2824655-plugin-11-interdiff.txt875 bytes
2824655-plugin-11.patch4.78 KB

Updated now that LayoutPluginManager is in core

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented
Title: Plugin definitions should store their class in a consistent manner (with no leading slashes) » Plugin definitions should store their class in a consistent manner (with leading slashes)
Issue summary: View changes

I just corrected the issue title and summary

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
Issue summary: View changes
jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran as a volunteer commented

Do we need a change notice here?

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
Title: Plugin definitions should store their class in a consistent manner (with leading slashes) » Plugin definitions should store their class in a consistent manner (without leading slashes)
Issue summary: View changes 
+++ b/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php
@@ -239,13 +240,17 @@ public function useCaches($use_caches = FALSE) {
+      $definition->setClass(ltrim($definition->getClass(), '\\'));
...
+      $definition['class'] = ltrim($definition['class'], '\\');

The original IS and title were correct originally.

Added a CR
https://www.drupal.org/node/2847370

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran as a volunteer commented
Status: Needs review » Reviewed & tested by the community

Thanks!

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

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now 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.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
Version: 8.4.x-dev » 8.3.x-dev
EclipseGc’s picture

EclipseGc CreditAttribution: EclipseGc commented

I love that this is consistent now, except for TypedConfig. Whatever the case, definitely an improvement. ++

Eclipse

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
Issue tags: +Contributed project soft blocker
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Version: 8.3.x-dev » 8.4.x-dev

Note that issues like this should be targeted against 8.4.x, but can be considered for backport once they are committed to that branch. See the updated alpha release policy. Thanks!

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
FileSize
2824655-plugin-22.patch4.44 KB

`use` statement conflict, git apply -3 worked cleanly but reuploading to get a green patch again.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 22: 2824655-plugin-22.patch, failed testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
Status: Needs work » Needs review
FileSize
2824655-plugin-24-interdiff.txt1.77 KB
2824655-plugin-24.patch5.28 KB

@effulgentsia asked to switch from implicit to explicit test coverage.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
Status: Needs review » Reviewed & tested by the community

@effulgentsia asked to switch from implicit to explicit test coverage.

The changes made to \Drupal\Tests\Core\Plugin\DefaultPluginManagerTest::providerTestProcessDefinition are expliciting testing this with and without slashes using both arrays and PluginDefinition objects.

  • effulgentsia committed ed4ad77 on 8.4.x 
    Issue #2824655 by tim.plunkett: Plugin definitions should store their...
effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia at Acquia commented
Status: Reviewed & tested by the community » Fixed

Thanks! Pushed to 8.4.x and published the CR with the version set to 8.4.x.

Since this is a low-risk, low-disruption patch, I'm open to cherry picking it to 8.3 prior to beta, if this is in fact a "Contributed project soft blocker". But why is it? Please add a comment explaining that and set it to Needs review or RTBC, depending on whether that comment warrants peer review.

+++ b/core/lib/Drupal/Core/Config/TypedConfigManager.php
@@ -63,6 +63,17 @@ protected function getDiscovery() {
+    // Typed config definitions assume a leading slash, see ::hasConfigSchema().
+    if (is_array($definition) && isset($definition['class'])) {
+      $definition['class'] = '\\' . $definition['class'];
+    }

A follow-up to remove this would be great, since I think it should be fine to fix that line in hasConfigSchema() to use Undefined::class.

  • effulgentsia committed ec4be55 on 8.3.x 
    Issue #2824655 by tim.plunkett: Plugin definitions should store their...
effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia at Acquia commented
Version: 8.4.x-dev » 8.3.x-dev

Never mind. I figured it out. Contrib should be able to compare with Foo::class. Therefore, cherry picked to 8.3.x and updated the CR to be on that version.

Status: Fixed » Closed (fixed)

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