Suggested commit message

Please add joshi.rohit100 to the commit message credits for creating the change record.

Problem/Motivation

\Drupal\rest\Plugin\Type\ResourcePluginManager::getInstance() really just routes any calls to ::createInstance(). It is essentially useless.

Proposed resolution

Remove the method and replace all calls to it with calls to ::createInstance():
Before:

$resourceManager->getInstance([
  'id' => $id,
]);

After:

$resourceManager->createInstance($id);

Remaining tasks

None.

User interface changes

None.

API changes

None.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug
Issue priority Normal, because no data loss or site malfunction
Disruption Minor disruption for core/contributed modules: method call must be renamed, parameter simplified.
CommentFileSizeAuthor
#4 drupal_2503355_4.patch2.62 KBXano
#4 interdiff.txt574 bytesXano
#1 drupal_2503355_1.patch2.06 KBXano
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Xano’s picture

Xano
he/they
Primary language English
Location Southampton
CreditAttribution: Xano commented
Assigned: Xano » Unassigned
Issue summary: View changes
Status: Active » Needs review
FileSize
drupal_2503355_1.patch2.06 KB
Xano’s picture

Xano
he/they
Primary language English
Location Southampton
CreditAttribution: Xano commented
Issue tags: +Plugin system

Status: Needs review » Needs work

The last submitted patch, 1: drupal_2503355_1.patch, failed testing.

Xano’s picture

Xano
he/they
Primary language English
Location Southampton
CreditAttribution: Xano commented
Status: Needs work » Needs review
FileSize
interdiff.txt574 bytes
drupal_2503355_4.patch2.62 KB
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii commented
Issue tags: +Novice, +Needs change record

Marking this as a good task for a new contributor to take a look on writing change record.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented

Once the change record is written, this is RTBC.

This really shows how unintuitive our API is. Because semantically the thing you want to do is get an instance. You don't care whether it's being created, cloned or teleported from Mars, you just want to have one. However, you still need to use the createInstance() method.

Xano’s picture

Xano
he/they
Primary language English
Location Southampton
CreditAttribution: Xano commented

I understand your point of view and agree, with the addition that sometimes you may want to enforce the creation of a new instance after all. MapperInterface unfortunately is rather useless when it comes to API semantics.

Thanks for the reviews! I'll leave the task for the change record open for a few days so a novice can pick it up if they want to. After that I think anyone of us can create the change record as we'll have to move forward at some point as well.

joshi.rohit100’s picture

joshi.rohit100
he/him
Primary language Hindi
Location Delhi, India
CreditAttribution: joshi.rohit100 commented
Assigned: Unassigned » joshi.rohit100
joshi.rohit100’s picture

joshi.rohit100
he/him
Primary language Hindi
Location Delhi, India
CreditAttribution: joshi.rohit100 commented

Here is the CR https://www.drupal.org/node/2503711

Xano’s picture

Xano
he/they
Primary language English
Location Southampton
CreditAttribution: Xano commented
Issue summary: View changes

Thank you!

To core committers: please add joshi.rohit100 to the commit message credits for creating the change record.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Druid commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs change record

CR and the patch looks good.

joshi.rohit100’s picture

joshi.rohit100
he/him
Primary language Hindi
Location Delhi, India
CreditAttribution: joshi.rohit100 commented

#10 Yes :) :)

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Needs work

I can't see how this passes the beta evaluation - nothing is broken and there is no need to make this change.

We could deprecate ResourcePluginManager::getInstance() for Drupal 9 and replace its usages but there is no benefit in breaking contrib now.

mgifford’s picture

mgifford
he/him
Primary language English
CreditAttribution: mgifford at OpenConcept Consulting Inc. commented
Version: 8.0.x-dev » 8.1.x-dev
Assigned: joshi.rohit100 » Unassigned

Unassigning.. Is this a 9.x issue or can it be dealt with in 8.1.x?

Status: Needs work » Needs review

mgifford queued 4: drupal_2503355_4.patch for re-testing.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Version: 8.1.x-dev » 9.x-dev

We need to maintain BC during 8.x. So we can add things and change implementation details, but we cannot remove things.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Related issues: +#2625554: getInstance() fatals on almost all plugin managers
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs review » Closed (duplicate)

Version: 9.x-dev » 9.0.x-dev

The 9.0.x branch will open for development soon, and the placeholder 9.x branch should no longer be used. Only issues that require a new major version should be filed against 9.0.x (for example, removing deprecated code or updating dependency major versions). New developments and disruptive changes that are allowed in a minor version should be filed against 8.9.x, and significant new features will be moved to 9.1.x at committer discretion. For more information see the Allowed changes during the Drupal 8 and 9 release cycles and the Drupal 9.0.0 release plan.