WYSIWYG: Introduce "Text editors" as part of filter format configuration

Sorry, I strongly disagree with the plan of attack.

I was under the assumption that we extensively discussed this idea and it was clear that Filter module is not about editors, but about text formats, filters, and processing user input. We discussed that it is OK to write a module that depends on Filter module and enhances its administration pages (apparently that's on the roadmap of Wysiwyg module since eternity already). However, Filter module remains a decoupled extension that does not know of client-side editors.

Well we could always call it "WYSIWYG" module. ;) ;)

In seriousness though, I don't really understand what the point would be of introducing that separation. Core having the concept of editors (even if such an editor is "none") tightly linked with the filter system seems to me it's better for security, better for usability, and a bazillion other problems we have right now with WYSIWYG configuration in Drupal 7 and below. In other words, Nate's approach makes a lot of sense to me.

I'll ping some folks to chime in here.

Concepts

If the principle was to get rid of text format information altogether (well, we'd still have to keep filters of course), and then specifically get rid of the list of allowed HTML tags, I'd be against it. Site builders always need to be able to remain in control of the HTML mark-up that appears on their site. But that's not what you want to do, so then I'm all in favor :)
My thinking with Aloha Editor was to keep it as simple as possible; i.e. keep all of the current text formats UI/system, and simply automatically configure Aloha Editor based on that. There's a problem with that approach though: it's incomplete. Not every single UI aspect (button or otherwise) of a WYSIWYG editor depends on the allowed HTML tags (and attributes). I was hoping to address that post-feature freeze, though in a less invasive (but probably also less intuitive) manner. So I welcome this with open arms :)

Secondly, the introduction of a hook_editor_info() implies that Drupal core itself explicitly supports multiple editors. It makes sense if you want a site to support an "editor" (I won't say "WYSIWYG editor" anymore) for e.g. Markdown and a different one for HTML (let's hope that one will ship with Drupal 8 core). But if the goal is to make Drupal core itself support multiple editors for just HTML, then I have to say: what's the point? (AFAIK we agreed that having WYSIWYG API in core and the inherent "lowest common denominator" aspect of it are a bad thing. The ability to swap core's HTML WYSIWYG editor out for another one is of course sensible and essential.)

Thirdly, regarding renaming "Text formats" to "Text editors": I think this alone will make it a lot more understandable/approachable for end users.

- Removes the allowedTags list from being added automatically to JS. Not all text editor libraries are capable of utilizing it (or it may not be complete enough).

Shouldn't we *always* pass this, but editors can choose to ignore it? No HTML WYSIWYG editor should not support this, it'd be acceptable for e.g. Markdown editors.

Finally, AFAICT this patch does not yet include any of the UI changes you talk about in the issue summary's "Proposed Resolution" section, right? What's the status/plan of attack for that?

Patch review

+++ b/core/modules/filter/filter.api.phpundefined
@@ -61,6 +61,9 @@
+ *   - js settings callback: The name of a function that returns configuration
+ *     options that should be added to the page via JavaScript for use on the
+ *     front-end. See hook_filter_FILTER_js_settings() for details.

Why would JS settings only be relevant for use on the front-end? Surely they can also be relevant for editing on the back-end?

I think you might mean "client side"?

+++ b/core/modules/filter/filter.api.phpundefined
@@ -119,6 +122,73 @@ function hook_filter_info_alter(&$info) {
+  $editors['some_other_editor']['title'] = t('A different name');

s/some_other_editor/myeditor/, otherwise the example doesn't make sense.

+++ b/core/modules/filter/filter.api.phpundefined
@@ -119,6 +122,73 @@ function hook_filter_info_alter(&$info) {
+ * Note that changing settings here only affects the front-end behavior of the
+ * filter. To affect the filter globally both on the front-end and back-end, use

Let's do s/front-end/client side/ here.

"front-end" may be interpreted as "only for editing on non-admin, i.e. non-back-end pages". Whereas what you mean here is server side vs. client side.

+++ b/core/modules/filter/filter.api.phpundefined
@@ -119,6 +122,73 @@ function hook_filter_info_alter(&$info) {
+ *   the text formats for which a user has access.

s/for which/to which/

+++ b/core/modules/filter/filter.jsundefined
@@ -24,4 +29,64 @@ Drupal.behaviors.filterGuidelines = {
+Drupal.behaviors.filterEditors = {

"filterEditors" makes less sense to me than just "editors" or "formatEditors".

An editor cannot be filter-specific, thus this name makes no sense to me.

+++ b/core/modules/filter/filter.jsundefined
@@ -24,4 +29,64 @@ Drupal.behaviors.filterGuidelines = {
+    // If there are no filter settings, there are no editors to enable.
+    if (!settings.filter) {

filter settings -> format settings?

+++ b/core/modules/filter/filter.jsundefined
@@ -24,4 +29,64 @@ Drupal.behaviors.filterGuidelines = {
+          // Detach the current editor (if any) and attach a new editor.

s/a new editor/a new editor (if any)/.

+++ b/core/modules/filter/filter.moduleundefined
@@ -58,8 +58,8 @@ function filter_help($path, $arg) {
+      $output = '<p>' . t('Text editors define the HTML tags, code, and other formatting that can be used when entering text. <strong>Improper text editor configuration is a security risk</strong>. Learn more on the <a href="@filterhelp">Filter module help page</a>.', array('@filterhelp' => url('admin/help/filter'))) . '</p>';

Minor: I'd say "HTML tags and attributes" rather than just "HTML tags".
Attributes can also have security risks.

+++ b/core/modules/filter/filter.moduleundefined
@@ -494,6 +498,28 @@ function filter_formats_reset() {
+ * Returns a list of text editors that are used with 'filtered_html' elements.

I don't understand the 'filtered_html' reference?

I'm sorry, but I strongly disagree with the entire proposal.

This patch attempts to turn Filter module into something that it is not.

It further attempts to introduce a (sorry) very poor equivalent of Wysiwyg module's backend editor API.

Filter module is about text formats, and processing and filtering of text strings. It is not about client-side front-end editors. These are two completely different things, and the suggested changes attempt to turn Filter module into an undefined thing with multiple purposes, whereas Filter module has a single, and only one, clearly defined purpose only.

I repetitively stated in our discussions that Filter module must be retained as an independent unit. Just because there is a relation between text processing and client-side editors does not mean that it is OK in any way to turn them into a single thing.

I certainly agree that it is a big challenge to get the relation between an editor and a text format's configuration properly across in the UI and simplify the setup. But I have to put on my Filter module maintainer hat here, and from the module's perspective, the proposed changes do not make any sense at all. You could as well propose to merge OpenID module into Filter module. From the Filter module perspective, the proposed changes straight-out won't fix.

What is acceptable for Filter module is to make it aware that there might be another, separate concept of client-side editors involved (but not necessarily). This is why changes to enhance metadata for formats and filters are acceptable.

Furthermore, the proposed new editor hooks are similar to the essential backend part of Wysiwyg API, and I fail to see how that can be denied. This could have been approached by polishing and properly moving that part of Wysiwyg API into core, but we punted on that months ago. If we had not, then I would have worked on that in the past months. It simply doesn't make sense to me to start to investigate a core inclusion of Wysiwyg API less than one month before feature freeze.

Forgot that I wanted to underline this with a screenshot:

I apologize for my harsh comment. I already tried to get the message across in #5, but that didn't see sufficient attention. The interpretation that this sounds like a big fat "NO" is correct.

I am completely aware of those proposals since DC London (actually, Chicago already, no?), because we discussed them to great lengths. The end result of those discussions has always been the same: Drupal is modular. We can dynamically enhance the filter format configuration pages. Not every site needs or wants a client-side editor. The functional and architectural concepts are vastly different. It does not make sense to shoehorn client-side editor functionality into Filter module.

Thus far, the agreement and conclusion was to perform the required plumbing directly in aloha.module, since that module knows exactly what it can offer functionality-wise and UI-wise, and can thus perform the best informed decisions on how to integrate itself into the configuration page(s). We further concluded that we can adjust Filter module's administration forms to provide the necessary context and information (API-wise) to allow to properly enhance and alter it without having to go through ugly workarounds. And ultimately, we concluded that the entire plumbing can be specific to Aloha module — users who want a different editor are expected to disable Aloha module and install Wysiwyg module instead, which will do more or less the same, but in a generic, library-agnostic way.

What I'm currently missing is the answer to the question for why we're suddenly diverging from that plan and trying to implement generic client-side editor library support in core (which is very limited, and further limited to the backend).

All of you know that I kept on repeating myself like a parrot during the course of the past years, "We need Wysiwyg API in core", and there's (almost) nothing that I'd like to do more. But, if we do that, then we want to do it properly. E.g., starting by porting the module to D8 (which should be trivial), and refactoring and polishing what needs to be cleaned up, and ultimately, proposing to include the backend API parts in core, plus an editor integration (leveraging the plugin system). In other words, moving the infrastructure into core (and taking VIE/CreateJS into account), while retaining the ability for a/the contrib module to re-introduce the generic, so-called "proxy editor plugin" functionality (which everyone loves to shit over, but reality is, it is doable and just needs work :P).

Personally, I'd be open to try that — but as mentioned before, the chances for that to succeed, less than one month before feature freeze, are relatively small. However, in the worst case scenario, there'd at least be a polished and ready to rumble contrib module that can be pulled in by distros and profiles.

Thus far, the agreement and conclusion was to perform the required plumbing directly in aloha.module, since that module knows exactly what it can offer functionality-wise and UI-wise, and can thus perform the best informed decisions on how to integrate itself into the configuration page(s). We further concluded that we can adjust Filter module's administration forms to provide the necessary context and information (API-wise) to allow to properly enhance and alter it without having to go through ugly workarounds. And ultimately, we concluded that the entire plumbing can be specific to Aloha module — users who want a different editor are expected to disable Aloha module and install Wysiwyg module instead, which will do more or less the same, but in a generic, library-agnostic way.

What I'm currently missing is the answer to the question for why we're suddenly diverging from that plan and trying to implement generic client-side editor library support in core (which is very limited, and further limited to the backend).

That's a very good question.

One I think that can simply be answered as follows: this is a part of the problem that we didn't want to address as part of Aloha.module before feature freeze. I.e. Aloha.module is built so that Aloha Editor's UI will automatically adapt depending on which HTML tags and attributes are allowed by each text format. That works for most things, but not all things. For kind of "meta things", that doesn't work.
E.g. a button to switch to a "meta view", or a button to toggle a mode of the WYSIWYG editor where you can directly edit the HTML itself, or an imaginary plug-in that inserts a random quote of wisdom as plain text (and is thus unrelated to *any* HTML tag).

@quicksketch firmly believes that it's essential to be able to configure WYSIWYG editors by dragging and dropping UI components (buttons or otherwise) of the WYSIWYG editor in a … well, WYSIWYGy way. That was outside the scope of what we wanted to do as part of Aloha.module, but I'm definitely not opposed to it.

From my POV, this isn't really about "implementing generic client-side editor support in core", but rather the above. It's just that you need to know which editors are installed in order to be able to build such an interface in the first place.

I think the two different stances here can be explained extremely simply:

• @sun is approaching this from a technical software architecture POV
• @quicksketch is approaching this from a UX POV

I think both are right.

• I think @sun is right that the filter system itself is about 1) filters (potentially with settings), 2) ordered lists of filters, called "text formats", 3) a UI to create "text formats".
• However, I think @quicksketch is also right. Editors (WYSIWYG HTML editors, "almost WYSIWYG" Markdown editors, etc.) must be configured on a per-text format basis. WYSIWYG editors are also the thing that users want and expect to interact with when typing content on the web. Only for the most technical adept users, terms such as "text formats" and "filters" make sense. What @quicksketch is trying to do, is to make the filter system UI feel and seem less complex, by making the eventual UI that users will get to see a key part of configuring text formats.

@quicksketch is trying to cater to both the technically adept and the John Doe, by making the editor reach to text format changes and vice versa. Make a change, see the impact on the other thing. In the perfect software-architectural world, everything is nicely decoupled. But to make the former work, it is necessary to have them on one page, so you can make configuration changes in either of them and see the effect on the other.

(continuing the split ;))

I can understand that, but apparently we also concluded one more thing:

We oriented ourselves at other implementations, and what the 80% use-case is. Thus, Aloha in core == limited functionality — for more, see contrib. (Similar to WP)

Yes, that's probably a good summary. ;)

What I'm trying to say is that we can integrate with and can customize the filter administration. Just not directly in Filter module. Because Filter module is about text/string processing. Not front-end JavaScript libraries.

What I'm also trying to say is that the proposed changes here are extremely limited, and there doesn't seem to be a larger plan of attack for how to complete the big picture. Adding a hook_editor_info() is certainly not sufficient for anything. It's also crystal clear from my perspective that this code will ultimately end up looking exactly like wysiwyg.module (because it has to, or it would be even more incomplete than wysiwyg.module already is, and that wouldn't make sense). Thus, it's not clear to me why we'd even attempt to re-invent the wheel?

Whoa this looks pretty cool. Is it correct machine name is broken?

This looks great to me (functionality wise - no patch review).

One change I would make is to separate the choice of editor. And leave the existing choice of formats wording.

So user chooses CKeditor (for example), which gives bare bones toolbar at top.

Then selects 'Full HTML' or 'Filtered HTML' or 'Custom' format.

Then drags and drops buttons onto toolbar as they wish.

Then selects another format and toolbar at top reverts to bare bones, and the process begins again.

Not sure if we would need a save button to save changes to individual formats/toolbar's, or just one overall 'save configuration'

This keeps the config the user is performing to text/HTML formats, not editors.
Choosing an editor is a separate action, and changeable.
But with selection of different editor, the whole process should start again from scratch.

I hope it's clear what I mean.

Actually rethinking that, the user may want a different editor for a special case format.

So, user chooses text format from dropdown, or creates custom one.

Then chooses editor from list of available one's (must be installed beforehand).

Then configures the bare bones toolbar via drag and drop.

Then chooses a different format and the process repeats.

No toolbar is shown until an editor is selected.

This keeps us to configuring text formats as we do today, and allows unique editors for special cases.

I guess this was what you were heading towards.

Hm. Couldn't you still use a nice drag/drop button interface for building out the list of allowed tags, despite the fact that some buttons are only displayed contextually in certain editors?

@quicksketch I didn't see an [edit] link.

I think we do want such a builder for Aloha, we can as @webchick notices just allow adding the normal buttons. And perhaps we have a "select list" that allows you to switch the different contexts? It seems to be though that adding the normal buttons is like the 99% usecase.

I was just saying in a more verbose way, that you need 2 dropdowns, and that the dropdown for text formats would read as today. Then the other reads available (installed) editors. That the definition of allowed HTML tags is driven off of the drag and drop buttons. E.G. adding the 'bold' button creates the <strong> tag as an allowed HTML element in the filter.
I believe this is what you are doing in the demo, just that you have labelled the filters dropdown incorrectly IMHO and I think this adds confusion.

FILTERS FORMAT ----------> CHOSEN EDITOR

Full HTML -----------------> CKEditor
Filtered HTML -------------> CKEditor
Custom HTML -------------> BUEditor
Another custom format ----> Aloha
Plain text -----------------> No editor

I am in no way saying your approach is wrong, I absolutely love what you have done, I was just trying to clarify for myself and maybe for others, and also looking to help a little in driving this forward somehow. Just wish I could be of more help.

The purpose of this patch is to do both at the same time, since they affect each other. For example if you drag in the Unordered list button, you should be able to see that this will cause <ul> <li> tags to be added to your list of allowed tags.

Just to clarify, the patch doesn't currently do that, right? (From glancing over it and playing with the demo site, I didn't think it did.) I actually have a couple concerns about a UI that made that kind of thing happen automatically without at least an explicit confirmation... but no point mentioning them if the patch doesn't actually do it :)

I will mention that one of the reasons the WYSIWYG module approach (of two separate admin pages) always made some sense to me is that you're performing very different kinds of actions on each one: Configuring text formats is often about permissions and security, but configuring WYSIWYG buttons is about controlling the experience your users will have in the UI. Often, you might even expect different people (developers vs. lower-level admins) to do one task vs. the other... and having separate pages does facilitate that.

However, the patch here still seems to maintain that separation (they're on the same admin page, but still in clearly-separated parts of the page) so it seems reasonable to me. Haven't reviewed it in detail, though.

I think that a very clear notification and distinction (sort of like what you described) between changes to the WYSIWYG toolbar that affect the list of allowed tags and those that don't could work, without needing an actual confirmation.

Besides the security implications of adding new tags, the other concern I had is what happens when you remove a WYSIWYG button on a text format that's already in use. Example: Your users are littering their posts with h1 tags, so you want to remove the corresponding button from the WYSIWYG to discourage that. But there's a big difference between removing the button and removing h1 tags from the filter entirely (because the latter will affect existing content too). So somehow the difference between those two options should be clearly communicated.

Note that there are long-standing feature requests for allowing multiple editor profiles/configurations per text format.

There are various use-cases for that, but the one that was mentioned most is the ability to have one text format with restricted markup à la Filtered HTML and a default editor/configuration associated to it. However, the default editor would not expose buttons for all allowed markup to regular users (primarily to not overload the UI). Instead, there is an additional editor config attached to the same text format for privileged/advanced users, which exposes UI controls for all allowed markup.

All use-cases I know of share the common root cause that text formats are (rightfully) bound to user permissions and less-privileged users are not able to edit content that is associated with a more privileged text format. Additionally, it does not make much sense to have a "A bit less Filtered HTML" format and to expose that as an option for some users.

Introducing multiple editor profiles/configs per text format was/is on the roadmap for Wysiwyg 3.x and I had definitely foreseen it when working on an implementation for Drupal core.

1. #31: I love the way users can configure the toolbar. It's *very* nice :)
   
   What if I want to customize what the toolbar looks like for mobile users? Same text format, same editor, different device/browser/context. It's very simple: it's up to the module to add that configuration to the form; meaning that each WYSIWYG editor can use its own mechanism/paradigm: great.
2. Regarding Aloha/#36/#37: I agree with @webchick and @Bojhan. It still makes sense to have an overview of "all allowed buttons", even though it may not look identical to the resulting UI. Note that it'd be (very) hard to make it look identical (i.e. with the contextual buttons), but not impossible.
3. #43:
   

   I was anticipating automatically adding the tags upon button addition but only adding them permanently to the format upon save. If you dragged in a button by accident and removed it immediately, the tag would be removed. But after the format is saved the tag would become part of the list and require manual removal. Sound logical?

   Makes sense to me. Curious to hear what @David_Rothstein and @sun think.

4. #44: That's a good point, and the most mentioned use case is a very good one I think. However, isn't that something we could address in a follow-up commit? The important/hardest aspect here is that the feature gets in by feature freeze, adding 1:n instead of 1:1 support seems like something that can be addressed later.OTOH, the ability to *switch* from one editor profile/config to another one needs some UI, and it needs to be accessible etc., so maybe it cannot happen post-feature freeze…

@quicksketch, what I am asking for is actually what you have on the demo site.
The only change I would make to that site to make things clear, is the names of the text formats. They should remain as 'Filtered HTML', 'Full HTML', 'Custom HTML' as today. Not 'Aloha Editor', 'CKEditor' as you have on the site. The distinction needs to be drawn and highlighted between editing 'editors' and 'text formats'.

I was anticipating automatically adding the tags upon button addition but only adding them permanently to the format upon save. If you dragged in a button by accident and removed it immediately, the tag would be removed. But after the format is saved the tag would become part of the list and require manual removal. Sound logical?

I think so, but wonder a bit if this will confuse people who actually are trying to remove a tag, since the same action results in different results in different situations. I guess it could work if the result is very visible, though - we'll have to see :)

Now with the move of Edit's JS to be built on top of http://createjs.org, we actually also have the concept of a "PropertyEditor widget". (A "property" being a field, and the possible "editors" are: "direct" (for contentEditable-based editing of textual fields), "direct-with-wysiwyg" (for in-place WYSIWYG editing of textual fields), "form" (for form-based editing of all other fields). So it is very different.) This "editor" aspect will not be user-visible though, so I don't think there will be an issue there.

The "edit" module is about in-place editing, which is related to but very different from configuring text editors. So is this really an issue? I think changing the user-visible name of the "editor" module to "Text Editor" instead of "Editor" might help there.
(Note that we renamed it to "edit", it was originally called "ipe" — for "in-place editing". It was renamed because Panels already had "Panels IPE". I definitely don't have strong feelings about the name.)

Thank you for working so hard on this, @quicksketch. I actually have a lot of ideas, suggestions, and also concerns, but from a pragmatic perspective, I think it is necessary to put the cart before the horse:

I'd really like to co-work on this and help to flesh this out, but ~11 days before feature freeze, I really don't see how such a major feature could reasonably mature enough to be core-worthy and commit-ready — in only 1 week and 4 days.

There's nothing I couldn't agree more with though: We need a better infrastructure and better APIs in Drupal core in order to address the fact that more than 80% of all users are installing a client-side editor, regardless of whether that editor is shipped with core or not.

At the same time, I still stand by my earlier comments: I'm highly confident that the end result of this issue (and its cumulated, future follow-up issues) will end up duplicating (at minimum the PHP parts of) wysiwyg.module to at least 90%, but probably more.

By now, it is also clear that Wysiwyg module's JS parts duplicate CreateJS, at minimum to the basic extent of attaching and detaching editors to semantically marked up page elements — I had the chance to briefly talk with @henribergius about what Wysiwyg module is doing (certainly incomplete), and he clarified that CreateJS doesn't handle editor library plugins at all yet, and I personally arrived at the conclusion that Wysiwyg module maintainers should contribute most of what we have there to upstream CreateJS, so as to entirely eliminate all of those Drupal-specific parts of Wysiwyg module and inherently share it with all other CMS applications that are using CreateJS.

My personal battle plan for Drupal core always was to attack the Wysiwyg 3.x roadmap first (which mainly consists of significant architectural clean-ups), and meanwhile, incorporate new findings and knowledge that did not exist before, and only afterwards, attempt to move the core-worthy parts into Drupal core.

In an over-simplified nutshell, that plan would boil down to:

1. Implement editors as plugins on the PHP-side, leveraging D8's new plugin system. (A proper interface, no info hooks, and no arrays of doom)
2. Attach editors (editor profiles) to text formats, including access permissions (text format access != editor access).
3. Re-implement most or even all of the client-side editor library integration code via CreateJS. Shift as much as possible to upstream, because upstream affects much more than just 2% of Drupal sites powering the net. That is, because upstream has the potential to shift this to 50+%. But most importantly, because only a joint cross-community effort like CreateJS has the power to end the utterly senseless duplication of client-side editor libraries (which still does not decline, even in times of HTML5).

That's a lot of work, which involves to open various cans of worms, regardless of whether it happens in the contrib module or in core. We ought to do it, that's for sure.

In turn, all of this boils down to this for me:

In order to achieve this in a well thought-out and architecturally clean way, we need an exception to feature freeze.

That's a lot to ask for with respect to other core contributors whose feature requests are bound to feature freeze. But OTOH, this effort never was an official initiative, and thus it never saw the same amount of attention as other core initiatives. Furthermore, since all of this would be possible to achieve from within a contributed module, the resulting, effective changes for Drupal core are "tacked-on" with regard to almost all parts of the functionality. Thus, no existing functionality is affected, neither in core nor in contrib. The only modules and people who would be affected are the maintainers of editor integration modules — and that space literally boils down to Wysiwyg module and CKEditor module these days, and as the lead of the former, I'd be more than happy for this exception to happen.

Now, I'm aware that this comment doesn't address or solve any of your feedback questions. But circling back into where I started, I could probably hammer this issue with code, functionality, and UI/UX reviews for the next 4+ weeks, at which point we'd potentially have a well thought-out and architecturally clean implementation, but feature freeze will be history. I also do not see how this change proposal could reasonably arrive at a 80% ready state within 11 days. Thus, I believe, from a pragmatic perspective, this question has to be asked, and it has to be asked first. If the answer is "No", then there are a gazillion of other, less significant features that are still possible to achieve before feature freeze, so our precious contributor time would be better spent on those.

What do you think?

Yes, I know that Acquia as well as individual contributors have invested lots of time and money to work on this. However, given that the exceptions of Drupal 7 have been one of the biggest mistakes in Drupal's history, and that most of them played a significant role for the sheer never-ending D7 cycle, I would not bet on any exceptions. At least, I'm fairly sure all of us try hard to learn from past mistakes, and thus, any kind of release cycle exception starts with "a mistake" from my perspective/experience. Therefore, IF such a thing were to happen, it first requires concrete, concise, and comprehensible reasoning, and second, we can immediately postpone the release date by a few months, each. (~10 exceptions for D7 roughly translated into +18 months, and while some of that delay was caused by other architectural stuff, the concrete exceptions itself definitely shaved off essential contributor time from other critical issues — and the architectural side looks way worse for D8; actually so much worse that I have serious doubts on the scheduled timeline today already.)

Again, that's not meant to say I wouldn't like to see an exception. It merely says that there are direct (and heavy) consequences of such a decision. That's why I do not take it for granted.

re: Filters as plugins and Editors as plugins:
We're converting text formats into Configurables right now, filters as plugins will follow directly afterwards. All of that is plain refactoring, so not bound to feature freeze, so this will happen for D8. Almost all info-style hooks will die in favor of proper PHP interfaces. The transition from info hooks to typed classes is straightforward, so there's no reason to introduce a new info hook. :)

I've reread the entire issue. If you do the same, I think you'll also notice we're coming from very different points of view and that we're in fact kind of talking past each other instead of to each other. I already something like that in #20, but now I see it more clearly.

sun: "NO, no WYSIWYG API-like functionality, that's for contrib, core should just provide tight integration with a single WYSIWYG module! That's why I'm in favor of Aloha: put a single, deeply integrated WYSIWYG editor in core, so that most users no longer have a need for a WYSIWYG API module."

quicksketch: (coming from the angle that Aloha is not sufficiently mature/stable, disappointed with the current UI, etc, hence now working on CKEditor in parallel)
"Let's move the plumbing that Aloha and CKEditor and any other WYSIWYG editor need in core (which will make it easier to compare the two, too), so that contrib doesn't have to do it over and over, plus to avoid silly conflicts between contrib modules. WYSIWYG API module tries to have a WYSIWYG plugin abstraction and that is an impossible thing to realize due to each WYSIWYG editor having different UI paradigms and very different ways for doing seemingly 'typical WYSIWYG tasks'."

Is it even possible to reconcile these POVs?

I agree with sun that it makes sense to ship one WYSIWYG editor with core, and do it well. Want multiple editors? Contrib will help you. If that's the case, then there's little point in having a "editor" module.

But I also agree with quicksketch that it is vital that different WYSIWYG editors can offer the ability to configure their toolbars/available functionality on a per-text format basis in a usable manner. To do that, it seems we do need a "editor" module.

I wish there was a clear-cut answer.

Further feedback I have upon rereading this issue, both for quicksketch and sun:

Additionally the full list of HTML tags (and attributes) could be wildly huge. See the tinyMCE documentation for the list of allowed tags/attributes that match the XHTML spec: http://www.tinymce.com/wiki.php/Configuration:valid_elements

Absolutely true. That's one of the reasons I preferred parsing to have a system where filters explicitly list allowed tags. But that was shot down. This was the only alternative.

Note that it may be true that not every WYSIWYG editor uses it, but from a purely logical POV, they should need to know about this, if they want to guarantee that Drupal won't filter it away. The problem is: Drupal is kind of unique with its (very abstract and very flexible) filter system, so no WYSIWYG editor out there really caters to such an advanced setup.

Spark team still pushing for inline editing in core, which is at least 3 more steps after this one

For some reason, you seem to have always misunderstood us/me: in-place editing is not X steps beyond WYSIWYG editing. You're implying that in-place editing is a superset of WYSIWYG editing, and that's simply not true.
"True WYSIWYG" editing is a flagship feature of in-place editing for sure. But in-place editing really is independent of WYSIWYG editing. If you have no WYSIWYG editor installed, you'll still be able to use in-place editing.
They're two disjoint sets, but it is possible to grow them to they intersect, then clicking on a field to edit it in-place through a "true WYSIWYG" editor would be the intersection.

Create.js can be useful here, but you'd only need a tiny subset of Create.js, plus it's not yet written in such a way that you can actually use that tiny subset handily. For the Drupal back-end (i.e. WYSIWYG editors in forms), that tiny subset needs to be directly usable. This is nice to have down the line, but in the current grand scheme of things, I don't think this is vital yet.
Especially because Create.js indeed does not yet care about which WYSIWYG plugins are loaded or even per-field/editor/editable/… WYSIWYG editor configurations, you'd have to make a lot of upstream contributions before you'd be able to leverage it in a useful way.

Thanks :) — I can't leave the interpretation in #54 without correction though:

My stance always was that Wysiwyg API needs to be in core, and nothing else.

The only reason for why I agreed with AE in core is 1) not everyone agrees with my stance on Wysiwyg API, 2) AE has the unique concept of Aloha blocks / nested editables which is an absolute requirement for a system that claims to manage content, and 3) after focusing on one particular editor, I'm 101% confident that we collectively will conclude that a single editor does not meet Drupal's architectural design and possible use-cases, thus circling back into where I started, but for D9 or D10. This might sound arrogant to you, but my experience as maintainer of the Wysiwyg project clearly shows this as a given, fundamental reality: There is no single editor that is suitable for everyone, there are about ~4+ new libraries every single year, and the entire technology/library space is evolving much faster than we're able to cope with.

I've also repetitively stated that we need Drupal distributions to make a decision on one or more editor libraries that are shipped with them. Once we can learn from cumulated Drupal distribution usage, there would be no barrier for adding one or potentially even more than one editor library to Drupal core itself. Which library, or whether there is any library at all in core, does not matter at all, because core should focus on providing and ensuring the right infrastructure only.

In short: There's nothing I could disagree more with - putting a single editor into core and focusing on that is only guaranteed to lock us into a space we absolutely do not want to get locked into, and doing so will hinder Drupal's ability to follow latest and greatest web standards and technologies beyond Drupal 8.

Furthermore, up until recently, Wysiwyg module was maintained for D5, D6, and D7 in one go; comparing the code-bases nicely reveals that the entire editor-specific functionality is completely detached from Drupal core, it was ~99% identical across Drupal core versions.

In short: I'm all-in, +1,000 for editor.module or actually wysiwyg.module in core. I merely don't see how we're able to produce something substantially mature in the remaining time-frame. If we do this, we should move Wysiwyg module into core, and we should learn from its mistakes and limitations, but keep almost everything else. And it should be done in a way that allows a contributed wysiwyg[_advanced].module to extend on the core functionality, without having to re-invent all the wheels being involved from scratch.

I was a bit bored today and remembered I came across this issue recently. I did not read every line in here today, so I may be skipping ahead, but I've begun porting Wysiwyg module to work with the proposed API in #31. Just want to know what the possibilities for contrib are.

I'm not going to comment on all the things mentioned here (the hows and whys of inline editing & WYSIWYG). Just noticed a patch was posted that could theoretically remove large chunks of Wysiwyg's code, and I wanted to test that theory.

Quick summary:
Had to port Wysiwyg to 8.x first, obviously...
Made the minimal amount of changes to make it work with the current HEAD, then began porting to #31 from there.
Almost all the backend stuff working. UI looks a bit stuffed when moving all of Wysiwyg's widgets in there. Wysiwyg's own vertical tabs won't work. (Interestingly, all of how Wysiwyg's original D7 configuration UI still remains functional in parallel with this new UI.)
Will continue with the frontend/JS parts soon, but need some sleep first.

