Add support for TinyMCE gzip compressor

I'm having the same issue with TinyMCE on Drupal 5.7. I've tested it on FF3 and IE7.

I can confirm this in Drupal 5.8: when I remove the compressor files, tinymce shows up. With those files placed in the tinymce directory, the editor will not display.

I've tried to implement support for gzip compressor now. However, TinyMCE fails to load external plugins (hook_wysiwyg_plugin()) when gzip compressor is enabled/used.

Basically, the first change in wysiwyg_editor.js (and subsequent replacements of tinyMCE with Drupal.wysiwygEditor) in attached patch should be sufficient actually, but that doesn't seem to be the case.

I'm inclined to postpone this feature until support for TinyMCE 3.x has been implemented, which hopefully works better.

Yes, I've had this issue for awhile - which is a shame, as JS compression really speeds up site performance - so until then, high traffic production sites can't use TinyMCE :(

As sun suggested, this is probably related to the Moxiecode TinyMCE module and not the WYSIWYG module? Please correct me if I'm wrong.

This is anything but critical.

Related thread under TinyMCE mod: http://drupal.org/node/320789

Potential patch: http://drupal.org/node/320789#comment-1167075

Related thread under JS Aggergator Mod: http://drupal.org/node/351192

I do not plan to work on this further. Feel free to re-open this issue.

Pardon me, but reducing the size of javascript is essential, if not critical. Have anyone tested the patch with the latest module?

Pardon me, but that does not really sound like you will be working on a rock-solid patch that implements gzip compressor support for TinyMCE. Feel free to prove me wrong and re-open this issue if you want to provide a patch.

I do not have time to work on this right now, but might be able to in the future.

In the meantime, let's mark the status appropriately. "Needs work" means it will get done by someone at some point. "Won't fix" means "left for dead".

Thanks for your understanding.

Proper status is postponed then.

All I know is that (optional) support for TinyMCE's gzip compressor requires a few major API changes in Wysiwyg's handling of editor libraries. That is, because "gzip" is actually a new, separate library variant for TinyMCE that _may_ exist, but does not by default. This issue is not so much about implementing proper function invocations for this specific library, but rather about adding generic support for optional library variants for editors in Wysiwyg API.

Why not simply adding this line to Apache? It's not working for IIS, but it does the job and it does it for *all* editors and not only one...

AddOutputFilterByType DEFLATE text/css text/x-js

Feel free to re-open this issue if you want to attach a patch. For now, it adds only clutter to my queue.

FWIW, if you're using a sensible CDN, than a patch like this is not needed. You can configure your CDN to gzip CSS/JS and then you have no issues.

Patch is attached for review that adds a selection of compressed / uncompressed TinyMCE library.

To use:
1) apply the patch to wysiwyg 6.x-2.x-dev (2010-Feb-14)
2) install TinyMCE compressor files per their instructions (Do not modify any code!)
3) new wysiwyg detects the compressor and enables library selector in wysiwyg profile - just select Library: "Compressed" on /admin/settings/wysiwyg/profile/%%/edit (%% is your profile number) - see screenshot 1
4) save wysiwyg profile with "Compressed" library selected, and verify that compressor works on the server side (navigate to any page with the wysiwyg editor and check that file "/sites/all/libraries/tinymce/jscripts/tiny_mce/tiny_mce_.gz" gets created). You may need to change write permission to tiny_mce_gzip.php depending on ISP / server configuration.

I tested this patch with TinyMCE 3.3, with or without "Optimize JavaScript files" (see screenshot 2).

I did not test with TinyMCE 2, but in theory it should work, if anyone cares - please test and report here.

One usability note:
While testing patch in #15 on another ISP, selecting "compressed" library caused the editor to not load with javascript exception "'tinymce' not defined" in the console.

Upon quick investigation I found that a compressed file was never created on the server side (the one that resides in /sites/all/libraries/tinymce/jscripts/tiny_mce/ and is typically called tiny_mce_09d41278055f69b55e09d17f3db21685.gz with random digits part). In server logs I found:

Premature end of script headers: /home/.../sites/all/libraries/tinymce/jscripts/tiny_mce/tiny_mce_gzip.php
SoftException in Application.cpp:252: File "/home/.../sites/all/libraries/tinymce/jscripts/tiny_mce/tiny_mce_gzip.php" is writeable by group

After removing group write permission to tiny_mce_gzip.php, everything works fine. I will add small note to the instructions in #15.

Subscribing. Thanks for the effort.

The patch works on local install with the stable release. It calls the tiny_mce_gzip.js and produces the tiny_mce_.gz as advertized. Any chance this is going to be applied for the next release? Will be a great additional feature for high traffic production. I can't live with a horribly slowing down site while I still need tinymce installed for a reason :)

Thanks to all.

