Over in #1986988: Add a dedicated @CKEditorPlugin annotation, @tim.plunkett demonstrated an ability to shorten both the namespaces and directory structure of plugins discovered using AnnotatedClassDiscovery by extending the class and providing a custom namespace. In that particular example, he shortened the namespace and directory structure like this:

Before:

core/modules/ckeditor/lib/Drupal/ckeditor/Plugin/ckeditor/plugin/Internal.php

After:

core/modules/ckeditor/lib/Drupal/ckeditor/Plugin/CKEditorPlugin/Internal.php

Likewise, the namespace was also shortened:

-namespace Drupal\ckeditor\Plugin\ckeditor\plugin;
+namespace Drupal\ckeditor\Plugin\CKEditorPlugin;

Upsides
- Shorter directory structure and namespacing.
- Developers don't need to know which module (or Core) provides a particular plugin.

Downsides
- Requires each module providing a plugin create a custom discovery class extending AnnotatedClassDiscovery (see #6)
- Could result in namespaces conflicts. (see #16)
- Ambiguity around which module provides which plugin. (see #16)

Follow ups
#2055871: Introduce specific annotations for entity types

CommentFileSizeAuthor
#23 discovery-structure-1987298-23.patch62.37 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 55,580 pass(es). View
#20 discovery-structure-1987298-20.patch62.19 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 55,936 pass(es). View
#18 discovery-structure-1987298-18.patch62.19 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
#9 shorten-discovery-structure-1987298-9.patch21.69 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 55,500 pass(es). View
#6 plugin-annotation-1987298-6.patch21.71 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
#1 shortened-directories.png189.44 KBquicksketch
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

quicksketch’s picture

FileSize
189.44 KB

Just to attempt stretching this pattern across all core, here's what system/lib/Drupal would look like if this were extended to all plugins (left is before, right is after):

Shortened directory structures

In this example, I *kept* the "Plugin" directory.

It results in some very useful redundancy removal, such as block/block simply becoming Block.

Additionally it combines together locations that are separated for architectural reasons, but in practice are pretty much the same thing. Previously developers would need to distinguish between plugins provided by system module vs. Core, such as system/imagetookit or Core/Archiver. Afterwards, both are simply moved to the root of /Plugin, such as ImageToolkit and Archiver. Who provided the plugin is no longer required knowledge.

On the flip side, these individually-created plugin namespaces introduce ambiguity. Does "ImageToolkit" come from image.module? It's no longer clear. For developers that aren't using IDEs, this could result in more directory structure hunting, but at the same time it makes scanning all plugins provided by a single module easier because there is less nesting.

webchick’s picture

Oh god yes, please. That would be amazing. "block/block" consistently causes glassy eyes in front of audiences during the module porting presentation.

webchick’s picture

chx’s picture

> - Requires each module providing a plugin create a custom discovery class extending AnnotatedClassDiscovery

I am so much in support perhaps that could be used to fold derivatives into it.

chx’s picture

tim.plunkett’s picture

FileSize
21.71 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

This to me would be the first easy step, which would mean that if we don't care about nixing the /Plugin/ directory, you wouldn't need a custom discovery class.

Instead of specifying $owner and $type, which the Component AnnotatedClassDiscovery does not care about, we just pass the subdirectory "$owner/$type" as one variable. Then you can freely only pass "$owner$type" if you see fit.

Regardless of what we decide to do with core plugins, this makes contrib more flexible.

tim.plunkett’s picture

Status: Active » Needs review
FileSize
21.69 KB
PASSED: [[SimpleTest]]: [MySQL] 55,500 pass(es). View

No need for DIRECTORY_SEPARATOR here.

quicksketch’s picture

Instead of specifying $owner and $type, which the Component AnnotatedClassDiscovery does not care about, we just pass the subdirectory "$owner/$type" as one variable. Then you can freely only pass "$owner$type" if you see fit.

That was my thought too. The new patch looks great IMO. We'd need followups to actually move directories around for each module of course, but the basic concept looks like a huge win. Plugins that require complex nested layouts can have nesting, and those that are straight-forward (like Block) can just use a single directory.

As much as I would like to eliminate the Plugin directory, as long as we have other non-plugin classes inside of /lib/Drupal/[module_name], I think mixing Plugin would cause confusion between what's a plugin and what's not. Keeping Plugin also reduces the number of places where we would have namespace conflicts.

quicksketch’s picture

Although this isn't something we need to immediately worry about if we take the approach in #6, this is an opportunity to adjust our casing on our plugins. In my examples, I was changing "block/block" to "Block". Is that something we should pursue? It seems like long-term, everything is going to be CamelCase. This conversion could eliminate 2 lowercase-only locations if we chose to do that, leaving only the module name as lowercase.

tim.plunkett’s picture

Yes, I regret not making views plugins CamelCase when we first ported them.
IIRC, we only did it so that all of the views code that uses the plugin type names as string would match the directories (argument_validator, style, etc).

yched’s picture

I'm all for shortening what can be shortened, but I don't think we should remove any overall structure in the [module]/lib folder.

Modules will come with an awful lot of classes in D8: plugins for various subsystems, event listeners, page controllers, form controllers, entity types (almost plugins but not really) and associated entity controllers, native business or helper classes for the module, what else ?

Let's please not mix everything in the same.folder, or it's going to be impossible to answer those simple questions when e.g evaluating / reviewing a contrib module:
- what pages / forms does it provide ?
- what events does it react to ?
- what entity types does it provide ?
- what plugins, and for which system, does it provide ?

In other words, yay for simplifying what happens below the Plugins folder, but before we consider *removing* it, can we have a community discussion on how we're going to maitain human discoverability of what's in a module ?
Structureless bag of folders --

quicksketch’s picture

In other words, yay for simplifying what happens below the Plugins folder, but before we consider *removing* it, can we have a community discussion on how we're going to maitain human discoverability of what's in a module ?

That's pretty much what this is right now. Tim's initial example removed the Plugins directory, but so far in this issue no one is advocating for removing the Plugin directory. Like you say, there are a lot of other "things" besides plugins, mixing plugins in with everything else would make things messy quickly. Are you concerned about the approach in #6, where the directory structure under "Plugin" gets to be defined by the module defining the plugin? And given this ability, do you think it makes sense to reduce things like "block/block" to just "Block"?

tim.plunkett’s picture

Here are my proposals:

Core/Archiver       => Archiver
block/block         => Block
ckeditor/plugin     => CKEditor
Core/Condition      => Condition
Core/Entity         => Entity
layout/layout       => Layout
system/imagetoolkit => ImageToolkit
system/plugin_ui    => PluginUI

And leaving the rest alone for now.

EclipseGc’s picture

To capture my own thoughts here.

Tim walked me through the highlights of this issue in IRC tonight, I expressed 2 basic principles I want to see maintained in the final outcome of this. Based upon Tim's responses, I think they have been maintained with his approach.

Principle 1.) We need at least 2 points of diversity when identifying plugins of a particular type. Previously that has been $owner/$type. The currently proposed resolution seems to be essentially $dir/$annotation_class. This continues to provide the two points of diversity amongst similarly named plugin types (or even identically named since we can use a different annotation class... though it'd be crazy to do that, it's technically possible).

Principle 2.) Drupal/my_module/Plugin/block/block at least attempts to communicate where I might find the managing class for plugins of this type. Switching away from that isn't problematic for a lot of plugin types that exist in modules, but it could be problematic for new users looking for anything that's defined in lib/Drupal/Core. msonnabaum and I have discussed this before, the annotation classes have been introduced since that time, and utilizing these should again lend a clue as to where the developer might find more information on plugins of this type. Both of these things are TOTALLY obtuse, but once a developer gets used to utilizing the plugin workflow it becomes a really important thing to know about, and as long as we maintain a way to do that, I am still quite happy. This ABSOLUTELY means every core plugin should ship with its own annotation class, but I think we intended on doing that anyway, so perhaps that should not give us pause.

Maintain these two principles and I'll be very happy.

Eclipse

quicksketch’s picture

Hey Eclipse, I'm not sure I'm following your first point. You're saying we need "two points of diversity". Previously that was $owner/$type. From the proposals above, we're essentially making things be $owner$type (represented as a single directory, no slash), or in the case the owner and type are the same thing, such as "block", then we don't do anything so dumb as BlockBlock. So we're *not* keeping two points of diversity in some situations, because the name of the thing is the same as the owner.

Additionally, in situations where Core or system.module is the owner, we also drop the owner, such as in Tim and my examples, we'd just have Entity and ImageToolkit instead of Core/Entity and system/ImageToolkit. It sounds like these suggestions are not in agreement with your first point. Is that the case?

tim.plunkett’s picture

FileSize
62.19 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

@quicksketch from my discussion with @EclipseGc, he means that we have both the $subdir ($owner$type) as well as the custom @SpecificPluginType annotation for identification.

So, this issue along with the continued efforts of #1966246: [meta] Introduce specific annotations for each plugin type should be fine.

Here is everything I outlined in #15, except Entity (since there are dozens of entity types and that's too much effort to make without 100% consensus)

Status: Needs review » Needs work

The last submitted patch, discovery-structure-1987298-18.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
62.19 KB
PASSED: [[SimpleTest]]: [MySQL] 55,936 pass(es). View

Stupid macs, being case-insensitive. (At least, I hope that's the only thing wrong here)
This will need a core committer who doesn't use a Mac.

EclipseGc’s picture

@quicksketch

Yeah, so using block as the example, we are currently $owner = block $type = block so can be found in dir Plugins/block/block. And this moves us to $dir = Block & $annotation class = @Block, so any @Block annotated class in Plugins/Block is a "block plugin".

To make another example of why I've been really hard-nosed on this issue:

Let's assume Drupal Core defines a "cache" plugin. Views also defines one, so does ctools, and probably a half dozen other modules. As things stand right now that will end in in Core/Cache, Views/Cache, CTools/Cache, etc, etc, but if we remove that second point of diversity, someone's going to want it to be just Cache and likely ViewsCache, and so on. That's fine, except core can't possibly squat all the relevant named things out there, so there will be collisions becomes someone somewhere is just going to name their plugin "cache", and want it in the cache dir. That's why the AnnotatedClassDiscovery forces $owner/$type on developers, but since we now have annotation class specification we can say "Drupal core's Cache plugins can be found in Plugins/Cache with an annotation class of @Cache." Then if there's every some conflict, the only thing that NEEDS to change is the annotation class on one of them, instead of having to refactor all the files around. This is much more likely in contrib where two modules might create identically named plugin types. If someone tries to make them work together on the same site, a.) hopefully their annotation classes were named differently (and we should define a spec for that) and b.) if they weren't named differently, the patch to fix one or both of them should be fairly minimal.

Hopefully that makes sense of what I'm trying to say.

Eclipse

quicksketch’s picture

Gotcha. So basically you just want some direct tie between a plugin and its parent module. Since the custom annotation type will be coming from a specific module, and that is easily traceable (since there is a "use" statement at the top of the file for that annotation type), this satisfies your requirement.

I'm fairly sure the plan is to convert everything to individual annotation types, so seems like we're all in agreement.

@tim.plunkett: your list of suggestions makes sense to me except for CKEditor. Although I doubt it will be common, you could have other things than just plugins for CKEditor, e.g. CKEditorSkin. And since we're defining "a CKEditor plugin" and not CKEditor itself, keeping the word plugin in the name would be preferred for me.

i.e.

ckeditor/plugin => CKEditorPlugin

Which doesn't shorten the name up any but still eliminates the nested directory.

tim.plunkett’s picture

Title: [meta] Shorten directory structure and PSR-0 namespacing for plugins » Shorten directory structure and PSR-0 namespacing for plugins
FileSize
62.37 KB
PASSED: [[SimpleTest]]: [MySQL] 55,580 pass(es). View

Okay, that makes sense.

If no one minds, I'd rather just do this here. I don't want to touch Core\Entity until the last possible moment (since this is scriptable but also will break a lot of patches).

EclipseGc’s picture

@quicksketch,

yeah, I think I am on board with the proposed changes here. I'm just explaining why I'm on board since before the annotation class came into the picture I'd have fought this pretty strongly.

Eclipse

neclimdul’s picture

Status: Needs review » Reviewed & tested by the community

I think types will still be useful but they are not broken with this and it looks nice and does simplify things for a group of plugins. Probably makes use outside of Drupal more sane in most cases too.

quicksketch’s picture

This looks great to me too. I applied it locally and everything still functions. Seems there are no special requirements for applying the patch despite what should be case-sensitivity conflicts (i.e. block/block being replaced by Block, git apply works perfectly to handle the renaming of the directory).

This does result in some interim jankiness while some plugins are being converted to custom annotations. For example CKEditor module now has Plugin/CKEditorPlugin/Internal.php (the new style), while at the same time still has Plugin/editor/editor/CKEditor.php (old-style). Of course after #1966246: [meta] Introduce specific annotations for each plugin type's child issues are all completed, this will no longer be an issue.

+1 RTBC. The flexibility and the decrease in wtf-'d-ness is a huge win.

quicksketch’s picture

Issue summary: View changes

Removing requirement of separate AnnotatedClassDiscovery implementation.

fubhy’s picture

I love this patch!

Here are my proposals:

Core/Archiver => Archiver
block/block => Block
ckeditor/plugin => CKEditor
Core/Condition => Condition
Core/Entity => Entity
layout/layout => Layout
system/imagetoolkit => ImageToolkit
system/plugin_ui => PluginUI
And leaving the rest alone for now.

Will there be a follow-up for converting the rest? I would really like to see the CamelCase namespaces everywhere tbh. while we are at it!

quicksketch’s picture

@fubhy: They'll likely all be done as they're converted to custom annotations, so the list at #1966246: [meta] Introduce specific annotations for each plugin type applies to everywhere else, except for Core/Entity, which seems like it will be its own followup.

xjm’s picture

Issue tags: +Needs followup

Yes please. Thank you!

Let's file a postponed followup for the entity namespaces, tagged "Revisit before release." We'll want to go over all of those the week before code freeze.

alexpott’s picture

Title: Shorten directory structure and PSR-0 namespacing for plugins » Change notice: Shorten directory structure and PSR-0 namespacing for plugins
Priority: Normal » Critical
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Committed 19e6c2c and pushed to 8.x. Thanks!

I'm sure that some change notice need updating...

quicksketch’s picture

Oddly I couldn't find change notices that mentioned the namespaces or directories of any of the changed plugins except for block, which I updated at Blocks are now Plugins.

Archiver, CKEditor, Condition, Layout, ImageToolkit, and PluginUI all don't seem to have any mentions by their previous plugin names.

tim.plunkett’s picture

tim.plunkett’s picture

https://drupal.org/node/1911646 is CKEditor's change notice
https://drupal.org/node/1813372 is for Layout
https://drupal.org/node/1961370 is for Condition

PluginUI never have one, would have been #1535868: Convert all blocks into plugins

quicksketch’s picture

https://drupal.org/node/1911646 is CKEditor's change notice
https://drupal.org/node/1813372 is for Layout
https://drupal.org/node/1961370 is for Condition

Yes, sorry I should have been more clear. I found the original change notices for all of these things, but none of them have mentions of the namespaces or directory structures, so they didn't need updating.

tim.plunkett’s picture

Yes, but they should mention it :)
So the originals are incomplete.

YesCT’s picture

So what do we need here?
A general change notice for directory structure and namespacing changes,
*and* mentions in each of those other change notices, with links to the main change notice for this topic?

https://drupal.org/contributor-tasks/draft-change-notification
is instructions for drafting a change notice.
Anyone can try the first draft. :)
Just make a comment saying you are going to try, and ask questions if you have any.

