WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#20055 closed enhancement (fixed)

Load .dev.js scripts for TinyMCE plugins if $concatenate_scripts == false

Reported by: ericlewis Owned by: azaozz
Milestone: 3.4 Priority: normal
Severity: normal Version:
Component: TinyMCE Keywords:
Focuses: Cc:

Description

After banging my head on a Javascript / TinyMCE wall for a bit this morning, I figured I would write a patch for this issue. This makes debugging TinyMCE plugins a lot easier.

If $concatenate_scripts is false ( which is automatically FALSE if SCRIPT_DEBUG is true ) editor_plugin.dev.js files will be scanned for and included automatically, and if they don't exist their editor_plugin.js counterpart is loaded.

I spoke to _duck in IRC and he implied there may be a reason for not loading .dev.js scripts, so there may be a reason not to do this, however I can't really see one.

Attachments (6)

1.diff (1.3 KB) - added by ericlewis 2 years ago.
20055.diff (148.0 KB) - added by ericlewis 2 years ago.
packaged .dev.js non-WP plugin files from TinyMCE source; also themes/advanced/editor_template.dev.js added ternary declaration of suffix of tinyMCEPreInit depending on SCRIPT_DEBUG constant.
20055.1.diff (154.0 KB) - added by ericlewis 2 years ago.
As per Nacin's suggestion, remove script_concat_settings(), move logic for compressions and concatenations to the function calls. Still has respect to the globals in case any plugin sets them. Just saw azaozz's comment, this will need to be ammended to use MCE debug mode.
test_compression.diff (2.8 KB) - added by nacin 2 years ago.
20055-2.patch (213.9 KB) - added by azaozz 2 years ago.
20055-3.patch (7.0 KB) - added by ericlewis 2 years ago.
If this is still on the table, this patch fixes the "polluted globals" issue Nacin cited earlier, using a new canonical format, which respects the global then the constant, varying a bit between the different cases. Also implements the "_src" conditional loading azaozz implemented earlier. I'll learn how to create a diff from a patch and more changes for next time ;)

Download all attachments as: .zip

Change History (19)

ericlewis2 years ago

comment:1 follow-up: nacin2 years ago

Not sure what the best solution here is, but something needs to happen here to clean this up.

The script-loader a while back got polluted with globals $concatenate_scripts, $compress_scripts, and $compress_css. This is silly as we also have constants for this, and also a site option called 'can_compress_scripts'. None of these are canonical. It's silly to have all of these settings. script_concat_settings() should die and the values should be wrapped up into a function or two. Then TinyMCE should also respect SCRIPT_DEBUG.

For this particular problem:

Hypothetically, setting tinyMCEPreInit.suffix to be '.dev' is enough to load editor_plugin.dev.js files. But, if the .dev.js file does not exist, as it does for all non-WP plugins, then TinyMCE breaks. I would have no problems packaging the .dev.js files for the non-WP plugins if that's what it takes. It doesn't look like external plugins (added by WP plugins) would be affected.

ericlewis2 years ago

packaged .dev.js non-WP plugin files from TinyMCE source; also themes/advanced/editor_template.dev.js added ternary declaration of suffix of tinyMCEPreInit depending on SCRIPT_DEBUG constant.

comment:2 ericlewis2 years ago

20055.diff fixes the issue as Nacin suggested, but doesn't deal with the polluted globals issue.

comment:3 in reply to: ↑ 1 azaozz2 years ago

Replying to nacin:

The script-loader a while back got polluted with globals $concatenate_scripts, $compress_scripts, and $compress_css. This is silly as we also have constants for this, and also a site option called 'can_compress_scripts'. None of these are canonical. It's silly to have all of these settings.

Agreed. Now most of these aren't needed. At the time of adding them few years ago, some web servers had different issues when compressing scripts from PHP (disregarding headers, double compressing, etc.) that seem (mostly?) resolved now.

Hypothetically, setting tinyMCEPreInit.suffix to be '.dev' is enough to load editor_plugin.dev.js files.