I'm aware this effort will most likely be trying to hit a moving target, but I don't mind that. Quicksketch posted a demo of Aloha Editor working with an earlier version of the patch, I liked that and I might do something similar and post the code if anyone is interested. If not, I'll probably just make a few comments about potential issues I find along the way.

@quicksketch Looking forward to your reroll, can we perhaps get any pointers on CKEditor work?

I've read the issue comments so far, reviewed the patch in detail and tried the demo site. All-in-all I agree this is doing what is minimally required to make the editors attach to formats and in doing so it satisfies the concerns raised by @sun that it should be in its own module even though it goes great lengths to do so. I also find it encouraging that @TwoD started porting WYSIWYG API to this approach and found no problems. That reinforces that although we are not moving the module en-masse to Drupal core, we move a useful set of functions that contrib could still work with. Very useful validation.

Overall the only thing that was surprising for me was to see this very D7-esque info hook approach but that is also extensively discussed above with people not necessarily agreeing if it should be an info hook or plugin. AFAIS info hooks are going out and typed classes are the way to go at least in the eventual Drupal 8 code, but that does not necessarily mean the initial patch should be hold up with this. I'm of the type of people who prefer steps done vs. no step done because it is not yet fully up to all requirements. That said, I can help with review and/or coding if people believe we should absolutely move this to plugins before it gets committed.

TwoD: Is your code on Drupal.org anywhere? I didn't see any recent work on WYSIWYG API in the Git repo.

I went through this issue once more.

To quicksketch #12:

Shouldn't we *always* pass this, but editors can choose to ignore it? No HTML WYSIWYG editor should not support this, it'd be acceptable for e.g. Markdown editors.

The problem was that our "blackbox" testing was wildly complex with little actual benefit. I spent a whole day revamping the blackbox testing before I found that the tradeoff between code weight and performance wasn't going to provide valuable information for all editors. Obviously the plain text editor (no editor) doesn't need it. BUEditor doesn't do filtering on the front end. Neither does CKEditor (other than dangerous tags). The assumption that all (or even most) WYSIWYG editors support a tag list is not true.

This is actually no longer true. CKEditor will add this functionality in version 4.1 (due Feb 1, 2013): #1260052-134: Candidate WYSIWYG editors, https://dev.ckeditor.com/ticket/9829.

And #13:

Additionally the full list of HTML tags (and attributes) could be wildly huge. See the tinyMCE documentation for the list of allowed tags/attributes that match the XHTML spec: http://www.tinymce.com/wiki.php/Configuration:valid_elements

If your site had more than a couple of text formats (since settings are *per-format*), you could significantly increase the page weight. Hence why I thought it better for each editor to provide their own JS settings entirely, rather than attempting to provide generic settings that could potentially be used across editors.

Page weight is a non-issue. WYSIWYG editors require JS, and if JS is required, we could also do an AJAX request to retrieve the settings (and potentially even cache it in localStorage — in D8 we can assume that localStorage is available!); Edit module must do something similar to retrieve metadata about the fields on the page, for page weight and render cache reasons.

Essentially, #23 says why I agree with quicksketch:

What I'm also trying to say is that the proposed changes here are extremely limited, and there doesn't seem to be a larger plan of attack for how to complete the big picture. Adding a hook_editor_info() is certainly not sufficient for anything.

Yes I agree that the scope of changes here is fairly limited. That's why I initially proposed it as a patch to Filter module. But limited as it is, it's the consistency that it brings to binding editors to formats that is important. It also brings consistency to the exposure of filter settings to the front-end. Even though I'm not pushing for a generic plugin API, having the individual settings for filters makes it possible for either filter or editor modules to incorporate the functionality of that filter into the WYSIWYG interface

To @sun in #50:

By now, it is also clear that Wysiwyg module's JS parts duplicate CreateJS, at minimum to the basic extent of attaching and detaching editors to semantically marked up page elements — I had the chance to briefly talk with @henribergius about what Wysiwyg module is doing (certainly incomplete), and he clarified that CreateJS doesn't handle editor library plugins at all yet, and I personally arrived at the conclusion that Wysiwyg module maintainers should contribute most of what we have there to upstream CreateJS, so as to entirely eliminate all of those Drupal-specific parts of Wysiwyg module and inherently share it with all other CMS applications that are using CreateJS.

I applaud the goal. But I can now say with confidence: please remember that Create was designed for editing on the front-end, not for editing on the back-end. Create is barely an abstraction for editor libraries. It's much more about providing common infrastructure to build an in-place editing UI/UX on top of.
If you want to use Create on the back-end, you'll not only have to contribute things upstream, you'd have to rearchitect Create entirely to be able to use only the very little code that we really need on the back-end.
I would love it if we were using the same abstraction on the front- (in-place editing, Edit.module) and back-end (forms, e.g. node/%/edit), but this is Drupal 9 material. Create currently has deep dependencies on Underscore, Backbone and soft dependencies on jQuery and jQuery UI Widget. That's a lot of JS to load on back-end forms where you typically won't need all the "JS app" stuff of Backbone.

In essence, I wholeheartedly agree with every statement of @quicksketch in #51. The scope of WYSIWYG module as outlined in #50 goes to far, the scope outlined in #51 (and as followed by @TwoD in #57) is the minimally viable thing. The limited feature set as described in #23 is why this is necessary.

To @sun in #55: I notice that I didn't apologize yet. I didn't want to put words in your mouth, of course. I was merely trying to word what you had told me in the past, but clearly I have misunderstood you at some point.

