This function takes around 3% of page execution time on node view (due to the comment form) on a D7 site I'm developing. Seems very cacheable.

Comments

smk-ka’s picture

StatusFileSize
new5.99 KB
new6 KB

Nice but didn't work for me, since we still need to include the files when loading lists from cache in wysiwyg_load_all_includes(). Also added reset flags at appropriate places, which are triggered on the wysiwyg profiles admin pages to reset the cached editor and plugin lists. Patches for D7 and D6 attached.

catch’s picture

StatusFileSize
new5.42 KB

Still applies, re-rolled d7 patch for fuzz/offset/windows line breaks.

sun’s picture

StatusFileSize
new5.44 KB
helmo’s picture

Status: Needs review » Reviewed & tested by the community

Looks good. And coder module has no complaints.

twod’s picture

Won't this break plugins which add additional scripts in hook_wysiwyg_plugin(), like media module does?
For reference: #1039076: Allow plugins to specify libraries and multiple javascript files

helmo’s picture

I did a quick test with the media module. It's media browser icon still works fine with the patch.

wysiwyg_get_all_plugins already has some caching, this is more about the lower level of which include files to load.

sun’s picture

Status: Reviewed & tested by the community » Fixed

Thanks for reporting, reviewing, and testing! Committed to 7.x-2.x.

A new development snapshot will be available within the next 12 hours. This improvement will be available in the next official release.

acrollet’s picture

Status: Fixed » Needs work

Sorry to report that #5 is correct, this change breaks the media wysiwyg plugin. (It works on the first page load after cache clear, but is broken for every page load thereafter, which is perhaps why helmo didn't see the problem) Would it be possible to add a 'cachable' parameter to the plugin definition, or alternatively a 'additional_js_callback' parameter?

sun’s picture

Priority: Normal » Critical
Status: Needs work » Active
Issue tags: +Release blocker
sun’s picture

Hrm. Do we need to roll this back until #1039076: Allow plugins to specify libraries and multiple javascript files has been fixed and postpone it until Media has been changed accordingly and has a new release?

twod’s picture

I say revert and wait. No matter if we get #1039076 in or add another callback as suggested by @acrollet, we'll still have to wait for Media to catch up, and they may not even be aware of the changes yet. To be sure they are, I created #1795968: Prepare for wysiwyg_load_includes being cached.
I hope they will be able to review the patches I added to #1039076 and spot any issues I've overlooked. If they think it looks good, we can commit that patch and re-commit this patch, and preferably sync the next release of both modules.

I'll make a sweep of the modules I know have plugins and check if they need to be changed as well.

sun’s picture

Category: bug » task
Priority: Critical » Major
Issue tags: -Release blocker +Performance

Reverted the commit.

socialnicheguru’s picture

Issue summary: View changes
Status: Active » Needs review

any update on this? I would like to cache all the js and css associated with the tiny_mce library that is loaded as part of the module

twod’s picture

@SocialNicheGury The situation has not really changed since the other patch mentioned in #10 is not yet committed.
The next release will be regression fixes, after that I will look at implementing new features.

The patch here will not actually cache the CSS and JS needed by editor libraries, only the meta-information about which files are needed.
We've discovered not many editor libraries so far can handle being aggregated by Drupal. The problems usually come down to missing semi-colons, which cause syntax errors when removing newlines. There are sometimes other problems as well, such as libraries trying to find their base path by searching for the script tags which include them, and otherwise failing to load assets.

We do try to aggregate library files where possible but due to differences between library versions and the many per-version overrides that would lead to we usually have to default to not doing so. You could try implementing hook_wysiwyg_editor_alter() to modify the editor definition and in there the options it passes along to drupal_add_js() if you wish to experiment.

socialnicheguru’s picture

@TwoD - thanks for the explanation. I am profiling my install now. I notice that the tinymce wyswig libraries are taking forever to load.

twod’s picture

Do you mean the integration files under wysiwyg/editors/js?
If so, those should be aggregated with the rest of Drupals scripts when needed. Above I was only talking about the actual library files in sites/all/libraries/tinymce/... Those can often not be aggregated, but like I said you can try for your current version of TinyMCE by implementing hook_wysiwyg_editor_alter() and changing the library files options array.
You'll likely need to change it somewhere at $editors['tinymce']['versions']['4']['libraries']['min']['files']; , if you're using TinyMCE 4. It's currently just an array with a string, but that array basically allows for all options you can send to drupal_add_js() by changing the current string value to be a key in an associative array where the value is an array passed along to drupal_add_js, see $editors['tinymce']['versions']['3.3']['libraries']['']['files']['tiny_mce.js']; .