Right, the problem here is mostly with MCE plugins added by WP plugins not having .dev.js files and also with the default MCE plugins having slightly different naming scheme (_src.js vs. .dev.js).

The other part is that MCE plugins are not loaded through script_loader. They are loaded by MCE using non-blocking methods. So force-loading everything as in 1.diff above would not work well. MCE has a debug mode loading/init we should use for that. The only requirement would be to rename our custom plugins that are in the MCE's plugins directory from .dev.js to _src.js. There doesn't seem to be a problem with that as currently the .dev.js files there are not used anywhere (they are distributed only as sources for the editor_plugin.js files).

ericlewis2 years ago

As per Nacin's suggestion, remove script_concat_settings(), move logic for compressions and concatenations to the function calls. Still has respect to the globals in case any plugin sets them. Just saw azaozz's comment, this will need to be ammended to use MCE debug mode.

comment:4 nacin2 years ago

The only requirement would be to rename our custom plugins that are in the MCE's plugins directory from .dev.js to _src.js. There doesn't seem to be a problem with that as currently the .dev.js files there are not used anywhere (they are distributed only as sources for the editor_plugin.js files).

Let's do it. bumpbot can easily account for the change.

nacin2 years ago

comment:5 nacin2 years ago

test_compression.diff is the directionality plugin's editor_plugin.js file. Looks like the minification is the same technique, but variables are assigned differently.

We will A) need to blacklist external plugins for bumpbot, B) whitelist WP plugins for bumpbot, or C) let bumpbot compress the external editor_plugin.js files the way it wishes. C, of course, is easiest.

comment:6 nacin2 years ago

I also forgot that wp-tinymce.js.gz needs to be built. Thinking bumpbot.php should do this as well.

comment:7 azaozz2 years ago

Replying to nacin:

We will A) need to blacklist external plugins for bumpbot, B) whitelist WP plugins for bumpbot, or C) let bumpbot compress the external editor_plugin.js files the way it wishes. C, of course, is easiest.

As the number of our custom plugins for MCE doesn't change a lot (4 times since WP 2.5) and we shouldn't minify the default MCE plugins that come in the MCE distribution, thinking it would be better to whitelist our plugins in bumpbot.

Alternatively we can whitelist all plugins whose directory name starts with "wp" or "wordpress".

Yes, wp-tinymce.js.gz can be build automatically. The format is pretty loose: the MCE's core has to go in first and the "mark as loaded" function has to be last. Not a big priority though as for development it can be re-added by hand after changing one of our MCE plugins.

azaozz2 years ago

comment:8 azaozz2 years ago

20055-2.patch seems to work properly. The only concern is about WP plugins that add MCE plugins and don't follow the "right way to do it" as described at 'mce_external_plugins' filter. (Note: it adds about 100KB of non-minified MCE plugins).

Scratch that. It used to be possible to add external plugin by only the URI to the plugin's directory. That has been replaced with specifying MCE plugin dependencies that seem to be only supported for the default plugins in tinymce/plugins/ directory.

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

ericlewis2 years ago

If this is still on the table, this patch fixes the "polluted globals" issue Nacin cited earlier, using a new canonical format, which respects the global then the constant, varying a bit between the different cases. Also implements the "_src" conditional loading azaozz implemented earlier. I'll learn how to create a diff from a patch and more changes for next time ;)

comment:9 azaozz2 years ago

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

In [19945]:

Load TinyMCE's non-minified "*src.js" plugin files when SCRIPT_DEBUG is defined, part props ericlewis, fixes #20055

This commit also includes the source for the mark_loaded function that goes at the bottom of wp-tinymce.js.gz: tinymce/mark_loaded_src.js.

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

comment:10 azaozz2 years ago

  • Milestone changed from Awaiting Review to 3.4

comment:11 nacin2 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

I think we should reconsider the implementation here. I'll comment in a bit.

comment:12 nacin2 years ago

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

Never mind. :-)

Will continue charset discussions in #19592.

comment:13 nacin2 years ago

script_concat_settings() continued in #20066. bumpbot aspects were handled in #19592.

Note: See TracTickets for help on using tickets.