I'd like to move this issue forward. I think we need to clear up the "JS settings callback" stuff, and if we indeed don't need it, properly get rid of it in the patch (there are still a few remnants to that). Further, I agree with @quicksketch's reasoning that it doesn't make a lot of sense to have Editors be plugins while Filters are not yet plugins.

@quicksketch: #1763974: Convert entity type info into plugins might be useful for that :) Wim also made plugin conversions in Edit module at http://drupalcode.org/project/edit.git/commitdiff/987c2dc5244f391647de73... - and you can find plugin API docs at http://drupal.org/node/1637614 explaining the concepts.

I'm currently working on this (conversion to D8 plugin system) and hope to have an initial version ready for review tomorrow.

@webchick, no, the code is not public anywhere yet, been too busy to clean up the loose ends and make it patch worthy. Will try to put something up within a few days.

Done. Looking forward to your feedback :)

Changes:

• hook_editor_info() is gone in favor of a plug-in manager that discovers plug-ins which should implement a new EditorInterface (which closely resembles what hook_editor_info() was doing).
• "client side" -> "client-side" plus more minor stuff
• D7's Drupal.settings -> D8's drupalSettings
• fully removed "editor js settings callback" — there were still some remnants
• changed the submit handler on the settings form into a validate handler — this makes more sense given that the actual saving happens in editor_form_filter_admin_format_submit(), the submit handler was only used for massaging the data into the right form.
• kept module name (machine name) as "editor", but rename all human-visible references to "text editor". There were plenty of places already where you were using "text editor", most notably in this very issue, so all I did was use it consistently. Now there is a "text format" and a "text editor", so it's consistent & clear.

Things that already were not (or not anymore) supported, but that are now explicitly absent:

Q: How to add filter-specific settings?

(You had already made this change, but the .api.php file didn't reflect this yet.)
A: implement hook_editor_js_settings_alter(&$settings, $formats), load the filters for each of the text formats, if any of the text formats uses the filter, then add your settings. This is more work indeed, but until many filters want to use this functionality, we want to keep the overhead low and the API simple.

Q: How do we ensure that the text editor that users associate with a text format is compatible?

(This is something that you simply have not yet addressed AFAICT.)
A: by implementing any necessary compatibility checking logic in EditorInterface::settingsFormValidate(). CKEditor does not have tags requirements (that I'm aware of) and can work for both plain HTML (zero FILTER_TYPE_MARKUP_LANGUAGE filters) and for BBCode (which is a FILTER_TYPE_MARKUP_LANGUAGE filter). Aloha only works when

• the text format doesn't use any filter of the FILTER_TYPE_MARKUP_LANGUAGE type -> easy to check
• the p and br tags are allowed -> only check that the filter_html filter allows this, if contrib-provided filters are installed that limit allowed HTML tags, it's fair to assume that the user is knowledgeable enough that we don't need to do a deep analysis of the text format (the blackbox testing that I was doing for Aloha); the only alternatives are to include blackbox allowed HTML tag/attribute testing or to make the Filter API more complex, neither of which is desirable

---

Next thing I'm working on (and have started already): figuring out how and where I can simplify Edit module (#1824500: In-place editing for Fields) by making it leverage as much as possible from Editor module:

• The CKEditor implementation for Edit already is leveraging Editor's metadata (see #1873500-1: CKEditor + Edit).
• ProcessedTextEditorInterface::checkFormatCompatibility() can go away, since that is now something that can happen while configuring/associating a text editor with a text format.
• Assuming we can add optional metadata to plug-ins implementing EditorInterface and maybe an optional method, we can have all the necessary "true WYSIWYG" text editor metadata in plug-ins written for Editor.
• ProcessedTextEditorInterface::addJsSettings() can be replaced by a few lines of code in Edit.module plus leveraging EditorInterface::generateJsSettings()
• Due to all of the above, we should be able to drop Edit's ProcessedTextEditorInterface entirely.

Oh, and here are the changes to your CKEditor module to convert it to the above plug-in-based approach: #1874332-1: Convert to use the plugin system.

#68: editor_module-1833716-68.patch queued for re-testing.

#72: doh! But also yay! :D

#73: well, you could change it split it over two lines:

get('plugin.manager.editor');
$editors = $manager->listAvailableDefinitions();

The first line is really all about D8's DIC and getting the right dependency out of it. It's a little bit of boilerplate of course, but that's the way it is meant to be in D8 (at least right now, and at least to my understanding).

If that's your biggest complaint, then I did a not too shabby job :)

#74: Weird, I did not touch that code *at all*!? Note that your patch is only refused by the testbot because it doesn't contain a trailing newline… Attached is the patch of #74 but with a newline.

Hm, I know we're trying to reduce abstraction layers, but $editors = drupal_container()->get('plugin.manager.editor')->listAvailableDefinitions(); is quite a bit more of a mouthful than $editors = editor_info();. Do we have a convention for making this sort of thing easier?

Apparently not, but it's under discussion. See #1875086: Improve DX of drupal_container()->get().

Also:
- We never really established solid conventions for naming services in general, or plugin managers specifically. We could potentially shorten 'plugin.manager.editor' to 'editor.manager'. We don't need to bikeshed that in this issue.
- listAvailableDefinitions() seems unnecessary to me. It's called in two places in the patch. In one, getDefinitions() would be perfectly adequate. In the other, we do want something that can be set as the value of '#options'. It might be nice to introduce a generic utility function for that, since as we convert more systems to plugins, this will likely surface for quite a few of them.

I don't think it makes sense to hold up this issue on such nits though, but just pointing out we still have some possibilities for improving DX.

The bottom part of #68 is coming tomorrow.

#77 & #78: thanks, that's helpful!

RE: Editor module

• @quicksketch: you don't have any other remarks? I made a few minor functional changes, such as no longer passing in the existing settings to each text editor plug-in's JS settings generator, since I couldn't see where this was necessary. It's very well possible that I'm not seeing your original intention though, so please tell me if I oversimplified it.
• In this reroll, I only converted editor_add_js() and its helper function to work with #attached (new name: editor_get_attachments()) and I got rid of its state tracking, which is necessary to make this work well with the way the Blocks system will work in D8 [1]. 21 less lines of code.
  In light of what @Crell said at #1875086-7: Improve DX of drupal_container()->get(), I think it might make sense to move editor_get_attachments() onto the plug-in manager to improve testability?

[1]: Note that it is possible that the same attachments get added multiple times per page; this is necessary in Drupal 8 because each block gets rendered independently and should know the attachments it needs. It's up to the asset bundling logic to ensure the same attachments are loaded only once. However, the automatic merging of Drupal JS settings was causing problems for CKEditor specifically. Fix at #1875632: JS settings merging behavior: preserve integer keys (allow for array literals in drupalSettings).

RE: leveraging Editor module from Edit module (as described at bottom of #68)

Essentially:

1. Get rid of Edit's ProcessedTextEditor, but leverage Editor's Editor instead, by having the following optional Editor plug-in annotation: supports_true_wysiwyg = "TRUE". If that annotation is added, then the following methods should also be implemented in the plug-in's JS file:
   • onChange()
   • attachTrueWysiwyg() (better name suggestions are of course welcome)
2. Edit provides a drupalwysiwygwidget.js ("direct-with-wysiwyg") Create.js PropertyEditor widget that will automatically work for any "Editor" text editor that is annotated with supports_true_wysiwyg = "TRUE". [2]
   When the user wants to edit a processed text field that has a text format with an associated editor that has supports_true_wysiwyg = "TRUE", then that text editor will be attached while editing in-place by using attachTrueWysiwyg. In all other cases, it will fall back to the "form" Create.js PropertyEditor widget.
   No "compatibility checks" have to be done anymore, because the user is now explicitly associating a text editor with a text format; and thus it makes total sense to use that same text editor on the front-end, *if* it supports "true WYSIWYG", if it does not, then a form will be used, and the same text editor will then be present on the form in the "form" Create.js PropertyEditor widget.

To test:

1. Apply the attached Editor #80 patch.
2. Apply #1875874: Minor clean-up for Edit (more generic WYSIWYG editor integration facilities, prevent Drupal behaviors on "transport" forms).
3. Apply the attached Edit patch.
4. Install http://drupal.org/project/wysiwyg_ckeditor.
5. Apply #1873500-2: CKEditor + Edit.
6. Apply #1875632: JS settings merging behavior: preserve integer keys (allow for array literals in drupalSettings).

[2]: In the future, I'd like to move the drupalwysiwygwidget.js Create.js PropertyEditor widget from Edit.module into Editor.module, because it is highly dependent on Editor.module (it leverages the same metadata, and just adds a few optional things). Doing that here, however, would lead us too far off topic. I'll make that possible in #1874936: Pluggable in-place editors (allow modules to define new Create.js PropertyEditor widgets) and then simply move some things around. Until then, however, I realize this is suboptimal to review but does clearly show the direction.

@sun: I do realize that what I'm doing here is mostly *against* how you envisioned we'd be able to leverage Create.js. This way, Create.js doesn't offer a solution to the WYSIWYG API question at all. Well, there's two reasons for this: 1) Create.js really is *not* a WYSIWYG API, instead it is about providing client-side editors for any kind of content, not just WYSIWYG-editable text, 2) Create.js' current "support" for Aloha, CKEditor, Hallo and Redactor is not only extremely superficial, it also makes too many (simplistic) assumptions. We could make Create.js better, and we should, but that is currently not the top priority. The top priority is to get a WYSIWYG editor in core, in a decoupled way. Once that is done, I'd be happy to move more responsibilities into Create.js.

Next steps:

1. Reviews of the Editor module patch (especially from quicksketch) — this is the top priority because this is what blocks WYSIWYG in core now.
2. I'll also start working on #1874936: Pluggable in-place editors (allow modules to define new Create.js PropertyEditor widgets), so that the Editor module can cleanly implement a Create.js PropertyEditor widget for Edit.

I've just been playing with http://quicksketch.org/wysiwyg

I have one word. WOW.

As a USER - I love this. Juggling wysiwyg and filters and keeping track of all the different places I need to poke to iron out issues is one of the things I personally find most frustrating about Drupal - this demo seems to almost entirely eliminate that pain.

I can't speak to the technical implementation details at all, and I don't know how far the latest patches have deviated from the demo - but the demo is fabulous. The fact that it leaves the door open for aloha OR ckeditor is also a big plus in my view.

The ability to just drag buttons and dividers onto the toolbar is SO much easier than the monster list of check boxes.

Now I wonder... could the filter order processing and enabling tabs be combined?

Kind of like this?

Or perhaps draggable into disabled / enabled areas.

Since I am on a mobile right now ill report that their is no wysiwyg during comment authoring and buttons can't be dragged around in the ckeditor admin (obviously ). Solvable problems for sure.

I finally got around to re-roll my Wysiwyg port today, or at least try.
D8 changed so much between me having time to look at it I've spent more than 5 hours just trying to figure out the relationships of all the classes involved in Editor module and the plugin system.

I've just begun rewriting the actual Wysiwyg code now that I know where to start and what's likely to need a poke, but a few things are holding me back.

Editor API forces me to use hook_library_info(), through the 'library' property in the @Plugin annotation. It implies I already know which library version the end user has installed, and which library files need to be loaded. My first attempt at porting Wysiwyg to work with this patch just used a dummy library to avoid errors and kept all the other file loading code intact. I forgot about this in my last comment, but it popped up again while looking through the latest patch.

Wysiwyg currently detects which version, and possibly which variant, of an editor package is installed and loads different sets of files depending on those conditions. I can't figure out a clean way of doing that while following the example set by http://drupal.org/project/wysiwyg_ckeditor. It can hardcode the version of CKEditor since it bundles the library with the module itself, something we feel very strongly about not doing in Wysiwyg module, and also a reason why http://drupal.org/project/libraries exists.

It seems I have no reliable way of knowing anything about which editors will actually be used during the current request from inside wysiwyg_libraries_info(), so I'd have to load metadata about all of them to parse out the currently installed version. If there even is any. That is a large part of Wysiwyg I was hoping to be able to remove or at least refactor to be more efficient, and this API forces me to keep it if we're not to drop or hack around this flexibility.

I might be missing something which would help with this situation, and I'm far from porting all of Wysiwyg yet so I might stumble upon a remedy, so suggestions and ideas are very welcome.

#81: the relevant issue for that aspect is over at #1834682: Consolidate filter options in the UI when configuring a format :)

#82: it seems that on http://quicksketch.org/wysiwyg, the default text format is "Filtered HTML"; that text format doesn't have a WYSIWYG editor associated with it, hence you don't see any. If you change the text format to "CKEditor", you'll get CKEditor on the comment form.
Ability to configure the toolbar on touch devices: good point, see #1872206-4: Improve CKEditor toolbar configuration accessibility.

#83:

• Sorry to hear it took you so much time, but indeed, D8 has made some major changes lately. I think the best way to figure out what you need and what Editor provides is for us to have a chat. You have my Skype and my e-mail address; can you let me know when is a good time for you?
• Note that the version *is* in fact present, thanks to hook_library_info(). Look at the sample implementation of CKEditor that leverages this proposed Editor module: its hook_library_info() lists the drupal.ckeditor library (the glue between Editor and CKEditor) and ckeditor (the actual WYSIWYG editor library). So I think we could provide you with the needed version information by extending the EditorInterface plug-in annotations a little bit to include the actual WYSIWYG editor library as well, so that any module can easily figure out what the version is that is being used.
  Even when a contrib module would override it with a different version through hook_library_info_alter() (by changing paths and version number), that would work. Worse even: whenever a contrib module would alter the "glue library" (in this case drupal.ckeditor) to use a different actual WYSIWYG editor library, then they should implement hook_editor_info_alter() and would be able to point to that different actual WYSIWYG editor libary; and all will still be well.
  Alternatively, we could also simply include the version of the WYSIWYG editor library in the plug-in annotation directly, or add a custom flag to the library definition that points to the actual WYSIWYG editor library.

  
  Somewhat related: #1787222: [meta] Strategy for updating vendor JS libraries within a major stable version.

• It seems like your concerns boil down to the fact that WYSIWYG API really needs to know version numbers. Why is that?

  
  In general, the WYSIWYG module tries to provide *all* the plumbing (including a generic WYSIWYG editor plug-in/extension API). The Editor module keeps the scope as small as possible and defers handling plug-ins for editors to the modules that provide the editor. And that brings us to the biggest other difference: the module that implements an EditorInterface plug-in also includes the JS for the WYSIWYG editor (or, in contrib, could of course rely on Libraries API if they wanted); hence there is no need for extensive version handling. The reason you need version handling in Libraries API + WYSIWYG API is because users must download the library manually, so you have no way of knowing the version number in advance. And because you want WYSIWYG API to be able to handle pretty much any version of any WYSIWYG editor.

  I think the only way Editor can work is by leveraging the existing infrastructure, i.e. hook_libary_info(). The only way the WYSIWYG API module can continue to work exactly like it does today, is by having the Libaries API functionality. There was work being done to get Libraries API in D8, but that was stalled because it was deemed to not yet be ready (#667058: Add a libraries folder with a README.txt in it to DRUPAL_ROOT, #1431804: Bikeshed 'lib' vs. 'libraries' directory, #1167496: Libraries API in core and http://drupal.org/node/1342220).

• That's all I can think of for now. Looking forward to your feedback. If you can, let's chat, because that'd be faster.

The Edit patch of #80 (step 3 in the testing process) was missing a crucial file. This patch includes that file.

Note that the version *is* in fact present, thanks to hook_library_info().

Not sure what your point is there, I already mentioned wysiwyg_ckeditor hardcodes the version. It doesn't care if the library is even installed or not so it can do that.

It seems like your concerns boil down to the fact that WYSIWYG API really needs to know version numbers. Why is that?

You said it:

The reason you need version handling in Libraries API + WYSIWYG API is because users must download the library manually, so you have no way of knowing the version number in advance. And because you want WYSIWYG API to be able to handle pretty much any version of any WYSIWYG editor.

Exactly, why shouldn't it be allowed to?

The version number from hook_library_info() is normally meaningless and unreliable if you don't bundle the library, and few contrib modules do, which is why I've not made a push to use it in Wysiwyg. To make it meaningful, you have to inspect the library files and make it a fact. That currently has to be done for every single editor library, on every single page load, because I have no clue yet if the library I expose will even be used and/or is present.
Caching could help, but I'd still like to at least know if the main library folder is there at all when showing the list of available editors.

If I didn't have to provide a version number in hook_library_info(), I could get around that and any module calling drupal_get_library('wysiwyg','tinymce') would not be allowed to assume version X.Y.Z is installed, just that we support that specific library. (hook_library_info() doesn't allow me to enter a version range, now does it?)
Instead they'd contact Wysiwyg module's API, ask for the library to be inspected for facts (if that's not already cached), and after that they'd be able to be sure that if Wysiwyg reports a version number, it also knows how to deal with whatever is installed without crashing.

If we don't inspect the library, any module exposing an editor plugin through Wysiwyg's API may also end up serving the wrong version of the plugin, or altering settings which are no longer relevant. If no known library version is present, other modules should not have their hooks invoked and we should not try to load the library at all to prevent cryptic errors. We HAVE TO factually know the version of the library supplied by the user, or we'll crash and burn once an attempt is made to attach the editor. Guess who gets blamed for that? ;)

With the release of CKEditor 4, we can't even have a hardcoded list plugins known to exist in version X because you can now bundle any combination of them into an official package. All we've got to go on is the version string from ckeditor.js for API recognition and [thankfully!] the build-config.js file detailing which plugins were actually included. That doesn't mean we can control everything though, since CKEditor 4 will by default enable all bundled plugins for all instances. This becomes a nastier issue when users discover a new plugin from the CKEditor 4 site that they'd really like to use on only some of their formats, but absolutely not on the others. Since CKEditor 4 will enable that by default, Wysiwyg must add the plugin name to the removePlugins list in the settings for each format for which it's not enabled. For formats where it's actually desired to use the plugin, we will still not know much about which buttons it provides without fully parsing ckeditor.js - and I'm not sure I want to go down that road - so I'll probably just show a message to the user about this and ask them to implement hook_wysiwyg_plugin() to provide meta data.

Alternatively, we could also simply include the version of the WYSIWYG editor library in the plug-in annotation directly, or add a custom flag to the library definition that points to the actual WYSIWYG editor library.

Forcing a single hardcoded version number anywhere else is just as bad. Not sure what you mean about a flag, but the point is that Wysiwyg supports multiple version and variants, so it must be able to accurately provide info about the actual state to other modules.

....hence there is no need for extensive version handling [in Editor module]

If you don't want to handle versions in Editor, why require the use of the 'library' property in the @Plugin annotation? It forces me to register a library through hook_library_info(), which in turn requires me to use a known version of that library. Libraries we've even been discouraged from putting into Drupal VCS and instead require users to download separately. Yes, I know I can use a hook to override the plugin annotation data or the data supplied via hook_library_info(), but those hooks are useless if I don't already know which library to load so I can inspect just that one when it's needed.

So I think we could provide you with the needed version information by extending the EditorInterface plug-in annotations a little bit to include the actual WYSIWYG editor library as well, so that any module can easily figure out what the version is that is being used.

Yes, I've begun creating a new WysiwygEditorInterface extending EditorInterface. Not specifically to put an "installed version" property in there though. Wysiwyg currently extends the "editor metadata" from hook_INCLUDE_editor() with properties like "installed version", "installed" (TRUE/FALSE), etc. All that will be retrievable as part of the interface itself though.
Some of the meta data currently used by Wysiwyg module is being put into the annotations, ignoring the fact that having any useful data in a code comment seems like an enormous WTF in itself. But that's a personal preference and I see no way of preventing Core going further down that alley so I'm not going to push that argument. ;P

Even when a contrib module would override it with a different version through hook_library_info_alter() (by changing paths and version number), that would work.

It would also have to implement all other alter hooks possible and override pretty much all the logic, forms, and meta data in wysiwyg_ckeditor if there are major API or plugin changes. Not much different from the current Wysiwyg situation, but Wysiwyg's core supports using a completely separate implementation if useful (FCKeditor->CKEditor transition), fully supplied by any module.

Worse even: whenever a contrib module would alter the "glue library" (in this case drupal.ckeditor) to use a different actual WYSIWYG editor library, then they should implement hook_editor_info_alter() and would be able to point to that different actual WYSIWYG editor libary; and all will still be well.

The "glue library" used by ckeditor_module is module-specific and should not be in hook_library_info() in the first place, judging by the hook description:

Registered information for a library should contain re-usable data only. Module- or implementation-specific data and integration logic should be added separately.

This got a lot longer - and probably sounds more hostile - than I intended it, but please remember it's not a "heck no!" to anything you've done so far. I'm actually very impressed by your work and dedication and I'm sure there's probably just a misunderstanding of intentions somewhere here. Like you said, let's see if we can have a chat about it instead. I'll try to be online on Skype as much as possible in the upcoming week or so.

Hah, I suppose this will be my last rant in 2012, with only an hour left of it over here. ;)

See ya next year all, and have a nice one!

Thanks for your feedback, it's very helpful :) I will digest all of it on Jan 2 and try to come up with a first step to address your concerns.

Please be aware that it is quicksketch who pushed this to where it is today, not I.

Please note that I'm approaching this from a "the minimally viable plumbing to get a decoupled WYSIWYG editor in core" POV. This has consequences for the amount of assumptions we could potentially make, versus the very few (indeed if any) assumptions you can make in WYSIWYG API.

Finally, your remark about hook_library_info() being used inappropriately: nice catch :), and it's true in D7, but no longer in D8; in D8 *everything* should be a library. Besides; it is not true that it is by definition the CKEditor "glue library" is not reusable: I've shown above that Edit can reuse it, and WYSIWYG API could too, potentially.