@gausarts
I guess it can happen ("Any chance this is going to be applied for the next release?") if 1) people report RTBC status, and 2) maintainer notices that and commits to CVS.

Since you tested it, you can change the status to RTBC. While I tested it, I also wrote the patch, so I can't do RTBC... or can I?

I didn't test this patch, but at least the coding style needs work. See http://drupal.org/coding-standards and sub-pages for details.

This is exciting news when there is a hope :) I was so discouraged because it was set won't fix a couple of times while I was driven here from 2 tinymce modules. With this gzip, the page doesn't hang forever upon refresh any longer, and cuts down 200-400% download time. So I would be very grateful if it can materialize one way or another.

Thanks again.

@sun
"I didn't test this patch, but at least the coding style needs work."
Appreciate the feedback, but I can't use it.
- can you be more specific what in the coding style of the patch needs work? what line number(s)? Pointing me to the main document and all its sub-pages does not help, as I studied it thoroughly and believe I follow it to the letter.

No problem. :)

+++ wysiwyg/editors/js/tinymce-2.js	2010-03-04 08:21:08.000000000 -0800
@@ -11,19 +11,37 @@
+  do_preinit = false;

(and elsewhere) Missing "var" statement.

+++ wysiwyg/editors/js/tinymce-2.js	2010-03-04 08:21:08.000000000 -0800
@@ -11,19 +11,37 @@
+  if (settings.global.execMode == 'gzip') {
+    do_preinit = true;
+  }
+  else {
...
+    if (do_preinit) {
+      do_preinit = false;

This entire do_preinit handling needs much more docs, because I do not understand the effect of these changes.

It seems like you're trying to avoid setting properties on the tinyMCE object if the loaded variant is gzip, in which case you only initialize the editor once for the entire lifetime of this JavaScript.

What's the initial state of tinyMCE.baseURL|srcMode|gzipMode ? If it's null, then we could possibly save us some conditional code structures here and just use

tinyMCE.foo = tinyMCE.foo || blah;

Either way, I do not understand yet, why only the first instance of tinyMCE needs a proper baseURL and stuff...? Thus, we need better inline docs in any way.

+++ wysiwyg/editors/js/tinymce-2.js	2010-03-04 08:21:08.000000000 -0800
@@ -11,19 +11,37 @@
+    if (settings.global.execMode == 'gzip') {
...
+    } else {

"else" should live on a separate line.

+++ wysiwyg/editors/js/tinymce-3.js	2010-03-04 08:24:58.939534502 -0800
@@ -12,23 +12,45 @@
+      // @see #454992: drupal_get_js() must not use 'q' as query string.
+      if (tinymce.query == 'q') {
+        tinymce.query = '';
+      }

Same as above: tinymce.query should never contain 'q' as query string, for all instances on a page.

It seems like this code is trying to re-use a magical setup global tinyMCE object that may be instantiated by the gzip compressor?

Did you test this with Drupal plugins? (e.g. Teaser break)

+++ wysiwyg/wysiwyg.admin.inc	2010-03-03 23:46:16.000000000 -0800
@@ -98,6 +98,28 @@ function wysiwyg_profile_form($form_stat
+  // Prepare list of installed libraries
+  foreach ($editor['libraries'] as $library => $files) {
+    $notfound = false;
+    foreach ($files['files'] as $file => $options) {
+      if (!is_array($options)) $file = $options;
+      if (!file_exists($editor['library path'] . '/' . $file)) {
+        $notfound = true;
+		break;
+      }
+    }
+    if (!$notfound) $libraries[$library] = $files['title'];
+  }
+  if ($libraries) {
+    $form['basic']['library'] = array(
+      '#type' => 'select',
+      '#title' => t('Library'),
+      '#default_value' => $profile->settings['library'],
+      '#options' => $libraries,
+      '#description' => t('The library to use for the editor interface.'),
+    );
+  }

Minor: Tabs here.

Major: This hunk/code doesn't belong into this patch/issue. I can understand that it was introduced to test this patch, but proper handling of library variants is a separate issue (which already exists in the queue, but will likely be deferred to 3.x + integration with Libraries API).

Powered by Dreditor.

@sun
Thanks! I will clean up the code... But it looks like you have a "Major:" objection besides the coding style points.

The whole compressor thing will not work without the selector part, since it won't be selectable. Therefore I patched the selector. I did not realize it was so complicated issue that I should not touch it ;). And it looks to me that there is no testcase for the selector as it is now now, and as you say it is deferred to 3.x. Is that a block for this issue to be resolved - will you not commit anyway even if I cleanup? If you're not going to commit anyway, I won't bother to cleanup the coding style.

Here is one other minor rebuttal:

+++ wysiwyg/editors/js/tinymce-3.js	2010-03-04 08:24:58.939534502 -0800
@@ -12,23 +12,45 @@
+      // @see #454992: drupal_get_js() must not use 'q' as query string.
+      if (tinymce.query == 'q') {
+        tinymce.query = '';
+      }

This +++ is just an indent of existing code (see previous --- in the patch). Don't ask me why it is there.

"This entire do_preinit handling needs much more docs, because I do not understand the effect of these changes."
I agree with that. It may be a very long comment... I'll pass it in a short form by you here:
In a nutshell, compressor introduces a stub js file which makes a call to a server-side php file that serves a compressed image but first checks if it exists, and if not - generates it. This introduces a complexity of when tinyMCE global variables / settings can be initialized. If compressor is not used, the original sequence is used, setting these variables before tinyMCE.init call. When the compressor is used, we postpone initializing of the variables until the compressed image is loaded (after tinyMCE_GZ.init call). We only want to configure these variables once, therefore do_preinit is reset to false.

I missed "var" on do_preinit and will have to fix it, since do_preinit is not global.

This patch has a chance to get in, but library variant selection is a separate and much more complex issue, which has to be tackled separately over in #374391: Allow to configure editor library variant to load -- for more details about why variant handling is more complex, you may also read #719896: Add a hook_libraries_info() (Libraries API)

Until variants are properly supported, you can use the new hook_wysiwyg_editor_settings_alter() to adjust the variant to load.

I'll take that chance. Updated patch is attached, with var do_preinit fix, comments regarding do_preinit, and couple of style touch-ups per sun.

Re: "Did you test this with Drupal plugins? (e.g. Teaser break)"
Yes. Attached is a screenshot with teaser break after inserting the break. Using _gzip.js as can be seen in the console.

RTBC based on mine and gausarts' tests.

Cleaned this up a bit + added a @todo that I do not understand.

Also, let's leave TinyMCE 2.x untouched. We need to consider to remove support for that crappy old version at some point.

I guess that this is more likely what you want. However, now there are 2 @todos.

oopsie, killed too much ;)

@sun
Re: tinymce-2, I don't care for it either, but it is not on this issue to remove it, so I maintained the functionality.

I can see where you going with your changes in #27 and #28, but I'm not sure it will work. First, how 'gzip' is supposed to be selected? Second, the settings.global would not be copied to tinyMCE.* if there is only one pass through "for (var format in settings)" loop. And it is mostly the case on regular nodes.

I can agree that each instance needs its settings, but it looks like these tinyMCE.* variables carry globals, and settings[format] is passed directly to *.init() call.

I'm not sure I understand why you want to mess with the initialization sequence. I tried touching it only to discover that it opens a can of worms.

1) As mentioned before, library variant selection cannot be part of this issue/patch. Although we are registering the possible variants already, Wysiwyg does not provide means to select the variant to load, since it is missing library variant detection, which simply means that users can wreck their site by configuring a variant that does not exist. Additionally, users would be able to configure a different variant for each profile currently, which would mean that we'd load the same library in different variants on the same page. We won't introduce a new feature that is known to be broken. Everyone is welcome to join forces on Libraries API, so we are able to entirely ditch Wysiwyg's precursor implementation in favor of Libraries API. Until then, power users are able to globally alter the variant to load via custom code.

2) Yes, that's one of the parts of this patch I don't understand. It seems to dismiss the global instance settings under certain circumstances. The reason why I added the @todos, which need to be tested + most likely still have to be resolved. Furthermore, IIRC, the gzip compressor takes the instance settings to dynamically aggregate a set of files. The current patch invokes that aggregator only once for the first configured profile, which ultimately means that subsequent profiles will only have plugins of the first profile available/loaded...?

3) This patch effectively messes with the initialization sequence. We therefore need an implementation that's based on solid knowledge of the underlying initialization code, so as to not break the existing implementation.