Shyamala’s picture

Assigned: Unassigned » Shyamala

I am going to try the general change notice.

Shyamala’s picture

  • Directory structure of plugins are discovered using AnnotatedClassDiscovery by extending the class and providing a custom namespace. With this the directory structure and PSR-0 namespacing for plugins are shortened.
  • Advantages of shortening:
    - Shorter directory structure and namespacing.
    - Developers don't need to know which module (or Core) provides a particular plugin.
  • Block, Archiver, CKEditor, Condition, Layout, ImageToolkit, and PluginUI all follow the above defined convention.

Plugin directory structure
Before:

core/modules/ckeditor/lib/Drupal/ckeditor/Plugin/ckeditor/plugin/Internal.php

After:

core/modules/ckeditor/lib/Drupal/ckeditor/Plugin/CKEditorPlugin/Internal.php

Likewise, the namespace was also shortened:

-namespace Drupal\ckeditor\Plugin\ckeditor\plugin;
+namespace Drupal\ckeditor\Plugin\CKEditorPlugin;

Previously what was $owner/$type, from the proposals above, will be $owner$type (represented as a single directory, no slash) examples, ckeditor/plugin => CKEditorPlugin Core or system.module is the owner, we also drop the owner, examples, we'd just have Entity and ImageToolkit instead of Core/Entity and system/ImageToolkit

