Please add joshi.rohit100 to the commit message credits for creating the change record.
\Drupal\rest\Plugin\Type\ResourcePluginManager::getInstance() really just routes any calls to ::createInstance(). It is essentially useless.
Remove the method and replace all calls to it with calls to ::createInstance():
Before:
$resourceManager->getInstance([
'id' => $id,
]);
After:
$resourceManager->createInstance($id);
None.
None.
None.
| 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. |
| Comment | File | Size | Author |
|---|---|---|---|
| #4 | drupal_2503355_4.patch | 2.62 KB | xano |
| #4 | interdiff.txt | 574 bytes | xano |
Comments
Comment #1
xanoComment #2
xanoComment #4
xanoComment #5
lauriiiMarking this as a good task for a new contributor to take a look on writing change record.
Comment #6
tstoecklerOnce 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.Comment #7
xanoI 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.
MapperInterfaceunfortunately 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.
Comment #8
joshi.rohit100Comment #9
joshi.rohit100Here is the CR https://www.drupal.org/node/2503711
Comment #10
xanoThank you!
To core committers: please add joshi.rohit100 to the commit message credits for creating the change record.
Comment #11
lauriiiCR and the patch looks good.
Comment #12
joshi.rohit100#10 Yes :) :)
Comment #13
alexpottI 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.
Comment #14
mgiffordUnassigning.. Is this a 9.x issue or can it be dealt with in 8.1.x?
Comment #16
wim leersWe need to maintain BC during 8.x. So we can add things and change implementation details, but we cannot remove things.
Comment #17
wim leersComment #18
wim leers