It was a good, constructive rant to end 2012 with :)

Glad you found it constructive, and yes, I'm aware of those things. :)
Wasn't aware of the policy change on that hook though.

Btw, is the patch in #85 also missing some other changes? Two methods called in the drupalwysiwygwidget.js file seem to be missing:

toolbarView.getMainWysiwygToolgroupId(),
toolbarView.getFloatingWysiwygToolgroupId()

#88: Those two methods are provided by #1875874: Minor clean-up for Edit (more generic WYSIWYG editor integration facilities, prevent Drupal behaviors on "transport" forms).

Full reply to the fundamentals of #86:

• To clarify what I said in #87: I'm approaching this from a "the minimally viable plumbing to get a decoupled WYSIWYG editor in core" POV, and if a WYSIWYG editor is going to ship with Drupal 8, it's not going to use Libraries API, because that has (unfortunately) been going nowhere for Drupal 8. Hence, it the WYSIWYG editor's JS will be included as part of a regular Drupal module and made available through hook_library_info().
• I now understand your continued attention for version number much better. Thanks!
  Note that I never said or intended to imply that the WYSIWYG API module shouldn't be allowed to handle every possible version. I was just asking "Why?" and trying to answer it myself.
• Note that over at #1787222: [meta] Strategy for updating vendor JS libraries within a major stable version, the consensus was that we should make version numbers and version handling an explicit part of hook_library_info() and friends. Including version ranges. That would alleviate most or at least some of your concerns, no?
• That being said, I think we can nuance this part:
  

  If we don't inspect the library, any module exposing an editor plugin through Wysiwyg's API may also end up serving the wrong version of the plugin, or altering settings which are no longer relevant. If no known library version is present, other modules should not have their hooks invoked and we should not try to load the library at all to prevent cryptic errors. We HAVE TO factually know the version of the library supplied by the user, or we'll crash and burn once an attempt is made to attach the editor. Guess who gets blamed for that? ;)

  That makes sense for WYSIWYG API. And we shouldn't prevent WYSIWYG API from doing/achieving precisely that. But why should Drupal core have to handle all that (at least without Libraries API)? The vision that quicksketch put forward (and I agree with it) is simply this:

  • Editor module provides the most minimal "WYSIWYG API" possible (ability to configure on text format page, these settings are available somewhere in drupalSettings and when the user switches from one text format to another, the Editor module's JS will handle the detaching/(re)attaching of WYSIWYG editors). This implies: Drupal modules ship with a specific version of a library, and the Drupal module for the WYSIWYG editor has its own (hook-based) API to let other modules inject plug-ins/extensions for this WYSIWYG editor; hence it's that module's responsibility to handle versions. (Yet each library should specify a version and keep it up to date, if it doesn't, then that's bug in the module.)
  • WYSIWYG API module can sit on top of Editor module and handle all other aspects in a more generic fashion. If it wants to handle "any version in existence", depends on Libraries API and whatnot, that's all *fine*!
  • From a pure software architecture POV, I love how WYSIWYG API has the furthest decoupling and maximal reuse. From a practical POV, however, something as extensive as the full WYSIWYG API module feels overkill/overengineered to ship with every Drupal installation. I'm not saying it's overengineered in and of itself, it has so many indirections because it wants/needs to support everything and more, I'm saying it's overengineered for Drupal core itself, since shipping WYSIWYG API with Drupal core itself implies we want users to be able to upload any WYSIWYG editor and have it work, which is a UX fail in and of itself; we want Drupal to ship with a default WYSIWYG editor exactly to avoid this complex choice. In other words: it's awesome that WYSIWYG API gives so much flexibility (choice) to the user, but it's simply too much flexibility. And then I have not even included/reiterated quicksketch's excellent points.
• Assuming that you can find yourself in the above, the question that remains is: is Editor module's (server- and client-side) architecture such that it is possible for WYSIWYG API to take over? And related: do we need to be able to answer that question with 100% confidence today, or is 70% confidence enough and we improve it further after commit?

A pretty long reply, but I hope it brings us closer to understanding each other better.

Replies to specific points in #86:

Not sure what you mean about a flag, but the point is that Wysiwyg supports multiple version and variants, so it must be able to accurately provide info about the actual state to other modules.

If the version is incorrect, then that's a bug in the library definition. It must be accurate. By "flag" (sorry, poorly worded) I meant something like the latest key-value pair in this array:

  $libraries['drupal.ckeditor'] = array(
    'title' => 'Drupal behavior to enable CKEditor on textareas.',
    'version' => VERSION,
    'js' => array(
      $module_path . '/js/ckeditor.js' => array(),
      array('data' => $settings, 'type' => 'setting'),
    ),
    'dependencies' => array(
      array('editor', 'drupal.editor'),
      array('ckeditor', 'ckeditor'),
    ),
   'editor library' => array('ckeditor', 'ckeditor'),
  );

If you don't want to handle versions in Editor, why require the use of the 'library' property in the @Plugin annotation?

Huh? How else would you handle it? Besides, every asset in Drupal 8 must live in a library, hence the only way to handle this is by pointing to such a library.

It forces me to register a library through hook_library_info(), which in turn requires me to use a known version of that library.

Well, yes. I don't see any problem with that. If it's a known version, then WYSIWYG API would also be able to get the metadata it needs.

Libraries we've even been discouraged from putting into Drupal VCS and instead require users to download separately.

Correct. This is still true for all non GPLv2+ (or compatible) libs. However, assuming the library is GPLv2+ (or compatible), there are two exceptions to this rule AFAIK:

1. Drupal core, which needs to ship with a WYSIWYG editor and we can't ask users to go download one separately.
2. If it's a custom build of a library (i.e. you can't download it right away), then it's okay to include it as well.

Yes, I know I can use a hook to override the plugin annotation data or the data supplied via hook_library_info(), but those hooks are useless if I don't already know which library to load so I can inspect just that one when it's needed.

I don't understand this.

Some of the meta data currently used by Wysiwyg module is being put into the annotations, ignoring the fact that having any useful data in a code comment seems like an enormous WTF in itself. But that's a personal preference and I see no way of preventing Core going further down that alley so I'm not going to push that argument. ;P

I couldn't agree more… but that's of course not something we should discuss here :)

I had a chat with TwoD today.

• I thought TwoD was talking about the WYSIWYG API module needing to double-check the version string provided by e.g. a CKEditor module. But he's really talking about just WYSIWYG API module's need to generate library definitions from user-uploaded WYSIWYG editors!
• hook_library_info(), which is where WYSIWYG API module would need to generate library definitions on the fly for user-uploaded WYSIWYG editors, is an uncached hook at the moment. Implementing #1787758: drupal_get_library() / hook_library_info() is not cached would solve that problem (TwoD agreed with this). The WYSIWYG API module could then force a rescan by clearing hook_library_info()'s cache.
  (Note that this implies that if a user replaces a WYSIWYG editor with a new version, does a rescan, then that could cause the library definition to change; but this is only logical.)
• TwoD made this (excellent) remark:
  

  Editor has an interface on the server so everyone knows what methods etc are available, but there's no real "interface" for the JavaScript files interacting with the editors. They directly call the methods on the global Drupal.editors['foo'] objects. That's also how W.M does it now, but I want to build something like Drupal.wysiwyg.getEditor('fieldID') which returns a WysiwygEditor instance.

  I of course wholeheartedly agree with this. A problem here is that we don't have any standards for JS APIs in Drupal core (AFAIK). Further, I think quicksketch's background/experience make him more apt to do improve that. (This would enable WYSIWYG API module to e.g. add event handlers for things like "editor (at|de)tached", etc.) I asked TwoD to post a modified editor.js file (even in pseudocode) so we could get a better impression of how he thinks it should work.

• WYSIWYG API module currently has insert(), setContent(), getContent() and a few other (less important) API methods. These are useful to let other JS on the page interact with the WYSIWYG editor. Does it make sense to have these in Editor module's JS API interface?

quicksketch will likely be back tomorrow; so I'm glad that we had this chat today: now he will have a pretty holistic view on what still needs to be improved after reading all the new comments in this issue :)

#1260052: Candidate WYSIWYG editors is settled, so this issue is now the primary one related to having wysiwyg editing in core. Therefore, raising to critical.

Also created #1878344: Add CKEditor JS library to core as a follow-up.

As promised in #80, I've now got a patch for #1874936: Pluggable in-place editors (allow modules to define new Create.js PropertyEditor widgets).

Consequently, the code that integrates the Editor module with the Edit module is now much, much, much cleaner! :) I didn't touch anything else. I included unit tests for the Edit + Editor integration.

