Follow-up to #1937600: Determine what services to register in the new Drupal class
We need to decide what we want to do with plugin managers, some of them are quite frequently accessed.
My suggestion:
1. Enforce the naming pattern that afaik all or most are already using, which is plugin.manager.$something
2. Add a Drupal::pluginManager($something)
3. For those that are accessed very frequently and provide custom methods, e.g. entityManager(), add a specific method so that we have type hints for the additional methods.
| Comment | File | Size | Author |
|---|---|---|---|
| #10 | 1950670-10.patch | 46.47 KB | damiankloip |
| #9 | drupal-plugin-manager-1950670-9.patch | 47.29 KB | berdir |
| #9 | drupal-plugin-manager-1950670-9-interdiff.txt | 544 bytes | berdir |
| #6 | drupal-plugin-manager-1950670-6.patch | 46.98 KB | berdir |
Comments
Comment #1
yched commentedDrupal::pluginManager($something), with $something = 'tour.tip' or 'views.field', i.e "a substring of a service id string" looks a bit odd DX wise.
Maybe Drupal::pluginManager($owner, $type) ?
+1 on Drupal::entityManager()
Comment #2
xjmYeah, I agree with #1 I think. Having two args would be way better DX. And I also think the explicit
entityManager()is a good idea.Comment #3
berdirTwo arguments shoulds fine to me, maps to how plugin implementations are defined. Do we want any special casing for those where it's the same, e.g. Drupal::pluginManager('block', 'block') is a bit silly, but so is Plugin\block\block\MyBlock.
I guess that means we will have to rename a few service names so that we can map the arguments to a service name...
Comment #4
yched commentedI'd say Drupal::pluginManager('block', 'block') is fine. Consistency++, IMO.
Comment #5
berdirLooks like the entityManager() already happened in #1982984: Create Drupal::entityManager for improved DX, so this just needs pluginManager().
Comment #6
berdirSomething like this..
There are some special cases, e.g. the layout_manager() that we could remove here and Views which already has Views::pluginManager(), not sure if we want to keep that?
Also added support for the service name plugin.manager.$owner where $owner == $type. Not sure if we want to support this.
In fact, I feel like we should discourage $owner == $type, it is IMHO more confusing to understand.
Renamed the rest resource.
Comment #7
tim.plunkett#1966246: [meta] Introduce specific annotations for each plugin type has a table with $owner and $type for each one, it shows a great deal of inconsistency.
Comment #9
berdirThis should fix the tests.
Not yet sure what to do with the core plugin managers (constraint, condition, archiver), as the are quite a mess in regards to naming. We could leave them out for now. It just means that pluginManager() doesn't work for them, which can be fixed/moved in separate issues.
Comment #10
damiankloip commentedRerolled.
How about we use an optional second parameter and go with logic like:
?
Comment #11
berdirI guess now that the plugin directory was simplified to a single argument, we should also go back to a single argument here I think.
Comment #12
dawehnerThis needs also a rerole.
Comment #19
berdirDoesn't look like there is a need for this.