Examples of changes to core namespaces:

-namespace Drupal\block\Plugin\system\plugin_ui;
+namespace Drupal\block\Plugin\PluginUI;
-namespace Drupal\block_test\Plugin\block\block;
+namespace Drupal\block_test\Plugin\Block;
-namespace Drupal\language\Plugin\Core\Condition;
+namespace Drupal\language\Plugin\Condition;
aspilicious’s picture

Status: Active » Needs review

This also needs examples how to pass the namespaces in the plugin manager.

Shyamala’s picture

Added examples to the change notice.

aspilicious’s picture

I mean examples of an actual plugin manager *defining* the new name. The part were you choose your anotation name. When reading your notice it "seems" that it magicly works when creating the anotation class.

Thats not true, the shortened directory structure is forced in each plugin manager. Look at the ckeditor plugin manager for an example.

Shyamala’s picture

Assigned: Shyamala » Unassigned

Busy at the moment to complete this task, unassigning myself.

vijaycs85’s picture

Issue tags: -Needs followup
vijaycs85’s picture

Issue summary: View changes

Updated issue summary

vijaycs85’s picture

Issue summary: View changes

Updated issue summary with follow up #2055871

catch’s picture

catch’s picture

Issue summary: View changes

Updated issue summary.

yanniboi’s picture