To test (and now less steps, because I've already gotten some improvements committed):

1. Apply the attached Editor #93 patch.
2. Apply #1874936-4: Pluggable in-place editors (allow modules to define new Create.js PropertyEditor widgets).
3. Install http://drupal.org/project/wysiwyg_ckeditor (and configure it for the text format you want to try it on).
4. Apply #1873500-3: CKEditor + Edit.

It should look like this:

(The patch is marked as do not test because it relies on a #1874936-4: Pluggable in-place editors (allow modules to define new Create.js PropertyEditor widgets), so it could never pass.)

The patch that failed testing does not belong to this issue; I mistakenly uploaded it (and marked it to be deleted, but d.o apparently didn't listen to that).

This is a detailed (and at times nitpicky :-() review of the patch. I must say I'm pretty impressed with how straightforward this patch is. I haven't read any code yet related to Spark in the broader sense and I expected to lots of more involved things that I wouldn't understand. (The fact that I don't understand the JS in this patch is due to the fact that I generally do not understand JS. :-)) To the contrary, this patch is excellently documented and completely follows current best practices. Given the straightforwardness and simplicity, my first impulse was to warn you that this might be too simple for WYSIWYG API to properly work with it and that you should contact @sun or @TwoD, but then I find @TwoD is already working with this code. :-) Awesome²!!!

Here goes:

+++ b/core/modules/editor/editor.api.php
@@ -0,0 +1,48 @@
+ *   Array of information on editors exposed by hook_editor_info()
+ *   implementations.

Seems hook_editor_info() does not exist anymore. Also I thought for consistency "editors" -> "text editors"

+++ b/core/modules/editor/editor.api.php
@@ -0,0 +1,48 @@
+/**
+ * Perform alterations on the JavaScript settings that are added for text formats.
+ *
+ * Note that changing settings here only affects the client side behavior of the
+ * filter. To affect the filter globally both on the client side and server
+ * side, use hook_filter_info_alter().
...
+function hook_editor_js_settings_alter(&$settings, $formats) {

This doesn't mention *editors* at all, but the hook is named hook_editor_js_settings_alter(). That is strange, IMO.

+++ b/core/modules/editor/editor.module
@@ -0,0 +1,263 @@
+ * Provides bindings between Filter module text formats and client-side editors.

Again "editors" -> "text editors" and I think "Filter module" can be dropped. The concept of "text formats" is pretty unambiguous in core.

+++ b/core/modules/editor/editor.module
@@ -0,0 +1,263 @@
+  $items['admin/config/content/formats']['description'] = 'Configure the processing of text, including toolbars or WYSIWYG editors used on input, and allowed HTML tags and automatic formatting used on output.';

If I didn't miss anything, this is the only time in core we say "WYSIWYG editors". I think it would be safer to go with "text editors" here as well, as I'm not sure people get "WYSIWYG".

+++ b/core/modules/editor/editor.module
@@ -0,0 +1,263 @@
+function editor_element_info() {
+  $type['text_format'] = array(
+    '#process' => array('editor_process_format'),
+  );
+  return $type;
+}

Hmm... I would have thought this would go into a hook_element_info_alter() implementation. But I guess it doesn't hurt this way...

+++ b/core/modules/editor/editor.module
@@ -0,0 +1,263 @@
+  // Splice in the column for "Associated editor" into the header.
...
+  // Then splice in the name of each text editor for each text format.

Not necessarily a dependency for this issue, but just a heads-up that in case #1876718: Allow modules to inject columns into tables more easily gets fixed, this will get a lot less ugly.

+++ b/core/modules/editor/editor.module
@@ -0,0 +1,263 @@
+    $editor_name = $editor && isset($editors[$editor->editor]) ? $editors[$editor->editor] : drupal_placeholder(t('None'));

Can we have get a set of parens around the && condition. I trust you that this works because of operator precedence, but I don't want to have to think about that :-)

+++ b/core/modules/editor/editor.module
@@ -0,0 +1,263 @@
+function editor_form_filter_admin_format_form_alter(&$form, &$form_state) {
+  $format = $form['#format'];
+  $editor = editor_load($format->format);
...
+function editor_form_filter_admin_format_submit($form, &$form_state) {
+  $format = $form['#format'];
+  $editor = editor_load($format->format);

Seems we could just add $form['#editor'] and save the editor_load() in the submit function.

+++ b/core/modules/editor/editor.module
@@ -0,0 +1,263 @@
+  $editor_options = array('' => t('None')) + $manager->listAvailableDefinitions();

This should be done with #empty_option.

+++ b/core/modules/editor/editor.module
@@ -0,0 +1,263 @@
+    '#weight' => -9,

That deserves a comment.

+++ b/core/modules/editor/editor.module
@@ -0,0 +1,263 @@
+    $form['editor_settings'] = array_merge($form['editor_settings'], $plugin->settingsForm($form, $form_state, $editor));

This looks a bit strange. (I haven't read to EditorInterface::settingsForm() yet, so I might be wrong.) The settingsForm gets passed the entire $form, but gets stuffed into $form['editor_settings']. I think would be easier to only pass $form['editor_settings']. Then we could also drop the array merge, because it's your own fault if you override #prefix or something.

+++ b/core/modules/editor/editor.module
@@ -0,0 +1,263 @@
+function editor_load($format_id) {
+  $editors = entity_load_multiple('editor');
+  return isset($editors[$format_id]) ? $editors[$format_id] : FALSE;
+}

I think we usually just do return entity_load('editor') and be done with it. (I know I'm getting into deep nitpicky territory here, I'm sorry...)

+++ b/core/modules/editor/editor.module
@@ -0,0 +1,263 @@
+ * Additional #process callback on 'text_format' elements.

I think 'on' -> 'for' would make the sentence more fluid.

+++ b/core/modules/editor/editor.module
@@ -0,0 +1,263 @@
+  $manager = drupal_container()->get('plugin.manager.editor');
+  $definitions = $manager->getDefinitions();

I've not seen this a lot in core, but I must say I really like this pattern. It not only breaks those ridiculously long lines, but also makes clear what that weird "container" thing :-) is about, and that the actual getDefinitions() call has nothing to do with it. DX++, nice!!!

+++ b/core/modules/editor/editor.module
--- /dev/null
+++ b/core/modules/editor/js/createjs/drupalwysiwygwidget.js

This is a super weird filename. If this is some sort of standard for create.js that's totally fine, I just wanted to bring it up. Even separating the different words with dashes or whatever would help.

+++ b/core/modules/editor/js/createjs/drupalwysiwygwidget.js
@@ -0,0 +1,158 @@
+ * Text editor-based Create.js widget for processed text content in Drupal.

I have absolutely no idea what that means. :-) That in itself is not in any way concerning I just hope people who know what Create.js is, understand this. :-)

+++ b/core/modules/editor/js/createjs/drupalwysiwygwidget.js
@@ -0,0 +1,158 @@
+      var that = this;

Sorry to spam this otherwise serious review, but this line is just *awesome*!!! JS++ (Disclaimer: I am a total JS-noob and have no idea whattheheck is going on at all in this file, but I literally laughed out loud reading this.)

+++ b/core/modules/editor/lib/Drupal/editor/EditorBundle.php
@@ -0,0 +1,27 @@
+ * Editor dependency injection container.
+ */
+class EditorBundle extends Bundle {

I'm pretty sure that's incorrect. I guess "Bundle for Editor module." should suffice.

+++ b/core/modules/editor/lib/Drupal/editor/Plugin/Core/Entity/Editor.php
@@ -0,0 +1,80 @@
+ * Definition of \Drupal\editor\Plugin\Core\Entity\Editor.

+++ b/core/modules/editor/lib/Drupal/editor/Plugin/EditorInterface.php
@@ -0,0 +1,75 @@
+ * Definition of \Drupal\editor\Plugin\EditorInterface.

+++ b/core/modules/editor/lib/Drupal/editor/Plugin/EditorManager.php
@@ -0,0 +1,42 @@
+ * Definition of \Drupal\editor\Plugin\EditorManager.

+++ b/core/modules/editor/lib/Drupal/editor/Plugin/edit/editor/Editor.php
@@ -0,0 +1,97 @@
+ * Definition of \Drupal\editor\Plugin\edit\editor\Editor.

+++ b/core/modules/editor/lib/Drupal/editor/Tests/EditIntegrationTest.php
@@ -0,0 +1,178 @@
+ * Definition of Drupal\editor\Tests\EditorIntegrationTest.

+++ b/core/modules/editor/tests/modules/lib/Drupal/editor_test/Plugin/editor/editor/Unicorn.php
@@ -0,0 +1,58 @@
+ * Definition of Drupal\editor_test\Plugin\editor\editor\Unicorn.

Only if you're already re-rolling, these should be "Contains \Drupal\..." i.e. start the namespace with a backslash. One file already does this correctly in this patch.

+++ b/core/modules/editor/lib/Drupal/editor/Plugin/Core/Entity/Editor.php
@@ -0,0 +1,80 @@
+ *   entity_keys = {
...
+ *     "editor" = "editor",

I think entity_keys is just for generic base properties (id, label, etc.), so editor can be removed here I think.

+++ b/core/modules/editor/lib/Drupal/editor/Plugin/Core/Entity/Editor.php
@@ -0,0 +1,80 @@
+   * The text format to which this text editor is bound.
+   *
+   * @var string
+   */
+  public $format;

This is ambiguous because filter.module weirdly does $format->format instead of $format->name for its machine name (i.e. that's totally not your fault). Therefore this should say "The machine name of the text format ..." for clarity.

+++ b/core/modules/editor/lib/Drupal/editor/Plugin/Core/Entity/Editor.php
@@ -0,0 +1,80 @@
+   * The label of the text format to which this text editor is bound.
+   *
+   * @var string
+   */
+  public $name;

I don't really have a suggestion on what would be a better label, but it does seem weird to duplicate the formats label here. I guess we could just kill this and then in Editor::label() do entity_load($this->format)->label(). Even though I would prefer that, it still doesn't really "feel clean", so not totally married to that idea.

+++ b/core/modules/editor/lib/Drupal/editor/Plugin/Core/Entity/Editor.php
@@ -0,0 +1,80 @@
+  /**
+   * Overrides Drupal\Core\Entity\Entity::id().
+   */
+  public function id() {
+    return $this->format;
+  }

Again, no suggestion on how to improve this, but this feels weird. This makes sure the id() is unique among a single entity type, but the fact that entity_load('format', entity_load(editor', $id)->id()) works is unique to these two entity types. (I know formats aren't (yet) true entities, but you get the idea...)

+++ b/core/modules/editor/lib/Drupal/editor/Plugin/Core/Entity/Editor.php
@@ -0,0 +1,80 @@
+    $this->settings = array_merge($plugin->defaultSettings(), $this->settings);

Not 100% sure on this one, but it might make sense to utilize NestedArray::mergeRecursive() here.

--- /dev/null
+++ b/core/modules/editor/lib/Drupal/editor/Plugin/EditorInterface.php

--- /dev/null
+++ b/core/modules/editor/lib/Drupal/editor/Plugin/EditorManager.php

Not really sure why these are in the "Plugin" namespace. Totally minor point, though...

+++ b/core/modules/editor/lib/Drupal/editor/Plugin/EditorInterface.php
@@ -0,0 +1,75 @@
+   * Note that this form will be a subform of filter_admin_format_form(). Hence
+   * the Filter module will take care of saving the settings.

I think there needs to be a comma after "Hence". Also, I think editor.module actually takes care of that in its submit handler, so this is not really true. I think the second sentence can just be omitted.

+++ b/core/modules/editor/lib/Drupal/editor/Plugin/EditorInterface.php
@@ -0,0 +1,75 @@
+   * If the editor's behavior depends on an extensive list and/or external data,

Maybe "an extensive list of options" or something. I think "an extensive list" on its own is unclear (at least to me :-P).

+++ b/core/modules/editor/lib/Drupal/editor/Plugin/EditorInterface.php
@@ -0,0 +1,75 @@
+   * @param array $form
+   *   The prepopulated form array of the filter administration form.
...
+  function settingsForm(array &$form, array &$form_state, Editor $editor);

This relates to one of the comments above. But why is $form passed by reference? I guess that's a feature that allows each editor's settings form to alter the entire filter admin form, but I'm not sure we want to allow that.
If we keep it, though, "passed by reference" should be added to the parameter documentation.

+++ b/core/modules/editor/lib/Drupal/editor/Plugin/EditorInterface.php
@@ -0,0 +1,75 @@
+  function generateJsSettings(Editor $editor);

Generally we don't abbreviate function/method names, I can see that "generateJavaScriptSettings()" would be a bit verbose, though, so I'm not going to "formally request" that :-). Per our current standards it should be generateJSSettings(), though.

+++ b/core/modules/editor/lib/Drupal/editor/Plugin/EditorManager.php
@@ -0,0 +1,42 @@
+
+  /**
+   * List all available plugin definitions.
+   *
+   * @return array
+   *   An array of available definition titles, keyed by id.
+   */
+  public function listAvailableDefinitions() {
+    return array_reduce($this->getDefinitions(), function($result, $definition) {
+      return $result + array($definition['id'] => $definition['title']);
+    }, array());
+  }

Awesome! I like this so much I'm grepping my brain right now for a base class to put this in. Really, really nice.

+++ b/core/modules/editor/lib/Drupal/editor/Plugin/EditorManager.php
--- /dev/null
+++ b/core/modules/editor/lib/Drupal/editor/Plugin/edit/editor/Editor.php

+++ b/core/modules/editor/lib/Drupal/editor/Plugin/edit/editor/Editor.php
@@ -0,0 +1,97 @@
+/**
+ * Defines the "editor" Create.js PropertyEditor widget.
...
+class Editor extends PluginBase implements EditorInterface {

It seems this is an actual plugin, whereas I would have expected this to be some sort of base class for "real" editors (i.e. CKEditor extends Editor). Also what does a PHP class have to do with Create.js. This might be totally legit, but some docs for this would be cool.

Oh, wait I just saw this has something to do with edit.module not editor.module. But that is not actually contained in this patch. :-( That's also why the contents of this file make little sense to me.

+++ b/core/modules/editor/lib/Drupal/editor/Tests/EditIntegrationTest.php
@@ -0,0 +1,178 @@
+use Drupal\edit\EditorSelector;
+use Drupal\edit\MetadataGenerator;
+use Drupal\edit\Plugin\EditorManager;
+use Drupal\edit\Tests\EditTestBase;
+use Drupal\edit_test\MockEditEntityFieldAccessCheck;

See the above. (This again is related to edit.module.)

+++ b/core/modules/editor/lib/Drupal/editor/Tests/EditIntegrationTest.php
--- /dev/null
+++ b/core/modules/editor/tests/modules/editor_test.info

Please either rename the "modules" directory directly to "editor_test" (preferred, personally) or add an additional subdirectory named "editor_test".

+++ b/core/modules/editor/tests/modules/lib/Drupal/editor_test/Plugin/editor/editor/Unicorn.php
@@ -0,0 +1,58 @@
+class Unicorn extends PluginBase implements EditorInterface {

A tip of the hat to you, if you actually manage to get unicorns in Drupal core. :-)

Edit: Fixed small Dreditor weirdness.

Woah, thanks, tstoeckler! :)

@quicksketch's EDIT: that's right :)

This is intentional because it is assumed that a very limited number of text editors will be installed on a site, and most likely if there are multiple, all of them will be loaded if one of them is (such as an administrator having access to 2 different WYSIWYGs based on two text formats). Using a multiple loader should be slightly more efficient than loading individual editors.

Okay, then that definitely deserves a comment. In my initial review I totally missed that you're not passing array($id) to entity_load_multiple().

And one more change: EditorInterface having a settingsFormValidate() but no settingsFormSubmit() method seemed incomplete. We should have both methods for the greatest amount of flexibility.

Wim probably has some more thoughts on this, but anyway: I think the point of the missing submit method was that submit functions generally save some data somewhere (in the DB, generally). As a plugin you should have no knowledge about any sort of storage nor should you implement any on your own. Consequently you should have no business "submitting" anything. In the empty submit method in EditorBase you note that this can be used to massage the form values before editor.module goes and saves them. This is usually something we do in validate functions, because there you definitely know that your code is running before any submit handler. (The way it currently is, the editor-specific submit function gets added *after* editor.module's, so if we leave this as is, that's actually a bug, if I'm not mistaken.) Now I realize that doing two things (validating the actual values and adapting their structure) in the validation handler is not super clean API-wise, but that's what we're doing all over core, and fixing correctly would probably involve introducing a third, post-validate, pre-submit phase for all forms. Now, I don't think it's the end of the world to have that submit function in the interface, I just wanted to point out I actually quite liked the differentiation in the previous patch.

I also found a few things I missed in the last round/some things in the new patch:

+++ b/core/modules/editor/editor.api.php
@@ -0,0 +1,43 @@
+/**
+ * Perform alterations on text editor definitions.
+ *
+ * @param array $editors
+ *   An array of information on text editors, as collected by the annotation
+ *   discovery mechanism.
+ */
+function hook_editor_info_alter(array &$editors) {

This should have some sort of @see to where I can look-up the structure of each $editor.
Specifically, it should be noted that the 'library' part of the annotation should be some library declared elsewhere that has a dependency on 'drupal.editor'. That, I think, is crucial information and is what makes this patch so valuable.
Also, I think it must be noted that these are not "Editor entities", "Editor plugins". I tried to grasp the relation of the two from the patch, but failed, and I think this will a great source of confusion lacking any documentation.

+++ b/core/modules/editor/editor.info
--- /dev/null
+++ b/core/modules/editor/editor.module

This needs an implementation of hook_help().

+++ b/core/modules/editor/editor.module
@@ -0,0 +1,258 @@
+  // If there aren't any options (other than "None") disable the select list.
+  if (count($editor_options) === 0) {

Very nitpick-y again, but I think empty() is more readable here.

+++ b/core/modules/editor/editor.module
@@ -0,0 +1,258 @@
+  $element['#attached'] = array_merge_recursive($element['#attached'], editor_get_attachments($formats));

I seem to have missed this on the last review: array_merge_recursive was only recently successfully banned from core. Please use NestedArray::mergeRecursive() for that.

+++ b/core/modules/editor/editor.module
@@ -0,0 +1,258 @@
+      'editorSettings' => $plugin->generateJsSettings($editor),

I think this actually works because PHP is being a weirdo again, but this should be generateJSSettings() now (capital JS)

+++ b/core/modules/editor/js/editor.js
@@ -0,0 +1,80 @@
+ * Initialize an empty object where editors where place their attachment code.

Duplicate "where".

+++ b/core/modules/editor/lib/Drupal/editor/EditorBundle.php
@@ -0,0 +1,27 @@
+ * Editor dependency injection container.

This is still incorrect.

+++ b/core/modules/editor/lib/Drupal/editor/Plugin/EditorBase.php
@@ -0,0 +1,53 @@
+
+/**
+ * Defines a base editor class from which other modules may extend.
+ */
+abstract class EditorBase extends PluginBase implements EditorInterface {

At least a little note saying "Editors without any settings can use this class without overriding anything" (with better words :-)) would be nice, I think.

1. I would go simply with "Bundle for Editor module." because that's precisely what it is. In theory this should start with a third-person verb but I could not find any sane way to do this. Sorry, that I obsess with this but the current wording is simply incorrect. What this class does is is provides a number of services to Drupal's central dependency injection container. It is not a dependency injection container in itself. This is rather important because the whole point of the dependency injection container is that there is only one. Symfony and by extension Drupal calls these things that provide services "bundles" and specifically in Drupal we have a hard-coded 1-to-1 relationship between module and bundle.

2. Yes, I realize this is a general problem and committing this without any specific docs would (sadly) be consistent with a lot of other subsystems. Therefore, I do not want to block anything because of this. What I have done when I ported some own modules to D8 so far is documented the structure of the annotation on the interface that the plugins must implement, because that is where plugin authors must look anyway. Because, as noted above, I do not grok the difference between "Editor entities" and "Editor plugins" in this patch, I'm not actually sure if that would be EditorInterface, but I think it is. Again, if you simply want to punt this, I won't object to that.

The editor.module submit handler is definitely added second, so the editor-specific submit handler can run first.

Oops, sorry. You are, of course, correct.

And I can only return the thanks to you for writing such a clean and straightforward patch!

Wow, I totally wasn't aware of that. I'm still 98% sure that that description is bogus, but given your numbers I guess it makes sense to simply punt on that.

I totally agree. Let's pretend I never said anything. :-) I'll comment over there.

I would RTBC this, but, even though we generally don't have the notion of veto power, I'm pretty sure @sun would like to take a last look at this for the filter module implications. Maybe one of the issue queue masters can assign this to him.

#100:

Please add interdiffs next time, that makes reviewing significantly easier.

It looks like Edit module/CreateJS support was accidentally(?) combined into the last patch. Can we separate these two (as I've done here) and focus on adding editor.module? I don't know enough about CreateJS to adequately review the (strangely named) drupalwysiwygwidget.js file.

It was added intentionally, to indicate that the Editor module has been proven to take both back-end (form/iframe) and front-end (contentEditable) editing in mind. And to prove that it's already working.

I think it's okay to add Edit support in a follow-up (though I would prefer it's done in this issue because there are some design changes necessary (see #80).

AFAICT you didn't separate the two, but just removed the Edit support from editor.module — or forgot to attach the separate patch?

On Edit module support can we rename $editor_info['supports_true_wysiwyg'] to $editor['inline_editor_support']? I don't know what "true wysiwyg" means, or what toggling that option would change. I suppose this should probably be a separate discussion.

This is one of the design changes needed for the above. Not every text (or WYSIWYG) editor supports front-end (contentEditable) editing. This is the flag that indicates whether that is supported or not. I don't really care what it's called. Maybe $editor['supportsContentEditable'] would be best. This belongs in the same issue where Editor adds Edit support (again, see the above).

#101:
RE: settingsFormSubmit(): tstoeckler hit the nail on the head regarding why I didn't add this. I don't have anything to add to his explanation.

#114: I'm glad to see you found this patch to have so few remaining issues! :) I don't think is fully ready yet though (see below).

So what does still need to happen here? Things I can think of:

• Tests.
• Auto-update filters (specifically the allowed HTML tags) when the text editor's (toolbar) configuration changes and vice versa. Or is that blocked on #1834682: Consolidate filter options in the UI when configuring a format in your mind, quicksketch?
• Documenting the JS API interface (which a text editor module must implement to provide the glue between Editor module's JS and the text editor's JS). Unfortunately, nothing like this exists in Drupal core yet. (See #90, bullet 3.)
• Is the JS API sufficiently complete? (See #90, bullet 4.)
• … anything else?

1. Interdiff from #93 (by me) to #112 (by quicksketch). Should facilitate other reviews.
2. A reroll of the patch in #112, with only very minor changes.
3. A regular interdiff for changes I made in this reroll.
4. A rerolled patch of the Edit support added to Editor as introduced in #93. It is now relative to the attached Editor patch as well as relative to #1874936-10: Pluggable in-place editors (allow modules to define new Create.js PropertyEditor widgets).

Next, I'll be working on tests. I'll post what I have tonight, then quicksketch can continue (with tests or other things) while I sleep.

I agree/am okay with everything you said in #116 :)

In one hour, I'll post the tests that I have, then hopefully you can continue from there (hopefully you'll only have to document the JS methods).

+++ b/core/modules/editor/lib/Drupal/editor/Plugin/EditorBase.php
@@ -0,0 +1,83 @@
+ *   - name: The name of the library to be added, as defined in
+ *       hook_library_info().

Minor nitpick: Intendation is off in the second line.

#118: It's indented analogously to the preceding lines/higher levels of this nested structure; so can you specify what is wrong exactly?

+++ b/core/modules/editor/lib/Drupal/editor/Plugin/EditorBase.php
@@ -0,0 +1,83 @@
+ * - id: The unique, system-wide identifier of the text editor. Typically named
+ *     the same as the editor library.
+ * - title: The human-readable name of the text editor, translated.
+ * - library: A nested array containing key-value pairs for adding library to
+ *     the page, as done through drupal_add_library(). This array should have
+ *     two keys:
+ *   - module: The name of the module providing the editor library.
+ *   - name: The name of the library to be added, as defined in
+ *       hook_library_info().

Right, totally missed those other lines, weirdly. Those are wrong, as well. I.e. "the same as...", "the page, as ..." "two keys." and "hook_library_info()." should all be indented two spaces less. That way everything aligns on the left.

Incorporated feedback in #120 — even though it decreases legibility/scanability. It's up to quicksketch to argue for the way he did it.

• I had to move editor.module/editor_get_attachments() to EditorManager.php/EditorManager::getAttachments() to make it cleanly testable. (I also suggested this in #80, which would be in line with #1875086-7: Improve DX of drupal_container()->get(), but I never got a response.) Related to this, it now accepts format IDs, not format objects.
• EditorManagerTest: Unit tests (DrupalUnitTestBase) for EditorManager. Takes 1 second on my system. Related to this: much like #1875098: Edit's processed text editor plugins bleed into test runs, we have to use ProcessDecorator to fix a temporary flaw in SimpleTest+Plugin system in D8.
• EditorAdminTest: No tests (WebTestBase) yet, since the form is JS-toggled, and I'm not sure what the best practices around testing this are. IIRC quicksketch has more experience with this wrt Poll module.
• EditorLoadingTest: A very simple test (WebTestBase and the 'testing' install profile), basically to ensure that editor_element_info() and editor_process_format() work correctly, since all the hard stuff is already happening in EditorManager and is thus already covered by the earlier test. Takes ±20 seconds on my system.

I'd much prefer to only have EditorManagerTest, but I doubt that's acceptable.

Nice work on the tests! I had totally forgotten about that, silly me...

This patch seems unlikely to cause:

Test name	Pass	Fail	Exception
Drupal\path\Tests\PathAliasTest	135	0	1
Message	Group	Filename	Line	Function	Status
Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[42S02]: Base table or view not found: 1146 Table 'drupaltestbotmysql.simpletest844643cache_bootstrap' doesn't exist: DELETE FROM {cache_bootstrap} WHERE (cid = :db_condition_placeholder_0) ; Array ( [:db_condition_placeholder_0] => variables ) in Drupal\Core\Database\Connection->query() (line 554 of /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Database/Connection.php).
Drupal\Core\Database\Connection->query('DELETE FROM {cache_bootstrap} 
WHERE  (cid = :db_condition_placeholder_0) ', Array, Array)
Drupal\Core\Database\Query\Delete->execute()
Drupal\Core\Cache\DatabaseBackend->delete('variables')
Drupal\simpletest\WebTestBase->refreshVariables()
Drupal\simpletest\WebTestBase->drupalPost('node/2/edit', Array, 'Save')
Drupal\path\Tests\PathAliasTest->testDuplicateNodeAlias()
Drupal\simpletest\TestBase->run()
simpletest_script_run_one_test('401', 'Drupal\path\Tests\PathAliasTest')
Uncaught exception	Connection.php	554	Drupal\Core\Database\Connection->query()	

Drupal\views\Tests\Plugin\DisplayTest	39	1	0
Message	Group	Filename	Line	Function	Status
The test did not complete due to a fatal error.	Completion check	DisplayTest.php	133	Drupal\views\Tests\Plugin\DisplayTest->testGetAttachedDisplays()	

...so sending it back for a re-test.

#121: editor_module-1833716-121.patch queued for re-testing.

@quicksketch: could you document the JS methods? If you could add the EditorAdminTest, that'd be great too of course :)

Thanks for moving this forward!

What follows is an extensive and in-depth review of the proposed code, focusing on technical aspects, usability aspects, but also extensibility aspects. The review is rather large, even though the total size of this patch isn't that big. Next to a few smaller details, I do think that we're missing some crucial code logic in various places here, and I did not understand why the essential logic was left out of the proposed code, given that a good chunk of the code literally moves the base building blocks of Wysiwyg module into core and given that the non-obvious details and business logic of those parts of the contrib module are actually well documented.

However, this is not supposed to discourage anyone. The majority of issues should be quickly addressable. For the other issues, there's at least concrete and documented code in contrib to serve as inspiration and potentially even copy/paste template. As I will note below, I rather strongly believe that we want to address these issues before committing this patch, since it is kind of guaranteed that we'll replay Wysiwyg module's history in the Drupal core queue otherwise.

In any case, I think that all issues should be addressable within a few days only, as the entirety of the proposed changes here aren't really rocket science, so it's mainly about accounting for pre-existing knowledge, experience, and pitfalls, so as to not fill up the core queue with bug reports right after committing this. ;)

I also have the impression that we're lacking tests for a lot of the contained and expected functionality. The tests being added in this patch are covering very very basic aspects only, but none of the more advanced business logic, and they also do not verify expectations for any edge-cases; i.e., unexpected scenarios. Now that the majority of the code already exists and has been manually tested to some extent, it will be very hard to come up with a concrete and detailed test plan — normally, you'd do that in real-time while writing the actual implementation code (which commonly involves considerations along the lines of "What if...? Ugh, yeah, let's plant a condition here...") - not necessarily as concrete test code, but at least in the form of a (giant) list of @todos in a test class. As mentioned before, retroactively adding tests is a recipe for incompleteness/bugs, but that ship has sailed already, so all we can really do here is to give our best to complete the tests, and/or if desired/necessary, also just implant a range of @todos for concrete, yet-untested expectations into test classes. In any case, better than nothing.

Thanks again for your hard work on this. Here we go:

+++ b/core/modules/editor/editor.info
@@ -0,0 +1,7 @@
+description = "Provides a framework for associating text formats with text editor libraries such as WYSIWYGs or toolbars."

"Allows to associate text formats with text editor libraries."

+++ b/core/modules/editor/editor.module
@@ -0,0 +1,242 @@
+  $items['admin/config/content/formats']['description'] = 'Configure the processing of text, including toolbars or WYSIWYG editors used on input, and allowed HTML tags and automatic formatting used on output.';

Here and elsewhere:

User interface texts are constantly referring to "toolbars and WYSIWYG editors", which IMHO has a too pedantic focus on the technical, nitty-gritty detail and difference that the majority of users doesn't make.

Likewise, phrases like "processing of text" do not present any meaning to the average user. And WYSIWYG on its own is an acronym that has plenty of different interpretations and expectations.

We need to remove all the technical drama and details and focus on the common user tasks instead, using terminology that everyone is able to understand; e.g.:

"Configure how user-contributed content is filtered and formatted, as well as the text editor user interface."

+++ b/core/modules/editor/editor.module
@@ -0,0 +1,242 @@
+      $path . '/js/editor.js' => array('group' => JS_LIBRARY),

JS_LIBRARY is the default group, so I do not understand why it is specified manually here.

(That being the default is a bug on its own though, see #1805452: [meta] Asset library declarations, registry, usage, DX, compatibility, documentation)

+++ b/core/modules/editor/editor.module
@@ -0,0 +1,242 @@
+  $start = array_splice($form['formats']['#header'], 0, $position, array('editor' => t('Associated editor')));

s/Associated editor/Text editor/

+++ b/core/modules/editor/editor.module
@@ -0,0 +1,242 @@
+    $editor_name = ($editor && isset($editors[$editor->editor])) ? $editors[$editor->editor]['title'] : drupal_placeholder(t('None'));

"—" would be more appropriate + scanable within a table (instead of "None").

+++ b/core/modules/editor/editor.module
@@ -0,0 +1,242 @@
+      '#type' => 'markup',

Obsolete. Just #markup is sufficient.

+++ b/core/modules/editor/editor.module
@@ -0,0 +1,242 @@
+function editor_form_filter_admin_format_form_alter(&$form, &$form_state) {
...
+  $editor = editor_load($format->format);
+  $manager = drupal_container()->get('plugin.manager.editor');
...
+      $editor = entity_create('editor', array(

The associated editor + manager needs to be available for others in $form_state; i.e.:

$form_state['editor'] = $editor = editor_load($format->format);
$form_state['editor_manager'] = $manager = ...;

+++ b/core/modules/editor/editor.module
@@ -0,0 +1,242 @@
+  // Check if we're switching to a different text editor and load that form.
+  if (isset($form_state['values']['editor'])) {
...
+function editor_form_filter_admin_form_ajax($form, &$form_state) {
+  return $form['editor_settings'];
+}

By moving 'editor' into $form_state, this massaging can be moved into the #ajax callback, which essentially replaces the object in $form_state with a new instance only.

+++ b/core/modules/editor/editor.module
@@ -0,0 +1,242 @@
+    // Position the editor selection before the filter settings (weight of 0),
+    // but after the filter label and name (weight of -20).
+    '#weight' => -9,

Why -9? Let's use -10 here? [That said, see also my other remark (far) below.]

+++ b/core/modules/editor/editor.module
@@ -0,0 +1,242 @@
+  // If there aren't any options (other than "None") disable the select list.
+  if (empty($editor_options)) {
+    $form['editor']['#disabled'] = TRUE;
+    $form['editor']['#description'] = t('This option is disabled because no modules that provide an editor are currently enabled.');
+  }

Not sure I get the need for this exposure — I'd prefer to simply set #access to FALSE in that case.

+++ b/core/modules/editor/editor.module
@@ -0,0 +1,242 @@
+    '#type' => 'group',
+    '#prefix' => '<div id="editor-settings-wrapper">',
+    '#suffix' => '</div>',

This #type does not exist.

Let's use #type 'container' + #id instead of this custom #prefix + #suffix here.

+++ b/core/modules/editor/editor.module
@@ -0,0 +1,242 @@
+    $form['editor_settings'] = array_merge($form['editor_settings'], $plugin->settingsForm($form, $form_state, $editor));

It is generally not a good idea to merge sub-form-definitions/sections into an existing parent element. Let's add the plugin-specific settings into a separate sub-key 'editor_settings']['settings', so they are cleanly detached from the wrapper element.

+++ b/core/modules/editor/editor.module
@@ -0,0 +1,242 @@
+    // Place validation first since editor validation errors should appear above
+    // individual filter validation errors.
+    array_unshift($form['#validate'], array($plugin, 'settingsFormValidate'));

1) This comment lacks technical reasoning. Unless there is one, let's just append it.

2) This should not use #validate in the first place. We need to use #element_validate on the aforementioned 'settings' sub-key instead. (The remainder of the form is still accessible with #element_validate for advanced use-cases. There's no reason to not use it.)

+++ b/core/modules/editor/editor.module
@@ -0,0 +1,242 @@
+function editor_form_filter_admin_format_submit($form, &$form_state) {
...
+  // Create a new editor or update the existing editor.
+  if ($form_state['values']['editor']) {
+    if (!$editor) {
+      $editor = entity_create('editor', array(

Continuing previous mentions of $form_state['editor'] benefits... the submit handler operations here scale down to a simple:

$form_state['editor']->settings = $form_state['values']['editor_settings']['settings'];
$form_state['editor']->save();

Alas — it should be duly noted that this entire code here actually translated into fieldable filter_format config entities ;) (A config entity attached to another config entity.)

+++ b/core/modules/editor/editor.module
@@ -0,0 +1,242 @@
+function editor_load($format_id) {
+  // Load all the editors at once here, assuming that either no editors or more
+  // than one editor will be needed on a page (such as having multiple text
+  // formats for administrators). Loading a small number of editors all at once
+  // is more efficient than loading multiple editors individually.
+  $editors = entity_load_multiple('editor');

I can certainly make sense of this, but are there actual performance benchmarks, which prove that this is actually valid and not premature optimization?

+++ b/core/modules/editor/editor.module
@@ -0,0 +1,242 @@
+function editor_process_format($element) {
...
+  $format_ids = array_keys(filter_formats($user));

The following remarks essentially repeat my remarks about editor.js vs. wysiwyg.js — I certainly know very well how tempting it is to "simplify a crappy contrib module's implementation" and occasionally, I'm surprised by how much more simple certain things can be accomplished with the brilliance + manpower of the Drupal core community. :)

However, the proposed logic and implementation here and elsewhere in this proposed patch is incomplete and we will not do ourselves a favor by ignoring Wysiwyg module's history and implementation.

Reality is, Wysiwyg's code looked similarly trivial in the beginning, ~5 years ago. What followed was a rain of bug reports for a tonne of different scenarios, and that's why the respective code looks like it looks today. All of such code is normally guaranteed to have an inline comment explaining the situation in Wysiwyg's code base.

Nothing in Filter module has significantly changed since D7, so I have to ask why we're purposively ignoring the well documented knowledge of Wysiwyg module in some major/fundamental aspects of this patch?

With regard to this concrete #process callback, please have a look at:

http://drupalcode.org/project/wysiwyg.git/blob/refs/heads/7.x-2.x:/wysiw...

1) #process is wrong/unnecessary (it was a #process in Wysiwyg a few years ago, too) -- this is about (page/form) rendering, so #pre_render is the correct and appropriate tool.

2) filter_formats() is insufficient/invalid, since contributed modules may alter the list of available text formats on a per-field basis. The formats to process need to be extracted from the available formats in the element only.

3) Contrib and custom sites need a #wysiwyg = FALSE property to force opt-out of the text editor handling. [Which originally was a hidden thing of Wysiwyg module and got removed since we had the impression that no one needs it, only to find out later that all other editor integration modules in contrib adopted the #wysiwyg property, so it was re-implemented into Wysiwyg afterwards. :P] We may consider to rename it into #editor for D8 (even though contrib adopted #wysiwyg already, so not sure about the rename, but quite potentially it's appropriate/required to rename it, even if for namespace purposes only).

4) We need to inject a hidden form input element in order to attach an editor to a format-enabled form element that does not have a text format selector. Alternatively, we have to significantly change how filter_process_format() works (but that should happen in a separate issue then, please).

5) The server-side code needs to make sure to annotate the actual input/textarea element that should be targeted by the frontend/client-side code.

6) Special handling is required in case the associated text format is not accessible for the current user - which means that Filter module will replace the form element's content with a user-facing notice that the element is not editable.

7) There was a relatively brilliant concept around "triggers" in Wysiwyg's code, which I apparently can't fully remember + describe right now (it is getting late over here), but I know that it was refactored and added later on only, and also wasn't complete yet. Roughly speaking, the JS settings required for each editor/format/field combination are pretty huge (» frontend performance), so the first step is to detach the format from the field, so format-level settings apply to all fields, but individual fields are still able to override format-level settings. Followed by a second step of detaching formats from [global] editor settings (IIRC, not implemented in W. yet), which ultimately compresses the entire, huge swath of JS settings down to global editor » format » field overrides.

+++ b/core/modules/editor/editor.module
@@ -0,0 +1,242 @@
+  if (!isset($element['#attached'])) {
+    $element['#attached'] = array();
+  }

I'm not able to see under which circumstances #attached wouldn't be defined + an array?

+++ b/core/modules/editor/js/editor.js
@@ -0,0 +1,80 @@
+Drupal.behaviors.editors = {

1) In general, this almost looks like a literal copy of wysiwyg.js. However, substantial parts are missing. And since Filter module did not change in significant ways since D7, this copy looks too simple and I've the impression that we'll re-run Wysiwyg's bug report history in core.

2) Why is this plural (editors) and not singular (editor)? The plural apparently repeats itself in .once().

+++ b/core/modules/editor/js/editor.js
@@ -0,0 +1,80 @@
+  attach: function (context, settings) {
...
+    if (!settings.editor) {
...
+        if (drupalSettings.editor.formats[activeEditor]) {

I do not understand why half of this code is based on settings.editor and the other half is based on drupalSettings.editor. Unless something has dramatically changed in our Drupal.behaviors API, then the entire code should work off settings.editor.

+++ b/core/modules/editor/js/editor.js
@@ -0,0 +1,80 @@
+    $context.find('.filter-list:input').once('editors', function () {
...
+      // Directly attach this editor, if the text format is enabled or there is
+      // only one text format at all.
+      if ($this.is(':input')) {

1) How can $this.is(':input') ever not be true with the enclosing .find('...:input')?

2) Contrary to the comment, this code doesn't actually do something in case there is no text format selector, because .filter-list doesn't exist in the first place - e.g., for anonymous users that only have access to a single format.

Again, this circles back into my earlier remark — a lot of code from wysiwyg.js is missing here, even though the contained logic in there is fairly basic only. By ignoring it, we will replay Wysiwyg module's bug report history in the core queue and we'll solve the exact same problems from scratch that have been fixed before already.

+++ b/core/modules/editor/js/editor.js
@@ -0,0 +1,80 @@
+      var field = $this.closest('.text-format-wrapper').find('textarea').get(-1);

CKEditor actually supports to replace textfields in addition to textareas. In general, the discovery/selector being used here is prone to errors - we need to annotate the input field/element that needs replacement on the server-side already.

+++ b/core/modules/editor/js/editor.js
@@ -0,0 +1,80 @@
+          // Prevent double-binding if the change event is triggered manually.
+          if ($this.val() === activeEditor) {
+            return;
+          }
...
+          activeEditor = $this.val();

To clarify the relationship and actually prevent duplicate events from being fired, let's move the activeEditor reassignment right below the initial condition.

+++ b/core/modules/editor/js/editor.js
@@ -0,0 +1,80 @@
+      // Detach any editor when the containing form is submitted.

Drupal.behaviors.editor is missing a .detach method that properly responds to possible triggers.

We need to save the editor's active contents back into the textarea e.g. when submitting the form via Ajax.

+++ b/core/modules/editor/js/editor.js
@@ -0,0 +1,80 @@
+      $this.parents('form').submit(function (event) {
+        // Do not detach if the event was canceled.
+        if (event.isDefaultPrevented()) {
+          return;
+        }
+        Drupal.editorDetach(field, drupalSettings.editor.formats[activeEditor]);
+      });

Most editors actually do this by default already, and the custom/double attachment of this event always bugged me in Wysiwyg module's design.

1) We should double-check whether this is actually necessary for core.

2) In any case, the code should be moved into a separate method, so the handler can be bound to the form by other code.

+++ b/core/modules/editor/js/editor.js
@@ -0,0 +1,80 @@
+Drupal.editorAttach = function (field, format) {
+  if (format.editor) {
+    Drupal.editors[format.editor].attach(field, format);
+  }
+};
+
+Drupal.editorDetach = function (field, format) {
+  if (format.editor) {
+    Drupal.editors[format.editor].detach(field, format);
+  }
+};

Having these as top-level methods on the Drupal object doesn't really make sense.

We should introduce a proper Drupal.editor object that supplies proper methods:

Drupal.editor.instances = {}
Drupal.editor.attach()
Drupal.editor.detach()

In other words, Drupal.editor is the front-end Editor Plugin Manager.

+++ b/core/modules/editor/lib/Drupal/editor/Plugin/Core/Entity/Editor.php
@@ -0,0 +1,79 @@
+  public $format;
...
+  public function id() {
+    return $this->format;
+  }

Good! The new FilterFormat in D8 will actually use $id instead of $format as entity ID property. However, for Editor entities, the entity ID itself is actually a foreign key, so choosing $format as its ID looks right to me.

+++ b/core/modules/editor/lib/Drupal/editor/Plugin/Core/Entity/Editor.php
@@ -0,0 +1,79 @@
+  /**
+   * The label of the text format to which this text editor is bound.
+   *
+   * @var string
+   */
+  public $name;

This data should not be duplicated into Editor entities. It will become stale.

Instead, we probably want to override the label() method and dynamically fetch the label of the referenced text format ID.

+++ b/core/modules/editor/lib/Drupal/editor/Plugin/Core/Entity/Editor.php
@@ -0,0 +1,79 @@
+  /**
+   * The name of the text editor library.
+   *
+   * @var string
+   */
+  public $editor;

Can we rename this to $library?

An Editor entity with an $editor property looks like infinite recursion to me.

+++ b/core/modules/editor/lib/Drupal/editor/Plugin/Core/Entity/Editor.php
@@ -0,0 +1,79 @@
+  public function __construct(array $values, $entity_type) {
+    parent::__construct($values, $entity_type);
+
+    // Initialize settings to the defaults, merging any passed in settings.
+    $manager = drupal_container()->get('plugin.manager.editor');
+    $plugin = $manager->createInstance($this->editor);
+    $this->settings = array_merge($plugin->defaultSettings(), $this->settings);

1) We can skip the array_merge() by initializing default settings before calling parent::__construct().

2) There's no protection against an empty/unset $editor property.

3) In general though, this pattern looks odd to me. If I create a new Editor entity, then I actually expect that to be empty (or to contain the most basic defaults only).

+++ b/core/modules/editor/lib/Drupal/editor/Plugin/EditorBase.php
@@ -0,0 +1,83 @@
+ * - title: The human-readable name of the text editor, translated.
...
+ *   title = @Translation("My Editor"),

'title' should be 'label'.

+++ b/core/modules/editor/lib/Drupal/editor/Plugin/EditorBase.php
@@ -0,0 +1,83 @@
+ *   library = {
+ *     "module" = "myeditor",
+ *     "name" = "drupal.myeditor"
+ *   }
...
+  public function generateJSSettings(Editor $editor) {

I do not think that the actual library definition/reference can be part of the annotation. That is, because one editor library can have multiple libraries.

The libraries can be source/jquery/non-jquery variants, but they can also be local vs. remotely-hosted/CDN variants.

Annotations are for meta data only. "Meta data" generally boils down to: Information that is needed _without_ instantiating the plugin.

I.e., pretty much comparable to any other D7-style hook pattern: The info hook declares meta data that's readily available _before_ anything is actually used. And the info hook declares callbacks [methods] to be invoked when a particular thing is used.

Alas, the library to use is not needed upfront. It is only needed when the editor plugin actually gets instantiated/loaded. By moving the library definition out of the annotation and into a class method, we open up the (required) door for 1) dynamic library definitions, 2) configurable libraries, 2) adaptable libraries.

Along with this change, we can rename generateJSSettings() into attach() and simply bake the entire logic for #attached into that.

+++ b/core/modules/editor/lib/Drupal/editor/Plugin/EditorBase.php
@@ -0,0 +1,83 @@
+  public function settingsForm(array &$form, array &$form_state, Editor $editor) {
+    return array();
+  }

1) $form should not be taken by reference.

2) The method should return $form.

+++ b/core/modules/editor/lib/Drupal/editor/Plugin/EditorBase.php
@@ -0,0 +1,83 @@
+  public function settingsFormValidate(array $form, array &$form_state) {
+    // Use form_set_error() to declare any validation errors.
+  }
...
+  public function settingsFormSubmit(array $form, array &$form_state) {
+    // Modify $form_state['values'] to adjust saved values.
+  }

The inline comments should be dropped. That's what the EditorInterface is for.

+++ b/core/modules/editor/lib/Drupal/editor/Plugin/EditorInterface.php
@@ -0,0 +1,98 @@
+ * Modules implementing this interface should instead extend the EditorBase

s/should instead/may want to/

+++ b/core/modules/editor/lib/Drupal/editor/Plugin/EditorInterface.php
@@ -0,0 +1,98 @@
+   * Returns the default settings for this configurable text editor.
...
+  function defaultSettings();

Common Drupal anti-pattern in core right now: Methods that are called as if they'd do something, but in reality, they return something.

If possible, we should rename this to ::getDefaultSettings().

+++ b/core/modules/editor/lib/Drupal/editor/Plugin/EditorInterface.php
@@ -0,0 +1,98 @@
+   * Note that this form will be a subform of filter_admin_format_form().
+   * Settings within this subform are validated by settingsFormValidate() and
+   * then saved exactly as specified in $form_state['values'] by
+   * editor_form_filter_admin_format_submit().

This paragraph can be dropped (after performing the other mentioned adjustments).

+++ b/core/modules/editor/lib/Drupal/editor/Plugin/EditorInterface.php
@@ -0,0 +1,98 @@
+   * @param array $form
+   *   The prepopulated form array of the filter administration form.
...
+  function settingsForm(array &$form, array &$form_state, Editor $editor);

(extending on earlier comments)

$form should be an empty array, not the entire filter format form. Implementations that need access to the complete form always have it via $form_state['complete_form']

+++ b/core/modules/editor/lib/Drupal/editor/Plugin/EditorInterface.php
@@ -0,0 +1,98 @@
+   *   A configured text editor object.
+   * @return array

Minor, but since the issue repeats itself through the entire code:

There should be a blank phpDoc line between @param directives and @return (and also after the @return directive, in case something else [e.g., @see/@todo] follows).

+++ b/core/modules/editor/lib/Drupal/editor/Plugin/EditorInterface.php
@@ -0,0 +1,98 @@
+   * The contents of the editor settings are located in
+   * $form_state['values']['editor_settings']. Calls to form_set_error() should
+   * reflect this location in the settings form.
...
+   * @param array $form
...
+  function settingsFormValidate(array $form, array &$form_state);

After changing to #element_validate, the first argument is $element instead of $form, all values are found (as usual) in $element['#value'], and form_error() should be used instead of form_set_error().

+++ b/core/modules/editor/lib/Drupal/editor/Tests/EditorLoadingTest.php
@@ -0,0 +1,132 @@
+    $filtered_html_format = array(
...
+      'filters' => array(
+        // URL filter.
...
+    $full_html_format = array(
...
+      'filters' => array(

+++ b/core/modules/editor/lib/Drupal/editor/Tests/EditorManagerTest.php
@@ -0,0 +1,141 @@
+    // Add text formats.
+    $filtered_html_format = array(
...
+      'weight' => 0,
+      'filters' => array(
+        // URL filter.
...
+    $full_html_format = array(
...
+      'weight' => 1,
+      'filters' => array(
+        'filter_htmlcorrector' => array('status' => 1),

Are the filter definitions for these test text formats actually needed and used by the test?

+++ b/core/modules/editor/lib/Drupal/editor/Tests/EditorLoadingTest.php
@@ -0,0 +1,132 @@
+    // Create node type.
+    $node_type = array(
...
+    // The newly added node type has caused some new permissions to become
+    // available, ensure WebTestBase::checkPermissions() knows about them.
+    $this->checkPermissions(array(), TRUE);

$this->drupalCreateContentType();

+++ b/core/modules/filter/filter.admin.inc
@@ -152,6 +152,7 @@ function filter_admin_format_form($form, &$form_state, $format) {
+    '#weight' => -20,

@@ -163,6 +164,7 @@ function filter_admin_format_form($form, &$form_state, $format) {
+    '#weight' => -19,

@@ -171,6 +173,7 @@ function filter_admin_format_form($form, &$form_state, $format) {
+    '#weight' => -10,

Let's use consistent intervals here, please; i.e., steps of -10, each.

Alternatively, was there any particular reason for not applying positive #weights to the following elements instead?

Putting my site builder hat on, I'm generally annoyed by negative weights in default form constructors and much rather appreciate sensible positive weights. E.g., as in this case, there's hardly a scenario in which a module would want to inject an element to have it appear below the filter settings vertical tabs.

#129: Thanks! As quicksketch already said, this is an incredibly thorough review. Most helpful! Posted at 6:23 AM our time. Please also think of your health :)

I'm currently working on incorporating as much of your feedback as possible (I'm assuming that quicksketch is not already working on it, otherwise he'd have posted the feedback for as far as he'd gotten).

All incorporated, but also took #130 into account while doing so (this means that I didn't copy what you suggested 1:1 for e.g. the .info file).

Other things that didn't work out:

By moving 'editor' into $form_state, this massaging can be moved into the #ajax callback, which essentially replaces the object in $form_state with a new instance only.

Unless I'm missing something, it's not quite as simple as that. An #ajax callback receives the already processed form. If we move the logic to set the variables that will cause the form to render differently in the #ajax callback, that means we'll have to process the form again. The end result then looks like this, and I wouldn't call it a net improvement (but I've included it in this reroll):

--- a/core/modules/editor/editor.module
+++ b/core/modules/editor/editor.module
@@ -108,23 +108,13 @@ function editor_form_filter_admin_overview_alter(&$form, $form_state) {
  * Implements hook_form_FORM_ID_alter().
  */
 function editor_form_filter_admin_format_form_alter(&$form, &$form_state) {
-  $format = $form['#format'];
-  $editor = editor_load($format->format);
-  $manager = drupal_container()->get('plugin.manager.editor');
-
-  // Check if we're switching to a different text editor and load that form.
-  if (isset($form_state['values']['editor'])) {
-    if ($form_state['values']['editor'] === '') {
-      $editor = FALSE;
-    }
-    elseif (empty($editor) || $form_state['values']['editor'] !== $editor->editor) {
-      $editor = entity_create('editor', array(
-        'name' => $format->name,
-        'format' => $format->format,
-        'editor' => $form_state['values']['editor'],
-      ));
-    }
+  if (!isset($form_state['editor'])) {
+    $format = $form['#format'];
+    $form_state['editor'] = editor_load($format->format);
+    $form_state['editor_manager'] = drupal_container()->get('plugin.manager.editor');
   }
+  $editor = $form_state['editor'];
+  $manager = $form_state['editor_manager'];
 
   // Associate a text editor with this text format.
   $editor_options = $manager->listOptions();
@@ -174,6 +164,25 @@ function editor_form_filter_admin_format_form_alter(&$form, &$form_state) {
  * AJAX callback handler for filter_admin_format_form().
  */
 function editor_form_filter_admin_form_ajax($form, &$form_state) {
+  $editor = $form_state['editor'];
+  if (isset($form_state['values']['editor'])) {
+    if ($form_state['values']['editor'] === '') {
+      $form_state['editor'] = FALSE;
+    }
+    elseif (empty($editor) || $form_state['values']['editor'] !== $editor->editor) {
+      $format = $form['#format'];
+      $editor = entity_create('editor', array(
+        'name' => $format->name,
+        'format' => $format->format,
+        'editor' => $form_state['values']['editor'],
+      ));
+      $form_state['editor'] = $editor;
+    }
+  }
+
+  // Process the form again, so that $form['editor_settings'] is updated.
+  drupal_process_form($form['#form_id'], $form, $form_state);
+
   return $form['editor_settings'];
 }

Why -9? Let's use -10 here? [That said, see also my other remark (far) below.]

To have the "Editor" select appear below "Roles", but above "Enabled filters".

Not sure I get the need for this exposure — I'd prefer to simply set #access to FALSE in that case.

I disagree. If we don't show this disabled option (along with the description that provides background info), then how will users find out how to enable a text editor? Now they're at least being told that they must enable a module that provides a text editor.

Alas — it should be duly noted that this entire code here actually translated into fieldable filter_format config entities ;) (A config entity attached to another config entity.)

Indeed. But that's only because you wanted editor.module to be separate from filter.module, of course. Otherwise this'd be part of filter_format config entities. I understand your reasoning there — I'm only duly noting that as well.

I can certainly make sense of this, but are there actual performance benchmarks, which prove that this is actually valid and not premature optimization?

There are no benchmarks. Quite possibly this is a premature optimization. I'll leave it up to quicksketch to defend that. It's sensible IMHO though.

2) filter_formats() is insufficient/invalid, since contributed modules may alter the list of available text formats on a per-field basis. The formats to process need to be extracted from the available formats in the element only.

That means those modules are effectively circumventing the user's permissions, so I'm inclined to claim that we shouldn't support that. For now, I've included this though.

4) We need to inject a hidden form input element in order to attach an editor to a format-enabled form element that does not have a text format selector.

Incredible that we missed that. (I did it for Aloha.module, but failed to ensure it also worked here :/)

5) The server-side code needs to make sure to annotate the actual input/textarea element that should be targeted by the frontend/client-side code.

I'm not sure about this one. What do you mean exactly and in which circumstances is it necessary?

6) Special handling is required in case the associated text format is not accessible for the current user - which means that Filter module will replace the form element's content with a user-facing notice that the element is not editable.

Part of the simplicity here is that we leave access handling to be solely text format-based. quicksketch and I have not communicated about this explicitly, but I think it makes a ton of sense: the WYSIWYG editor is configured to match what is allowed by the text format's filters, so there's no need for yet another set of permissions.
Strong -1 for this.

7) There was a relatively brilliant concept around "triggers" in Wysiwyg's code, which I apparently can't fully remember + describe right now (it is getting late over here), but I know that it was refactored and added later on only, and also wasn't complete yet. Roughly speaking, the JS settings required for each editor/format/field combination are pretty huge (» frontend performance), so the first step is to detach the format from the field, so format-level settings apply to all fields, but individual fields are still able to override format-level settings. Followed by a second step of detaching formats from [global] editor settings (IIRC, not implemented in W. yet), which ultimately compresses the entire, huge swath of JS settings down to global editor » format » field overrides.

You know that I'm in favor of anything that can improve front-end performance :). But this is not the right time and issue for that. Let's do that in a follow-up issue.

1) How can $this.is(':input') ever not be true

Good point. Fixed. But looking at wysiwyg.js, I see the same check. Why is it necessary for WYSIWYG module? (Just trying to make sure we learn as much as possible from WYSIWYG module.)

CKEditor actually supports to replace textfields in addition to textareas. In general, the discovery/selector being used here is prone to errors - we need to annotate the input field/element that needs replacement on the server-side already.

Good point about textfields. I think most people commonly forget that these can contain filtered text, too. I know I forget about it :)
I disagree. This implies more settings to be passed, whereas looking for textarea and input[type=text] should be sufficient. Hence I've chosen to just improve the selector.

Drupal.behaviors.editor is missing a .detach method that properly responds to possible triggers.

We need to save the editor's active contents back into the textarea e.g. when submitting the form via Ajax.

This is a major omission indeed. Fixed.

Having these as top-level methods on the Drupal object doesn't really make sense.

We should introduce a proper Drupal.editor object that supplies proper methods: […]

I also always found these rather strange (though WYSIWYG module has similar methods). At the same time, we have to be careful to keep this as simple as possible. So, for now, I'd rather see us stick with this approach and then improve editor.js based on feedback from you and TwoD, so that we can design editor.js in such a way that WYSIWYG module can extend it easily and elegantly.

Can we rename this to $library?

An Editor entity with an $editor property looks like infinite recursion to me.

I disagree. An Editor entity with a $library property looks like a reference to a library definition in hook_library_info().

This is why I introduced the distinction in the code comments between "configured text editor" and "text editor". A text editor is just that, a text editor. A configured text editor is one that has configured settings and is associated with a text format — i.e. an Editor ConfigEntity.

I attempted to make this somewhat more clear by cleaning up the code comments in Editor.php further.

1) We can skip the array_merge() by initializing default settings before calling parent::__construct().

2) There's no protection against an empty/unset $editor property.

3) In general though, this pattern looks odd to me. If I create a new Editor entity, then I actually expect that to be empty (or to contain the most basic defaults only).

1) No, we can't, because to know the default settings, we need $this->editor, and for that we must call the parent constructor first.
2) No Editor objects is created with an empty editor property. See editor_form_filter_admin_format_submit().
3) I agree it looks odd. Particularly because if the default settings are changed, then the resulting settings may start to look differently.

So instead of merging, I changed it to:

    if (empty($this->settings)) {
      $this->settings = $plugin->getDefaultSettings();
    }

quicksketch, can you chime in?

I do not think that the actual library definition/reference can be part of the annotation. That is, because […]

I partially disagree.

• Disagree: Remotely hosted/CDN variants: a module that wants to serve many libraries from a CDN can easily do so by altering this metadata.
• Agree: Dynamic library definitions (or rather, dynamically selecting a library, because library definitions are by definition static) are useful.
• AFAIK there is (unfortunately) no policy about which data belongs in annotations. Annotations are inherently static. IMHO that implies that any information about a plugin that is static, can live in the annotations. This also clearly indicated that text editors always use the same library (CSS/JS), but can be configured differently dynamically thanks to attach
• Since I agree with the dynamic selection of libraries, and that is clearly tied to how WYSIWYG module can leverage Editor module, I've moved the library out of the annotation.
• Disagree: we cannot simply bake the entire logic for #attached (the way it is handled in editor_pre_render_format()) to the plugin level, because in the #pre_render callback we may need to attach the resources for several text editors. So, instead of renaming generateJSSettings() to attach(), we should rename it to getAttachments(). Finally, generateJSSettings() generates just the JS settings for the editor and doesn't need to know about the eventual structure that the Editor module uses for its JS settings (drupalSettings.editor.formats[formatID] = {editor: editorName, editorSettings: <SETTINGS>), so however tempting/elegant this attach()/getAttachments() idea, it's not quite that simple.

We need to think this one through more. I haven't changed this at all yet in this reroll.

After changing to #element_validate, the first argument is $element instead of $form, all values are found (as usual) in $element['#value'], and form_error() should be used instead of form_set_error().

Under the hood, we use #element_validate, hence we should rename $form to $element. That makes sense. But from the plugin implementer's POV, it's still a form. So I'd rather stick with $form.

• Updated version of CKEditor to match the plugin changes over at #1874332-9: Convert to use the plugin system.
• I haven't added the EditorAdminTest yet.

I hope I didn't forget anything — while it was a very thorough review, it's also to forget something while changing so many little things :)

Thanks! I did not re-review and compare the patch against #129 yet, but here's some more feedback/replies:

re: #132: @Wim Leers:

That means those modules are effectively circumventing the user's permissions, so I'm inclined to claim that we shouldn't support that.

Nope, not necessarily. In general, the go-to solution for this is http://drupal.org/project/better_formats - which has a very high usage/adoption rate and doesn't do anything evil in terms of security. Most sites that need Wysiwyg module actually need Better Formats module, too. We've ensured optimal compatibility between both modules over the past years. I guess it's too late for pulling its primary functionality into core for D8, but we definitely want to do that at some point.

> 5) The server-side code needs to make sure to annotate the actual input/textarea element that should be targeted by the frontend/client-side code.

I'm not sure about this one. What do you mean exactly and in which circumstances is it necessary?
...
This implies more settings to be passed, whereas looking for textarea and input[type=text] should be sufficient. Hence I've chosen to just improve the selector.

The JavaScript makes too many assumptions about the structure and HTML markup for a text format-enabled form element, including its exact position in the DOM, which it should not do, because theme functions may decide to output it differently, using more or different wrappers.

Instead of searching for the closest textarea/input nearby, the form_alter should inject an #attributes][class][] into $element['value'], so that the JS has a unique target element to search for.

> 6) Special handling is required in case the associated text format is not accessible for the current user - which means that Filter module will replace the form element's content with a user-facing notice that the element is not editable.

there's no need for yet another set of permissions.

Sorry for not being clear. I did not mean to add module permissions. What I meant is:

filter_form_access_denied() is a #pre_render callback that gets injected by filter_process_format(), in case the format-enabled form element has an existing value with a text format associated, which the current user is not allowed to use.

The unique condition for that is if (!empty($element['value']['#disabled'])).

> 1) How can $this.is(':input') ever not be true

looking at wysiwyg.js, I see the same check.

The outer selector in wysiwyg.js does not use .filter-list, but targets a generic .wysiwyg class instead:
http://drupalcode.org/project/wysiwyg.git/blob/HEAD:/wysiwyg.js#l46

The general idea there was to make it possible to trigger the attachment of editors in custom/other situations, merely based on the .wysiwyg class. We can look into that later. In the previous patch (I didn't check the new one yet), the double-condition on :input was merely pointless.

An Editor entity with a $library property looks like a reference to a library definition in hook_library_info().

This is why I introduced the distinction in the code comments between "configured text editor" and "text editor". A text editor is just that, a text editor. A configured text editor is one that has configured settings and is associated with a text format — i.e. an Editor ConfigEntity.

I'm afraid I cannot make much sense of the separation between a "configured text editor" and a "text editor". "A text editor is just that, a text editor."? Dude, what's a text editor? :)

I agree that $library wasn't a good suggestion either though. But what exactly is $editor? It's not a label. It's not a library ID. It seems to duplicate the plugin ID to some extent...? Is it just a plugin_id?

Speaking of, the configuration object - and thus the Editor config entity - must also record and contain the module that provided the editor plugin implementation; i.e., we need to add a $module property. That is, because Editor module is responsible for maintaining the editor entities - if an editor-plugin-providing module is disabled/uninstalled, then Editor module has to react to that in appropriate ways - for instance, the editor plugin is still assigned to a format, but it can no longer be edited in case the providing module happens to be disabled.

re: generateJSSettings() / attach() / getAttachments()

It seems you are trying to make a differentiation between the JS settings and the more generic attachment/loading of an editor library. That makes sense to a certain extent, but there is actually a close binding between the library files to load and the library settings.

- For multiple formats with different settings, different editor plugins need to be loaded. The total set of library files to load is the cumulative result of required plugins for all formats. The JS has to control which plugins are actually initialized for the editor instance of a particular format; there may be more plugins loaded than actually need to be initialized/used for a particular format/editor-instance.

- Ultimately, we want the editor settings to be configurable and the plugins to be configurable. This means that the settings/configuration of an editor have an impact on how the editor library is loaded.

- We can produce the JS settings in a separate attachSettings() method and have another attachFiles() method for producing the cumulative files to #attach for all needed formats.

Expectations that need tests:

• 'To have the "Editor" select appear below "Roles", but above "Enabled filters".' — That's a pretty concise expectation that other patches can unintentionally break too easily, so should be verified in a test.
• Appearance of HTML classes in the HTML markup

  - for a text format selector,
  - for a format-enabled element with only one format, and
  - for a format-enabled element to which the current user does not have access.

• Injection of the editor plugin settings form in the text format form (leveraging $this->drupalPostAjax()).

+++ b/core/modules/editor/editor.module
@@ -146,27 +133,24 @@ function editor_form_filter_admin_format_form_alter(&$form, &$form_state) {
+    $form['editor_settings']['settings'] = $plugin->settingsForm($form, $form_state, $editor);
+    $form['editor_settings']['settings']['#element_validate'][] = array($plugin, 'settingsFormValidate');

We need to pass the settings form section as $form only; i.e.:

$settings_form = array();
$settings_form['#element_validate'][] = ...;
$form['editor_settings']['settings'] = $plugin->settingsForm($settings_form, $form_state, $editor);

+++ b/core/modules/editor/editor.module
@@ -146,27 +133,24 @@ function editor_form_filter_admin_format_form_alter(&$form, &$form_state) {
     $form['#submit'][] = array($plugin, 'settingsFormSubmit');

We might need to move the settingsFormSubmit() invocation into editor_form_filter_admin_format_submit() and pass the sub-section of submitted form values only?

I.e., so that a particular plugin only ever deals with its own stuff by default - from the form, to validate, to submit.

+++ b/core/modules/editor/editor.module
@@ -174,9 +158,28 @@ function editor_form_filter_admin_format_form_alter(&$form, &$form_state) {
 function editor_form_filter_admin_form_ajax($form, &$form_state) {
+  $editor = $form_state['editor'];
+  if (isset($form_state['values']['editor'])) {
...
+  // Process the form again, so that $form['editor_settings'] is updated.
+  drupal_process_form($form['#form_id'], $form, $form_state);

Oh, OK. So if I get it right, then we actually want to move this a 'editor' form value processing into a #process callback that is defined for the 'editor' form element. That way:

1) When the form is initially built, the #process checks the value of $element['#value'] (of the 'editor' select element) and populates $form_state['editor'].

2) When the form is submitted - either via Ajax or regularly - then the #process callback gets invoked with the new submitted value and adjusts $form_state['editor'] accordingly.

3) $form['editor_settings'] lastly needs to be populated in the appropriate situations; i.e., during initial build, but also upon submit+rebuild. This could happen in the same #process callback, but I'm not sure whether that's a good idea (concerning hook_form_alter()).

I wonder whether our beloved Ajax guru @effulgentsia can provide some guidance?

+++ b/core/modules/editor/editor.module
@@ -225,17 +217,43 @@ function editor_load($format_id) {
+  // Use a hidden element for a single text format.
+  if (!$element['format']['format']['#access']) {
...
+    $element['format']['editor'] = array(
+      '#type' => 'hidden',
+      '#name' => $element['format']['format']['#name'],
...
+  // Otherwise, attach to text format selector.
+  else {
+    $element['format']['format']['#attributes']['class'][] = 'editor';
   }

I wonder whether we could simply change the #type of the 'format']['format' element from 'select' to 'hidden'? I.e.:

$element['format']['format']['#attributes']['class'][] = 'editor';
if (!$element['format']['format']['#access']) {
  $element['format']['format']['#type'] = 'hidden';
  $element['format']['format']['#value'] = $element['format']['format']['#default_value'];
}

Assigning to @effulgentsia for feedback on best/recommended #ajax usage/integration in the filter format edit form.

function editor_form_filter_admin_form_ajax($form, &$form_state) {
  ...
  // Process the form again, so that $form['editor_settings'] is updated.
  drupal_process_form($form['#form_id'], $form, $form_state);

What we tried to do when upgrading D6's AHAH system to D7's AJAX system is completely separate #ajax['callback'] functions from form processing. The purpose of the #ajax['callback'] function is simply to return the part of the form that needs to be sent back to the browser. Any needed server-side form rebuilding should happen prior to the invocation of #ajax['callback']. The reason we tried to do this is to ensure consistent server-side processing whether JS is enabled or not. Since #ajax['callback'] only runs when JS is enabled, any server affecting stuff that's in there must also get duplicated in some other "nojs" submit handler, and that gets annoying.

For buttons that trigger AJAX, it's pretty straightforward: do any $form_state affecting stuff in a #submit handler and end that handler by setting $form_state['rebuild'] = TRUE. The canonical example is the "Add another item" button in WidgetBase::formMultipleElements().

In this issue, however, looks like we want to trigger the AJAX from a select element, not a button. Unfortunately, we currently have at least 2 or 3 different ways in which we're doing that in core.
- In book.module, we add a 'pick-book' button that's hidden if JS is enabled, and has a submit handler that's only called for the nojs case. And then _book_add_form_elements() handles the AJAX case by adjusting $node the same way book_pick_book_nojs_submit() does. In the case of book.module, it's a tiny piece of server-side processing (set $node->book), so the duplication is not so bad.
- In Field UI's DisplayOverview::form(), we have a more complicated case of a 'refresh' button that in the case of JS enabled, we hide, and then have JS code that calls click() on that button when the user selects a different formatter.
- We also have support for a #ajax['trigger_as'] feature that lets you do something similar to the Field UI approach, but without needing custom JS code. However, it doesn't look like we're using this feature anywhere in core.

Here's a patch that demonstrates how to do the use-case here with the 'trigger_as' feature. It's my personal favorite one, and maybe in other issues, we can update book.module and similar use-cases to use it, and in the process refine some of the implementation details for it. Or, if you guys don't like it and want to suggest a better approach to standardize on, please do.

Awesome, thanks @effulgentsia! That's the kind of input I hoped for :)

So let's merge #135 into the patch and try to address the remaining issues from #133.

Not sure what else has been left out from #129, but I'll re-check once there's a new patch.

I'm on this again.

First: it's 2013 and we still can't rely on browsers to retain our meticulously entered data in textareas. (I replied to 90% of #133 yesterday, then went to a local meet up, then noticed it was just gone.) :(

#133:

theme functions may decide to output it differently, using more or different wrappers. […] inject an #attributes][class][] into $element['value'], so that the JS has a unique target element to search for.

That's a very good point!

Done, but not using classes like you suggested because then it's still hard/impossible to uniquely target the element to search for. I did it this way:

@@ -235,6 +238,7 @@ function editor_pre_render_format($element) {
   $format_ids = array_keys($element['format']['format']['#options']);
 
   // Use a hidden element for a single text format.
+  $field_id = $element['value']['#id'];
   if (!$element['format']['format']['#access']) {
     // Use the first (and only) available text format.
     $format_id = $format_ids[0];
@@ -244,12 +248,14 @@ function editor_pre_render_format($element) {
       '#value' => $format_id,
       '#attributes' => array(
         'class' => array('editor'),
+        'data-editor-for' => $field_id,
       ),
     );
   }
   // Otherwise, attach to text format selector.
   else {
     $element['format']['format']['#attributes']['class'][] = 'editor';
+    $element['format']['format']['#attributes']['data-editor-for'] = $field_id;
   }

if (!empty($element['value']['#disabled'])).

Already done in #132.

The outer selector in wysiwyg.js does not use .filter-list, but targets a generic .wysiwyg class instead: […] the double-condition on :input was merely pointless.

Oh, of course! Already fixed in #132.

I'm afraid I cannot make much sense of the separation between a "configured text editor" and a "text editor". "A text editor is just that, a text editor."? Dude, what's a text editor? :)

Hah :) Of course I agree that the distinction is far from clear enough; but I couldn't think of anything better.

Is it just a plugin_id?

Yes!

we need to add a $module property. […] if an editor-plugin-providing module is disabled/uninstalled, then Editor module has to react to that in appropriate ways

I was going to say "no, we don't need to do that, because plugin_ids are supposed to be unique", but your point about reacting to disabled modules is of course a good one.

However, I can't imagine that there aren't any other plugin-providing modules in core that need to react in this way too. How are they doing this?

We can produce the JS settings in a separate attachSettings() method and have another attachFiles() method for producing the cumulative files to #attach for all needed formats.

This is precisely what I was thinking too, but hadn't posted yet because I figured it'd be deemed an unacceptable solution. Glad to see I'm not alone here :)

Now implemented.

Expectations that need tests: all covered, and more.

Unless mentioned here, your new suggestions have been taken into account. Special thanks to effulgentsia in #135 of course, without whose help this would have remained far more kludgey :)

I.e., so that a particular plugin only ever deals with its own stuff by default - from the form, to validate, to submit.

That's always been the goal IMO.

We might need to move the settingsFormSubmit() invocation into editor_form_filter_admin_format_submit() and pass the sub-section of submitted form values only?

I'm not sure how to do this cleanly. The thing is, this is not intended to be a "real" submit callback, because the saving itself is taken care of by editor_form_filter_admin_format_submit():

    $form_state['editor']->settings = $form_state['values']['editor_settings'];
    $form_state['editor']->save();

I haven't changed this yet.

I wonder whether we could simply change the #type of the 'format']['format' element from 'select' to 'hidden'?

I don't think that's a good idea: then we're preventing other modules from doing the same checks that we are doing in the Editor module. If that's not a real concern, then yay for less code.

Updated version of CKEditor to match the plugin changes over at #1874332-10: Convert to use the plugin system.

Yay — all 112 added assertions green :)

I noticed that I'd left a few lines of debugging code in the tests. Fixed.

I've not made a deep analysis of the latest patch but I noticed a few things when looking through it.

Getter methods:
Don't we have the same thing as before (defaultSettings() vs getDefaultSettings()) with the attach* methods? Looking at the first line in their comments, they all state "Returns [...] settings to be attached." and end with "An array of [...] that will be added...".
They even include a "@see EditorManager::getAttachments()" line, indicating they are to return what they need attached rather than attach anything themselves.

I'm not entirely sure if settingsForm() is to be considered a getter or not though...

Annotation examples:

+++ b/core/modules/editor/lib/Drupal/editor/Plugin/EditorBase.phpundefined
@@ -0,0 +1,66 @@
+ * @Plugin(
+ *   id = "myeditor",
+ *   label = @Translation("My Editor"),

Missing module?

+++ b/core/modules/editor/tests/modules/lib/Drupal/editor_test/Plugin/editor/editor/UnicornEditor.phpundefined
@@ -0,0 +1,65 @@
+ *   library = {
+ *     "module" = "edit_test",
+ *     "name" = "unicorn"
+ *   },

Still includes library.

#141:

• hah, I was thinking the same thing in bed last night about attach(Files|Settings). Will fix in next reroll.
• 'module' missing in EditorBase: will fix in next reroll.
• 'library' in UnicornEditor: will fix in next reroll.

Quick reroll while awaiting further feedback. Addressed #141/#142, plus improved 2 comments.

"Quick reroll" is one without running the tests — bad idea. I forgot to update the function names in two places. OTOH, this means that the tests are doing what they're supposed to :)

Besides that, I've now also created an issue for adding in-place editing (edit.module) support to Editor, as mentioned in #68, #80 and #93. It was removed from this patch in #100 because it makes more sense as a follow-issue to this one (see #115 and #116). So, here is the second follow-up issue for this issue: #1886566: Make WYSIWYG editors available for in-place editing. Issue summary updated.

The wysiwyg_ckeditor module's 8.x-1.x branch is compatible with the patch in #144 as of this commit: http://drupalcode.org/project/wysiwyg_ckeditor.git/commit/33f1c07.

And as of now, Spark 8.x-1.x includes this patch plus related patches, to show you Editor.module-powered CKEditor, for both back-end (form) editing and front-end (in-place/inline/"true WYSIWYG") editing. Set up a quick demo for yourself over at http://simplytest.me/project/spark/8.x-1.x.

(Also retroactively tagging.)

yched's been wanting to push some of the Field module patterns related to settings management deeper into the plugin system: #1764380: Merge PluginSettingsInterface into ConfigurableInterface. Getting this issue in with a similar pattern might help with that.

Including the default settings could make the annotations quite large over time, if accounting for all settings available in certain editors, and the plugins. Ref: http://www.tinymce.com/wiki.php/Configuration
Is the discovery mechanism up to that task?

*personal opinion*
I don't think we should annotations this way. Annotations are mainly used to define stuff that is needed to get the object loaded. These wysiwyg settings belong to a protected $setting property.

#153 and #154 are in line with what sun said in #129:

Annotations are for meta data only. "Meta data" generally boils down to: Information that is needed _without_ instantiating the plugin.

I.e., pretty much comparable to any other D7-style hook pattern: The info hook declares meta data that's readily available _before_ anything is actually used. And the info hook declares callbacks [methods] to be invoked when a particular thing is used.

Initially, I wasn't sure about it, but if you think about the ugliness, maintainability and fragility of annotations, I think that makes a lot of sense. We don't have "schemas" for annotations, so it's impossible to enforce correctness. The reasoning outlined by sun forces as many things as possible to live in normal code instead of annotations, which makes it a lot easier to enforce correctness.

#154: a problem with having the settings in a $settings property or getDefaultSettings() method (as is currently the case) is that there's not a very elegant way for other modules to alter the settings (#148). That's why quicksketch was contemplating moving the settings over to annotations in #149 & #152. And coming to the same conclusion as you are in #153.

That's why I propose a somewhat less elegant method, that doesn't have the downsides of annotations: a hook_editor_default_settings($settings, $editor_id) (with $editor_id == $plugin_id):

+    $plugin = $manager->createInstance($this->editor);
+    $this->settings = array_merge($plugin->defaultSettings(), $this->settings);
+    drupal_alter('editor_default_settings', $this->settings, $this->editor); 

The OO part of my brain would say we just should extend the class and edit the settings. But I guess with these kinda plugins multiple modules will inject settings in the same class.

Your solution sound Ok'ish but I would call the alter hook 'editor_settings' and not 'editor_default_settings' as the defaults already got extended by other modules.

#156: I don't really care about the hook name, but I don't see how the default settings already got extended by other modules? I think you misread the code.

Yeah I did misread the code. :)
Ok I would go with that solution for now.

Just wondering, where is this issue going? It seems like the mega-reviews are taken care of. Given that we only have like 1 month to get CKEditor into core, should we get this to RTBC? I am sure there are plenty of things to make nicer, but with this big of a change that is normal.

This looks great to me. There might be a few minor things here that could be improved, but I think that can happen in follow ups. We're more likely to discover what those are when we do #1878344: Add CKEditor JS library to core and people play with some other editors in contrib. For which, we need this initial patch to land.

Whoo! Looking forward to this :)

Committed to 8.x. Great birthday present for Drupal! :)

Glad to see this in. :)

I think @TwoD and me should have deserved commit credit, too, but you can't have everything ;)

I think @TwoD and me should have deserved commit credit

Definitely. There were a couple issues in the recent past where we fixed a commit credit mistake by reverting the original commit and recommitting with proper credit: Issue #1833716 by quicksketch, Wim Leers, sun, TwoD: Added WYSIWYG: Introduce 'Text editors' as part of filter format configuration. RTBC'ing this for committer attention.

Woah, yesterday night when I stopped working, there hadn't been any activity in 12 hours, now another 12 hours later and it's been RTBC'd and committed! Thanks, effulgentsia and Dries for the super fast turnaround time!

I'd never expected this to go from RTBC to committed so fast, otherwise I'd have written a commit message with proper credit in the issue summary; of course I agree with #170 and #171!

Also note that because this issue has been committed, as well as #1874936: Pluggable in-place editors (allow modules to define new Create.js PropertyEditor widgets) a few days ago, we can now also work on the follow-up issue at #1886566: Make WYSIWYG editors available for in-place editing.

It's ready for review, it's working great (you can see it live in action at http://simplytest.me/project/spark/8.x-1.x), but for right now the top priority is CKEditor itself, i.e. CKEditor for forms, not CKEditor for in-place editing, i.e. #1878344: Add CKEditor JS library to core. However, if you want to help push that forward, reviews are definitely welcome over at #1886566: Make WYSIWYG editors available for in-place editing.

@sun and @TwoD: sorry for omitting you guys from the commit message. That was an oversight, and not intensional. We can re-commit it with the proper credit.

Done!