Drupal\Component\Plugin\Discovery\DiscoveryInterface::getDefinitions() is documented as returning an array of plugin definitions. As there is no mention of the array keys, one can assume they are numeric and of no importance (an indexed array). However, most plugin managers use plugin IDs for the keys and a lot of code in core and contrib depends on that.
The only backwards-compatible solution is to rework all calling code to use the ID from the plugin definition. This would not change the method implementations, but it would require changing a lot of calling code.
The other solution, which I am suggesting, is to make the interface reflect current practice, which is that keys are plugin IDs. This is technically an API change, but will not matter much in practice, but most of the code implementing this method uses plugin IDs for array keys already anyway.
None.
None.
The keys of the array Drupal\Component\Plugin\Discovery\DiscoveryInterface::getDefinitions() returns must be plugin IDs (already done in practice).
| Comment | File | Size | Author |
|---|---|---|---|
| #3 | drupal_2458723_3.patch | 614 bytes | xano |
| #1 | drupal_2458723_1.patch | 618 bytes | xano |
Comments
Comment #1
xanoComment #2
Anonymous (not verified) commentedWell, this doesn't change the api, but only improves the documentation of the api and thus is allowed during the beta phase.
It is something you are getting, so you cannot control it. As a consequence "The keys are plugin IDs." (not "must be"), right?
Comment #3
xanoIt does change the API. The API is not our classes, as those are API implementations. The API consists (primarily) of the interfaces that we defined.
DiscoveryInterfacedoes not specify array keys, which means they are not necessarily part of the API. For example: someone can have implemented this interface without using array keys and it would be perfectly compliant with our API. It just won't work with many of core's classes. All this comes from the fact that PHP does not let us specify return values in this much detail without using code comments, which is why the documentation tag is also valid here.I updated the documentation according your feedback.
Comment #4
berdirYou are theoretically right, but in this case, that doesn't really matter.
It's not that "many core classes won't work", nothing would work, everyone who would try to implement it without taking care of the array keys would immediately notice, because getDefinition() would not work.
This *is* the API, we were just not documenting it properly.
Comment #5
Anonymous (not verified) commentedRe #3: I see what you are saying and can agree this is an api change.
A docblock finetuning won't have any immediate consequences (as in: nothing will break) for core/contrib development. And this patch improves DX by specifying in more detail what this method should do.
Furthermore, I think we actually all agree this should go in. So RTBC.
Comment #6
xano@Berdir: I understand this is mostly theory. Since these things are a hot issue so late in the release cycle, I wanted to be conservative and upfront about it as to prevent possible confusion about what happened here later on.
@pjonckiere: agreed. Thank you!
Comment #7
alexpottRe-titling because the current documentation is not incorrect it is just not complete. Also this is not an API change. Committed 95305c4 and pushed to 8.0.x. Thanks!