@sun,
1) So a power user is needed who will know how to select 'gzip' variant through custom code. Without providing that custom code, I don't think you will find many users willing to test it. I don't know how for one. I won't mind the commit split out, but at least give a recipe how to select the gzip variant. The way I see it - having a selector box is the most intuitive way. Select box may actually seed some development for that Libraries issue. If it later is redesigned as part of Libraries issue and adds more functionality, that's no problem for gzip - it will be a good test case / regression test.

2). "It seems to dismiss the global instance settings under certain circumstances." - no, it moves it after the tinyMCE_GZ.init() call. If you ask me why - I don't have whole insight into workings of gzip loader and tinyMCE.init(), but I spent some time testing various approaches, and this was the only one that worked.
Your point on the aggregation makes sense, however some plugins are loaded individually (I see a bunch of editor_plugin.js loaded separately), so gzip loader & tinymce core seem to be taking care of incremental plugin loads. Given that, I'm not sure if it's viable to call tinyMCE_GZ.init() every time instead of tinyMCE.init(), since plugins are smaller than the core that gets compressed and sent once. Asking to resend the core for each configuration seems to be a bigger burden than have few uncompressed plugins loaded as needed.

3). In my patch I made a point to preserve the init sequence for non-gzip variant, and figured out the quirks around gzip variant that sets globals properly. Your above patches most certainly won't get the globals right for gzip case, but you are welcome to try it for yourself.