I was thinking of having a go at taking over from @Shyamala on this, but I don't think I quite understand what @aspilicious wants an example of...

I had a look at CKEditorPluginManager.php, but I couldn't find anything that is "The part were you choose your anotation name. "

(unless they are talking about the use statement?)

chx’s picture

CKEditorPlugin is a particularly confusing example because of the repeated and different meaning of the word plugin -- ckeditor itself has plugins. Here are a few examples of the relevant call, here's where you choose your annotation name (in reality, annotation class):

parent::__construct("Plugin/migrate/$type", $namespaces, 'Drupal\Component\Annotation\PluginID');
parent::__construct('Plugin/CKEditorPlugin', $namespaces, 'Drupal\ckeditor\Annotation\CKEditorPlugin');
new AnnotatedClassDiscovery('Entity', $namespaces, 'Drupal\Core\Entity\Annotation\EntityType');

  • "Plugin/migrate/$type" is the plugin directory-namespace. Previously you would have passed the defining module (migrate) and the plugin type ($type) and the system had Plugin/$module/$type hardwired. This is the new thing: you are no longer bound to this structure -- if you want it, you can use it as migrate does but you need to pass in the whole thing because nothing is hardwired now. For example, CKEditorPluginManager is using 'Plugin/CKEditorPlugin' and EntityManager simply uses 'Entity'. Bliss!
  • $namespaces is just the module namepaces.
  • Drupal\Component\Annotation\PluginID is the annotation class. Right now, Migrate plugins only have an identifier, that's the simplest case there is.Drupal\Core\Entity\Annotation\EntityType is another annotation but it's probably one of the most complicated ones we have.
