Problem/Motivation

Basing on Coding standards / Object-oriented code / Naming conventions, class properties should use lowerCamel naming. The LazyPluginCollection uses a $instanceIDs that doesn't follow those naming conventions.

Running git grep --fixed-strings --line-number 'instanceIDs' gives the following output.

core/lib/Drupal/Component/Plugin/LazyPluginCollection.php:24:  protected $instanceIDs = [];
core/lib/Drupal/Component/Plugin/LazyPluginCollection.php:69:    return isset($this->pluginInstances[$instance_id]) || isset($this->instanceIDs[$instance_id]);
core/lib/Drupal/Component/Plugin/LazyPluginCollection.php:119:    if (!isset($this->instanceIDs[$id])) {
core/lib/Drupal/Component/Plugin/LazyPluginCollection.php:120:      $this->instanceIDs[$id] = $id;
core/lib/Drupal/Component/Plugin/LazyPluginCollection.php:131:    return $this->instanceIDs;
core/lib/Drupal/Component/Plugin/LazyPluginCollection.php:141:    unset($this->instanceIDs[$instance_id]);
core/lib/Drupal/Component/Plugin/LazyPluginCollection.php:157:    return count($this->instanceIDs);
core/lib/Drupal/Core/Plugin/DefaultLazyPluginCollection.php:67:      $this->instanceIDs = array_combine($instance_ids, $instance_ids);
core/lib/Drupal/Core/Plugin/DefaultLazyPluginCollection.php:69:      $this->originalOrder = $this->instanceIDs;
core/lib/Drupal/Core/Plugin/DefaultLazyPluginCollection.php:90:    uasort($this->instanceIDs, [$this, 'sortHelper']);
core/lib/Drupal/Core/Plugin/DefaultLazyPluginCollection.php:109:    $current_order = $this->instanceIDs;
core/lib/Drupal/Core/Plugin/DefaultLazyPluginCollection.php:112:    $this->instanceIDs = $this->originalOrder + $current_order;
core/lib/Drupal/Core/Plugin/DefaultLazyPluginCollection.php:123:    $this->instanceIDs = $current_order;
core/lib/Drupal/Core/Plugin/DefaultSingleLazyPluginCollection.php:96:    $this->instanceIDs = [];
core/modules/system/tests/modules/plugin_test/src/Plugin/TestLazyPluginCollection.php:30:    $this->instanceIDs = array_combine($instance_ids, $instance_ids);

Proposed resolution

In all those cases, the $instanceIDs property should be renamed $instanceIds. BC is not necessary, since PHP is not case sensitive and the property is protected.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jungle created an issue. See original summary.

jungle’s picture

apaderno’s picture

naveenvalecha’s picture

Thanks @jungle! RTBC+1
Open Question: Is there any issue that exists to address this convention in the whole of the plugin system?

apaderno’s picture

Title: Rename LazyPluginCollection::$instanceIDs to LazyPluginCollection::$instanceIds » Rename the $instanceIDs property used from plugin classes to $instanceIds
Issue summary: View changes
Status: Needs review » Needs work

Actually, running git grep --fixed-strings --line-number 'instanceIDs' gives me the following output.

core/lib/Drupal/Component/Plugin/LazyPluginCollection.php:24:  protected $instanceIDs = [];
core/lib/Drupal/Component/Plugin/LazyPluginCollection.php:69:    return isset($this->pluginInstances[$instance_id]) || isset($this->instanceIDs[$instance_id]);
core/lib/Drupal/Component/Plugin/LazyPluginCollection.php:119:    if (!isset($this->instanceIDs[$id])) {
core/lib/Drupal/Component/Plugin/LazyPluginCollection.php:120:      $this->instanceIDs[$id] = $id;
core/lib/Drupal/Component/Plugin/LazyPluginCollection.php:131:    return $this->instanceIDs;
core/lib/Drupal/Component/Plugin/LazyPluginCollection.php:141:    unset($this->instanceIDs[$instance_id]);
core/lib/Drupal/Component/Plugin/LazyPluginCollection.php:157:    return count($this->instanceIDs);
core/lib/Drupal/Core/Plugin/DefaultLazyPluginCollection.php:67:      $this->instanceIDs = array_combine($instance_ids, $instance_ids);
core/lib/Drupal/Core/Plugin/DefaultLazyPluginCollection.php:69:      $this->originalOrder = $this->instanceIDs;
core/lib/Drupal/Core/Plugin/DefaultLazyPluginCollection.php:90:    uasort($this->instanceIDs, [$this, 'sortHelper']);
core/lib/Drupal/Core/Plugin/DefaultLazyPluginCollection.php:109:    $current_order = $this->instanceIDs;
core/lib/Drupal/Core/Plugin/DefaultLazyPluginCollection.php:112:    $this->instanceIDs = $this->originalOrder + $current_order;
core/lib/Drupal/Core/Plugin/DefaultLazyPluginCollection.php:123:    $this->instanceIDs = $current_order;
core/lib/Drupal/Core/Plugin/DefaultSingleLazyPluginCollection.php:96:    $this->instanceIDs = [];
core/modules/system/tests/modules/plugin_test/src/Plugin/TestLazyPluginCollection.php:30:    $this->instanceIDs = array_combine($instance_ids, $instance_ids);

Since those are all plugin-related classes, that should be fixed in the same patch.

I changed the title to make the issue more generic.

apaderno’s picture

Issue summary: View changes
jungle’s picture

Status: Needs work » Needs review

Hi, @kiamlaluno. All you listed are in the following 4 files, which have been fixed by the patch already. I'd set it back to needs review.

  • core/lib/Drupal/Component/Plugin/LazyPluginCollection.php
  • core/lib/Drupal/Core/Plugin/DefaultLazyPluginCollection.php
  • core/lib/Drupal/Core/Plugin/DefaultSingleLazyPluginCollection.php
  • core/modules/system/tests/modules/plugin_test/src/Plugin/TestLazyPluginCollection.php
apaderno’s picture

I apologize: Only the issue summary and the title needed to be updated.

jungle’s picture

Apologize? Closed(won't fix) @kiamlaluno, Easy easy :p Thank you for helping/reviewing/commenting on this issue and other issues.

@naveenvalecha, very glad to meet here, and thanks!

quietone’s picture

Status: Needs review » Reviewed & tested by the community

Patch applies cleanly to 8.9.x. After applying the patch I ran `git grep --fixed-strings --line-number 'instanceIDs'` from the IS and nothing was returned. I reviewed the patch and it only changes the instanceIDs string as stated in the IS.

  • catch committed da6f725 on 9.1.x
    Issue #3115987 by jungle, kiamlaluno, quietone: Rename the $instanceIDs...

  • catch committed 08b6fd9 on 8.9.x
    Issue #3115987 by jungle, kiamlaluno, quietone: Rename the $instanceIDs...

  • catch committed 409d915 on 9.0.x
    Issue #3115987 by jungle, kiamlaluno, quietone: Rename the $instanceIDs...
catch’s picture

Version: 9.0.x-dev » 8.9.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 9.1.x/8.9.x/8.8.x, thanks!

Status: Fixed » Closed (fixed)

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