I have tested last #29 patch (using patch from #862100: Allow to configure the library/execMode to use for easing the exec mode selection)
The only problem is with

  // @see #454992: drupal_get_js() must not use 'q' as query string.
  if (tinymce.query == 'q') {
    tinymce.query = '';
  }

Where the tinymce is not yet defined when it runs and fails,
but checking #454992: JavaScript query string breaks $_GET['q']
it seems that is already fixed and then removing that part of the code the compressed version works genially.

It creates a different gziped file for each defined profile with only the selected plugins, languages or themes.
A quick test with my local development server, without caches, for a normal configuration:
standard Exec Mode : It has a size of 163.8 kB and loads in 985ms
compress Exec Mode: it has a size of 55.8kB and loads in just 31ms.
(really amazing)

I have tested it with js/css compression on and off.

New patch

Niiice, testing right now :)

subscribe

any patch for 7 branch ?

i use patch from #33 and #862100: Allow to configure the library/execMode to use when i choose compressed mode tinyMCE upload but not executed - text area as plain text

patch in #33

patching file editors/js/tinymce-3.js
Hunk #1 FAILED at 12.
1 out of 1 hunk FAILED -- saving rejects to file editors/js/tinymce-3.js.rej
can't find file to patch at input line 55

but i look at the code and i see that tiny_mce_gzip.js was loaded but not executed - text area as plain text

i use Compressor 2.0.4 PHP

A small piece of code was changed in tinymce-3.js, causing the patch to fail.
I've re-rolled it for D7 but it should apply for D6 as well. (Apply the patch from #862100: Allow to configure the library/execMode to use first, if you haven't already.)
I've not noticed any problems so far, even with JavaScript Aggregation enabled, but this needs some testing with a couple of plugins provided by other modules just to be sure.

great work ! thanks for your quick help

i use #38

maybe something is incorrect in patch, but work perfect.

#patch < wysiwyg-tinymce-gzip-276465-38.patch
can't find file to patch at input line 5
Perhaps you should have used the -p or --strip option?
The text leading up to this was:
--------------------------
|diff --git a/editors/js/tinymce-3.js b/editors/js/tinymce-3.js
|index b04a4ce..db3ab63 100644
|--- a/editors/js/tinymce-3.js
|+++ b/editors/js/tinymce-3.js
--------------------------
File to patch: editors/js/tinymce-3.js                             
patching file editors/js/tinymce-3.js
can't find file to patch at input line 51
Perhaps you should have used the -p or --strip option?
The text leading up to this was:
--------------------------
|diff --git a/editors/tinymce.inc b/editors/tinymce.inc
|index b88f07f..d3cc715 100644
|--- a/editors/tinymce.inc
|+++ b/editors/tinymce.inc
--------------------------
File to patch: editors/tinymce.inc
patching file editors/tinymce.inc

i test it with Chrome and Firefox
Main packages (3.4.x)
with TinyMCE core (standalone)
and TinyMCE core (jQuery) (the difference is not felt)

with JavaScript Aggregation enabled

wjth this http://drupal.org/project/agrcache

and jQuery Update

all work don't forget Clear cache.

I noticed that there are files:
/sites/all/libraries/tinymce/jscripts/tiny_mce/langs/en.js
/sites/all/libraries/tinymce/jscripts/tiny_mce/themes/advanced/editor_template.js
/sites/all/libraries/tinymce/jscripts/tiny_mce/themes/advanced/langs/en.js

not aggregated and and slow to load
with css same problem

is this normal of security reason that tiny_mce_gzip.php load on page load and available by the url ?

@protools, If you use the patch utility instead of git apply, you'll need to do patch -p1 < file.patch to have it ignore the first a/ part of the paths and find the files automatically.

See my reply in #1346822: css and js are not aggregated and duplicate - performance improvement regarding the non-aggregated files. The editor controls which files are loaded and how, bypassing Drupal completely, so there's not much we can do about it.

TinyMCE calls tiny_mce_gzip.php to get the aggregated files core files/plugins so it must be possible to access it from the client. I can't vouch for how secure the code in it is though. You'll have to consult the TinyMCE forum for that. (A quick look through the code did not reveal any obvious security issues as far as I can tell.)

Just confirming that this patch seems to work along nicely with the current version.
Thanks!

Wanted to re-roll this patch, but the code has changed too much since. If anyone has an updated patch, please share.

Sorry, I don't have any bandwidth to do it now.