xjm’s picture

Issue summary: View changes
Issue tags: +Missing change record

Tagging "Missing change record", as completing the change record updates here blocks a beta: #2083415: [META] Write up all outstanding change notices before release

The patch for this issue was committed on May 7, 2013. This means that the needed change record updates for this issue have been outstanding for more than eight months. See the comments above for change records that could use simple updates. It would also be very valuable if someone who has deep understanding of the Plugin API could review @Shyamala's proposed change record and see if it is necessary, or if it should simply be incorporated into the main Plugin API change record. At this point documenting the D8 to D8 change is pretty unhelpful, because eight months.

We should also check https://drupal.org/node/2087839.

dawehner’s picture

chx’s picture

The BAP change notice https://drupal.org/comment/7616615 now contains the right Block namespace in several examples. I do not think this even needs that draft, really. We don't write d8-d8 notices.

dawehner’s picture

@chx
This is no longer true, seriously we need to write change notices for all changes.

chx’s picture

dawehner’s picture

Thank you for being so nice.

jessebeach’s picture

Title: Change notice: Shorten directory structure and PSR-0 namespacing for plugins » Shorten directory structure and PSR-0 namespacing for plugins
Status: Needs review » Fixed
Issue tags: -Needs change record, -Missing change record

CN: https://drupal.org/node/2186931

I edited the original CN started by Shyamala. It succinctly explained the change. I just formatted the text with a little more vertical spacing. It must also be noted that Views plugins are an exception to the new pattern.

I added a link to the Annotations-based plugins documentation to address aspilicious' comment in #39, https://drupal.org/node/1882526. This change notice concerns the PSR-0 namespace shortening, not how to define a new type of annotation plugin, so I'd rather just link to existing docs. aspilicious, did I understand your comments correctly?

I also went through each Plugin Change Notice and either updated or added PSR-0 namespace information.

Archiver: https://drupal.org/node/2003376
ImageToolkit: https://drupal.org/node/1993056
CKEditorPlugin: https://drupal.org/node/1911646
Layout [REVERTED]: https://drupal.org/node/1813372
Condition: https://drupal.org/node/1961370
Block: https://drupal.org/node/1880620
FieldFormatter: https://drupal.org/node/1805846
FieldType: https://drupal.org/node/2064123
FieldWidget: https://drupal.org/node/1796000
Mail: https://drupal.org/node/2191201
DataType: https://drupal.org/node/1806650
Action: https://drupal.org/node/2020549
Filter: https://drupal.org/node/2015901
ImageEffect: https://drupal.org/node/2072699

I'm pretty sure this satisfies all of the critiques with the change notice. I'm going to mark this closed. Feel free to re-open if you disagree. Or just edit the CN to correct.

Status: Fixed » Closed (fixed)

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