_edit_get_wysiwyg_info() and hook_edit_wysiwyg_info() are evil, crappy and designed to be temporary. We must convert this to use the plugin system.

Core should ship with a plugin that integrates Aloha Editor with Edit. Contrib could replace that plugin with one that integrates with another WYSIWYG editor, or even with *any* WYSIWYG editor.

Comments

Wim Leers’s picture

Issue summary: View changes

Updated issue summary.

Wim Leers’s picture

Issue summary: View changes

Updated issue summary.

yched’s picture

As a preliminary step to ease later migration to plugins, the array keys in your current hook_edit_wysiwyg_info() should use underscores instead of spaces. Plugin's annotation reader doesn't support spaces, so you'd have to convert those entries, which is always tedious / error prone / raises "wtf ?"s in reviews :-)

Wim Leers’s picture

That is a super helpful hint, thanks @yched! :) If you have any more directions on how you'd like to see this implemented: let me know!

Wim Leers’s picture

Status: Active » Fixed

Done. I'm sure this can be significantly cleaned up in the future though.

http://drupalcode.org/project/edit.git/commit/987c2dc

yched’s picture

A couple remarks on the commited code :

in function _edit_wysiwyg_get_field_editor() :

+    $type = new ProcessedTextPropertyEditorManager();

The usual core practice is to put the Manager in a DIC entry at 'plugin.manager.[owner].[plugin_type]', using a class EditBundle extends Bundle in edit module.

+        'definition' => $definitions[$plugin_id],
+        'plugin' => $type->createInstance($plugin_id),

Dunno if this would help here, but if your plugin classes extend PluginBase, then plugin objects can access their own definition with $plugin->getDefinition(). Would it allow getting rid of your $wysiwyg_plugin['definition'] here ?

in ProcessedTextPropertyEditorManager:

+ $this->discovery = new CacheDecorator(new AlterDecorator(new AnnotatedClassDiscovery('edit', 'ProcessedTextPropertyEditor'), 'edit_wysiwyg'), 'edit:wysiwyg');

This just in, but we're settling on a stacked notation, for legibility:

$this->discovery = new AnnotatedClassDiscovery('edit', 'ProcessedTextPropertyEditor');
$this->discovery = new AlterDecorator($this->discovery, 'edit_wysiwyg');
$this->discovery = new CacheDecorator($this->discovery, 'edit:wysiwyg');
+    $this->factory = new DefaultFactory($this);

Strictly speaking we should be passing $this->discovery instead of $this.
Some of the core code had to pass $this because of a WTF around #1764278: Run PluginManagerBase::processDefinition() in a ProcessDecorator, but this just got fixed, and we'll be returning to passing $this->factory.

Nitpick: ProcessedTextPropertyEditorManager / ProcessedTextPropertyEditorInterface
Is there a way the class names could be simplified a bit ? :-)

yched’s picture

Also, minor :

AnnotatedClassDiscovery('edit', 'ProcessedTextPropertyEditor');

Core settles on lower case (and thus underscore separated I guess, even though we don(t have multi words examples right now) for the 'plugin type name'.
EntityManager uses 'Entity', but that one is a bit special because it's owned by a Component rather than a module - it's also heavily rediscussed right now.

Wim Leers’s picture

*Thanks*, yched! :)

The usual core practice is to put the Manager in a DIC entry at 'plugin.manager.[owner].[plugin_type]', using a class EditBundle extends Bundle in edit module.

I looked a lot at Aggregator.module's implementation and there it is not done…

RE: simplification and pluginBase: I'm now using pluginBase — thanks a lot for the tip! :) At least somewhat less hacky now :)

RE: stacked notation for legibility: THANK YOU. Before it was a nightmare :P

RE: factory: okay, fixed, I was just doing what other examples in core were doing.

RE: plugin type naming: from what I've read, I understood that class-based things are fine to use CamelCase, the lowercase stuff is more for legacy reasons (familarity with previous APIs)?

RE: "Nitpick: ProcessedTextPropertyEditorManager / ProcessedTextPropertyEditorInterface", well I wanted to be as clear as possible. It encapsulates the whole: this is about making editors (not just WYSIWYG editors) pluggable for not plain text fields, but for processed text fields, and the proper Create.js term is "PropertyEditor widgets". (I already cut off the "widget" part.) I also much prefer conciseness, but for now I think this is best. I'm very much open to suggestions though :)

Commit: http://drupalcode.org/project/edit.git/commit/1f5004b.

yched’s picture

Happy to hear it could remove some hacks :-)

I looked a lot at Aggregator.module's implementation and there it is not done…

Yeah, aggregator was the 1st plugin that went in (possibly as part of the initial plugin API patch), so that was not really settled at this point. It should be fixed. All other plugin types that went in since then put their manager in the DIC.

Re: CamalCase as the $plugin_type string used as a second param for AnnotatedClassDiscovery :
no legacy reason here, plugins using AnnotatedClassDiscovery could have provided any kind of string they want (as long as they're suitable for a folder filename), and right now they all use lowercase, underscored strings - again, except EntityManager which is a bit special.

Actually, I think the main reason core currently uses lowercae for this is to have consistency with the name of the entry in the DIC :
plugin.manager.[owner].[plugin_type]
[owner] and [plugin_type] are lowercase-underscore there, and the same strings are used as params for AnnotatedClassDiscovery (i.e as folders in the lib/[module]/Drupal/Plugin tree.

So the two changes (DIC + lowercase) are actually related :-)

yched’s picture

Class naming : I'd tend to go with something like RichTextEditorPluginManager / RichTextEditorInterface - but well, I'm not really involved in the Create code and thus probably have a much more liberal bias regarding strict accuracy here ;-)
Note that we're namespaced, though, so perhaps some of the accuracy can be taken care of by the namespace part.

Wim Leers’s picture

RE: CamelCase -> underscores & DIC: Done.

RE: "RichTextEditor": the problem is that this is not limited to Rich Text Editors (RTEs). I wouldn't call BU Editor (here on d.o) an RTE. Nor would I call http://oscargodson.github.com/EpicEditor/ an RTE. They're just "processed text" editors. We could decide to call them RTE's for simplicity's sake, but it wouldn't be accurate.
I did rename it from ProcessedTextPropertyEditorBase/Manager to ProcessedTextEditorBase/Manager; that's already much shorter and still stresses the "processed text" part.

Why not just go with "editor" as the name used in the DIC? Because also non-textual fields have "editors", and thus that would be confusing.

Commit again: http://drupalcode.org/project/edit.git/commit/244c9f7 :)

yched’s picture

Cool - yes, ProcessedTextEditor[Interface\Base|Manager] sound better already :-)

Status: Fixed » Closed (fixed)

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.