wp_tiny_mce() cannot call wp_editor(), and other issues

Description

wp_tiny_mce() previously spit out the settings for the editor. Now it actually calls wp_editor() which will render the actual editor. This has broken at least three plugins (more-fields, advanced-custom-fields, wp-resume).

We need to be able to render only pieces of WP_Editor for backwards compatibility. IMO, we need to make it an absolute priority to make WP_Editor a standard object that is instantiated for each editor. The abstractions that went into it for 3.3 are backwards and will constrain us for future releases. The singleton pattern here will hurt us.

Anything that is static in terms of general initialization, rather than specific to a single instance of an editor, should be just that — static.

A bunch of functions, such as wp_fullscreen_html(), wp_link_dialog(), and wp_link_query() were deprecated and merged into WP_Editor. This makes sense if and only if these methods are going to then leverage the individual editor instances. But, I recently confirmed, all they do is render generic content. They should be marked as static class methods, or even better, reverted to become standalone functions until it makes sense to bring them into a class. (Reminds me of WP_Screen all over again.)

I am willing to work on this but no earlier than this evening/tomorrow.

Change History (29)

IMO, we need to make it an absolute priority to make WP_Editor a standard object that is instantiated for each editor.

We've been over this a few times now. There's no point instantiating it for each editor instance. The different instances of the editors are in JS not in PHP. It works exactly like WP_Scripts or WP_Styles (we don't instantiate new WP_Scripts for each wp_enqueue_script).

The only reason it is a class is to keep track of what has been outputted already and to allow lazy loading. The first can easily be done with globals (although not the best way). The second cannot be easily done with loose functions, so if we want to get rid of the class we will have to include them on every page load. That's not good since these functions are needed less than 0.001% of the time.

There is no reason for it to follow OOP patterns, WP's code is not OOP and including a "standard" class is usually redundant.

Anything that is static in terms of general initialization, rather than specific to a single instance of an editor, should be just that — static.

In these terms all functions in WP_Editor are static. It doesn't make sense to have more than one instance of them. There's nothing to be gained from that, except it will "look" like OOP code.

A bunch of functions, such as wp_fullscreen_html(), wp_link_dialog(), and wp_link_query() were deprecated and merged into WP_Editor. This makes sense if and only if these methods are going to then leverage the individual editor instances.

All these functions have to ignore the number of editor instances. They should be run once per page load. They were kept as separate function as not all of them need to be run all the time an editor is included.

But, I recently confirmed, all they do is render generic content. They should be marked as static class methods, or even better, reverted to become standalone functions until it makes sense to bring them into a class.

Yes, they work in exactly the same way as in 3.2. The only reason they are in a class is to facilitate lazy loading, see above.

We've been over this a few times now. There's no point instantiating it for each editor instance. The different instances of the editors are in JS not in PHP. It works exactly like WP_Scripts or WP_Styles (we don't instantiate new WP_Scripts for each wp_enqueue_script).

Yes, and those are classes architected specifically for the singleton pattern. The script-enqueueing process and the style-enqueueing process each happen, in total, once per page.

Here we have multiple editors. There are very, very good reasons to ensure that the PHP class is instantiated individually for each editor. This includes the ability to properly extend it for a particular editor, and finely control various pieces and values. Additionally, it prevents us from having our hands seriously tied in the future.

At WordCamp Philly, I tried my hardest to enable the visual editor for a non-logged-in user on the frontend. Due to the singleton pattern and singular array of arguments, I found it to be impossible. Not all arguments belong in JS only, far from it.

I am far from a preacher of OOP patterns. But from a practical perspective, this makes sense not only now, but for maintaining and extending it for years to come.

Anything that is meant to be static, can be static. We can make it so things only run once regardless of how many times it is instantiated. This is easy.

Yes, they work in exactly the same way as in 3.2. The only reason they are in a class is to facilitate lazy loading, see above.

The functions didn't need to be deprecated to facilitate lazy-loading. They could have been moved to wp-includes/editor.php. I sure hope no one was using them directly, because backwards compatibility was not maintained.

A better design pattern brings along things like proper encapsulation, visibility, and inheritance, and allows us to mirror multiple JS instances in PHP. There is no reason, given all of the PHP code that goes into this, that multiple instances should be limited to JS. We have a class we tell people not to touch because it has moving bits, but on the other hand, we tell them it can be extended and used directly. This makes no sense, and will kill us when we want to do anything in the future.

Yes, and those are classes architected specifically for the singleton pattern. The script-enqueueing process and the style-enqueueing process each happen, in total, once per page.

So is the editors "enqueueing". At first, when starting to code it was a multi-instance class, but later dropped that in favor of the same design like WP_Scripts. WP_Editor does nothing more than enqueue html, js and css for the editor instances. It enqueues several components for each instance, similarly to WP_Scripts that enqueues several scripts for each wp_enqueue_script() to satisfy dependencies and doesn't enqueue the same script twice.

...
At WordCamp Philly, I tried my hardest to enable the visual editor for a non-logged-in user on the frontend. Due to the singleton pattern and singular array of arguments, I found it to be impossible.

Think the only thing that stops this at the moment is user_can_richedit() which expects the user to be logged in. That works the same way as the_editor() in 3.2.

I am far from a preacher of OOP patterns. But from a practical perspective, this makes sense not only now, but for maintaining and extending it for years to come.

Anything that is meant to be static, can be static. We can make it so things only run once regardless of how many times it is instantiated. This is easy.

I'm sure we can make it work as "proper" OOP class. My doubts are if we really need that / what are the benefits of making it more complicated. It is a simple semi-private class that has to be called from an external function to load. If in the future we decide to change it, there will be no problems maintaining back-compat in that function like instantiating it multiple times, etc.

The functions didn't need to be deprecated to facilitate lazy-loading. They could have been moved to wp-includes/editor.php. I sure hope no one was using them directly, because backwards compatibility was not maintained.

Lazy-loading seems to work best when used with one class = one file. Of course we can break these functions apart and do if ( ! function_exists(...) ) include_once(...) for each.

BTW nearly done with making wp_tiny_mce() fully back-compat. Thinking we can use that code whether or not we change WP_Editor.

It's an odd class, that's for sure. It has instance variables, but to output a new editor, no new instance is created, the instance variables are just changed. It would make a lot more sense as an actual class with a separate instance for each editor. And either static methods or a WP_Editor_Factory singleton to control the global stuff that needs to happen. But I don't think we can get into all that now.

I'd like to prioritize the following goals:

Backwards compatibility. Let's not break plugins.

Making sure no one can use this class in its current state. I'd go beyond the admonishment in the file and actually force it to be a singleton. That way we can clean it up in a subsequent release. It's not meant to be reused, so let's make sure it can't be.

Yeah, it is an odd class. It was modeled after WP_Scripts and is roughly equivalent of enqueueing two scripts that have shared dependencies and reuse the same localization object.

The truth is that once the JS and CSS is outputted there's no need of any more PHP output to create more instances of the editors, both TinyMCE and Quicktags. It even had an (very) early iteration where the HTML didn't have any IDs, the whole DOM branch was cloned with JS and the editors initialized for the second, third, etc. instances.

We probably can go one step back and split WP_Editor in separate functions. The code would be more verbose but not too much. We still can keep all related functions in a separate file (as @nacin suggests above) and would need to do include_once() for that file if somebody calls an old (deprecated) function.

The public API is just one wrapper function that currently includes the file and passes along the args. Don't see how that can box us in, we can change it at any time to do anything else we need.

Constructor becomes private and made into a proper singleton. All methods are made static. Most methods and variables are made private. Method calls should be changed to use the paamayim nekudotayim (::, of course).

The default value for the tinymce setting should be null. If it remains === null, then it should be set to user_can_richedit().

wp_fullscreen_html(), wp_link_dialog(), and wp_link_query() are brought back for backwards compatibility reasons, undeprecated for now. They can wrap the current static functions. They can check ! class_exists( '_WP_Editors' ) to ascertain whether the file needs to be included.

Thought we were using a single internal instance of the class as described here: ​http://www.php.net/manual/en/language.oop5.patterns.php#example-202 together with declaring the class static. That would let us use $this->whatever. Main reason is that self::$var doesn't work right in strings, i.e. $js = "var something = '{self::$var}foo';"; fails.

Doesn't seem this works right: private $this_tinymce = false; and then from whitin a function self::$this_tinymce = ...

19320-3.patch makes the class one instance only (throws an error if instantiated again) and doesn't need all static stuff. Also allows normal use of $this and $class_instance->function(). The three functions that only output html are still static, can be called directly from the deprecated old functions if needed.

Well, it serves the same purpose without making everything static (which looks strange imho). Also I would rather throw an error when it's instantiated second time instead of making it no point to be instantiated. And I had this idea after the first time we talked about it, just wanted to put it up here as it works too :)