Make WordPress Core

Opened 13 years ago

Closed 13 years ago

#19320 closed defect (bug) (fixed)

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

Reported by: nacin's profile nacin Owned by: nacin's profile nacin
Milestone: 3.3 Priority: highest omg bbq
Severity: blocker Version: 3.3
Component: Editor Keywords: has-patch
Focuses: Cc:

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.

Attachments (7)

19320.patch (6.5 KB) - added by azaozz 13 years ago.
19320.diff (24.5 KB) - added by nacin 13 years ago.
19320-2.patch (24.4 KB) - added by azaozz 13 years ago.
Fixed 19320.diff
19320-3.patch (8.2 KB) - added by azaozz 13 years ago.
19320.2.diff (24.6 KB) - added by nacin 13 years ago.
19320-4.patch (1.8 KB) - added by azaozz 13 years ago.
19320-5.patch (1.9 KB) - added by azaozz 13 years ago.
Fix setting of self::$this_tinymce

Download all attachments as: .zip

Change History (29)

#1 @nacin
13 years ago

tinymce_include() has the same issue.

#2 @ocean90
13 years ago

  • Cc ocean90 added

#3 in reply to: ↑ description ; follow-up: @azaozz
13 years ago

Replying to nacin:

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.

#4 in reply to: ↑ 3 ; follow-up: @nacin
13 years ago

Replying to azaozz:

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.

#5 @nacin
13 years ago

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.

#6 @anointed
13 years ago

  • Cc anointed added

#7 in reply to: ↑ 4 @azaozz
13 years ago

Replying to nacin:

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.

@azaozz
13 years ago

#8 @markjaquith
13 years ago

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.
  • Public APIs that don't box us in.

#9 @azaozz
13 years ago

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.

#10 @ryan
13 years ago

  • Version set to 3.3

#11 @nacin
13 years ago

Suggested fixes:

  • WP_Editor becomes _WP_Editors.
  • Class becomes final.
  • 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.
  • Anything necessary from 19320.patch goes in.
  • Action callbacks become array( '_WP_Editors, 'method_name' ), rather than $this. The $wp_editor global is destroyed entirely.

I'm being very thorough here. There really isn't much work to make this happen. If azaozz can merge in 19320.patch then I will work around that to implement the rest ASAP.

#12 @azaozz
13 years ago

In [19408]:

Restore back-compat with wp_tiny_mce(), see #19320

@nacin
13 years ago

#13 @azaozz
13 years ago

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 = ...

Fatal error: Access to undeclared static property: _WP_Editors::$this_tinymce
Version 2, edited 13 years ago by azaozz (previous) (next) (diff)

@azaozz
13 years ago

Fixed 19320.diff

#14 @nacin
13 years ago

I fixed those (and a few other references). Looks like the replacement upload failed. Will handle.

#15 follow-up: @azaozz
13 years ago

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.

Last edited 13 years ago by azaozz (previous) (diff)

#16 in reply to: ↑ 15 @nacin
13 years ago

Replying to azaozz:

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 or $class_instance->function().

But why??

#17 @azaozz
13 years ago

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 :)

Last edited 13 years ago by azaozz (previous) (diff)

@azaozz
13 years ago

@nacin
13 years ago

#18 @nacin
13 years ago

  • Keywords has-patch added

Per IRC: 19320.2.diff.

#19 @nacin
13 years ago

  • Owner set to nacin
  • Resolution set to fixed
  • Status changed from new to closed

In [19420]:

Move WP_Editor to a _WP_Editors encapsulation.

  • WP_Editor will return in 3.4 as a one true API for editor instances. Stick to wp_editor() for now.
  • TinyMCE can now be forced on with tinymce = true. It defaults to the value for user_can_richedit().
  • Restores wp_default_editor(), wp_link_query(), wp_link_dialog(), wp_fullscreen_html().

fixes #19320.

#20 @azaozz
13 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

This line https://core.trac.wordpress.org/browser/trunk/wp-includes/class-wp-editor.php#L45 ignores user_can_richedit() and the user option to have TinyMCE or not.

Think it was done that way so the visual editor can be used on the front end for not logged in users, but ignoring user options is a regression.

Looking for better workaround for that.

@azaozz
13 years ago

#21 @azaozz
13 years ago

19320-4.patch changes user_can_richedit() to default to true for logged out users which is the default for logged in users too.

@azaozz
13 years ago

Fix setting of self::$this_tinymce

#22 @azaozz
13 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In [19432]:

Changes user_can_richedit() to default to true for logged out users, same as the default for logged in users, fixes #19320

Note: See TracTickets for help on using tickets.