EnlighterJS / Plugin.WordPress

:package: Official WordPress Plugin of EnlighterJS
http://wordpress.org/plugins/enlighter/
GNU General Public License v2.0
115 stars 17 forks source link

Double slash used inside DRI and theme customizer URLs #294

Closed zaneclaes closed 3 years ago

zaneclaes commented 3 years ago

This bug is comprised of three smaller bugs which have ultimately broken Enlighter for my site.

First, the Javascript URL used by the includes is wrong. It has two slashes: enlighter//resources (which does not work with my front proxy).

Second, it does not respect my custom cache (CDN) settings which I configured from the EnlighterJS WP settings. The only way to get Enlighter to pull from the correct domain is to completely override ENLIGHTER_PLUGIN_URL, which is not a good solution because it may have unintended consequences.

Third, it does not work with CDN plugins. Even with DRI off and "Single File," I can see in the Chrome network tab that it is trying to load from my domain (www). This should not happen for two different reasons.

  1. Autoptimizer should have compressed the file into the JS bundle.
  2. W3 Offload Media should have rewritten the www. to my CDN.

The fact that neither happened suggests that it is not playing nicely with other hooks/actions.

AndiDittrich commented 3 years ago

Hi @zaneclaes ,

  1. i'm unable to reproduce your observed behaviour regarding the enlighter/resources path. both frontend and backend resource URLs are working as expected (dev env without reverse proxy, production enc with reverse proxy). It sounds like a specific error within your environment (maybe caused by overriding ENLIGHTER_PLUGIN_URL)

  2. please post your configuration what you've used as cache path+url - in case the plugin detects that the path is not accessible these settings are ignored (you will see a warning message)

  3. Enlighter is using the standard wordpress enqueue hooks - therefore it must be a third party issue. Generally i can not recommend to merge EnlighterJS with other libraries - it doesn't give you any performance gain..

  4. Technically, the DRI feature cannot work with optimization plugins

zaneclaes commented 3 years ago
  1. I did not implement the define('ENLIGHTER_PLUGIN_URL') until after I first saw this problem. I just took a peek at the source code, and the bug is pretty apparent. The Enlighter.php class explicitly defines ENLIGHTER_PLUGIN_URL to include the trailing slash after /enlighter/. Then, when this value is used, it is concatenated to a path in varied ways. Namely, DynamicResourceInvocation.php and ThemeCustomizer.php both include a prefix slash on /resources, whereas ResourceManager.php does not include the prefix slash. Moreover, this sort of naive path concatenation is brittle across filesystems (and even sometimes a potential security vulnerability). Portable software should rely upon native filesystem path building methods (to avoid unexpected character sequences), not concatenate strings together.

  2. To be clear: the CSS worked fine, the JS did not. Again, the bug seems pretty apparent. The DynamicResourceInvocation.php file is explicitly using the ENLIGHTER_PLUGIN_URL. The implementation of the JS importing is different than the CSS just a few lines above, and the two assets are thus constructed differently.

Screen Shot 2020-08-04 at 6 01 11 PM

FWIW, per your request, here are my settings. Note that the CDN cache URL. Store files at: /var/www/html/technicallywizardry/wp-content/plugins/enlighter/cache/ Cache url: https://assets.technicallywizardry.com/wp-content/plugins/enlighter/cache/

3/4. I was merely pointing out that my optimization plugins should have fixed the fact that Enlighter is not using the correct CDN URL, but they did not. They are meant to rewrite www. to assets. wherever they find static assets, like CSS or JS. I've never had them fail me before across a half dozen Wordpress deployments.

I should also mention... you said:

The DRI feature cannot work with optimization plugins

But the actual help page says:

It also works with [...] third party optimization plugins.

Here's a screenshot:

Screen Shot 2020-08-04 at 5 54 29 PM

... so which is it?

AndiDittrich commented 3 years ago

URL issue

thanks, now i'm understanding what's going wrong (this affects only DRI and the theme customizer in the backend) - i'll fix it soon. Technically PHP allows the use of forward slashes also on Windows systems, that's a common practice (the internal WordPress wp_normalize_path normalize it to forward slashes). And of course, using native join+normalize for each path is a good practice - but in PHP there is no native join method and within Enlighter the leading path elements are all normalized.

DRI Explained

The plugin behaves as intended and the invocation is correct. You need a better understanding about how the DRI implementation as well as the resource invocation internally works:

Using DRI with CDN offloading is technically not possible with "standard" plugins because there is no common API in WordPress for this use-case. If it's required you can manually offload the files and enqeue the EnlighterJS resources manually of using one of the filter hooks the change the URL as intended

zaneclaes commented 3 years ago

You need a better understanding about how the DRI implementation as well as the resource invocation internally works:

I understand perfectly well how it works, thank you, it's a beginner concept I've encountered before in my career.

In my OP, on bug number 3, I said:

[...] even with DRI off [...]

In order to point out that I understood the implications of DRI.

So to clarify, my bugs (1) and (2) were with DRI on. I already showed you the source code responsible for these two bugs (the "URL issue" causes both). Bug (3) was with DRI on or off.

Using DRI with CDN offloading is technically not possible with "standard" plugins because there is no common API in WordPress for this use-case.

Sure there is. You're generating the DRI with PHP, as we already established. You just need to run the resource URLs through the appropriate filters so that Wordpress knows what kind of content it is. Have a look at style_loader_src and script_loader_src, which is how CDN plugins rewrite asset URLs.

AndiDittrich commented 3 years ago

If you have any other bulletproof concept to achieve a similar feature with WordPress without using any kind of dependency manager your contribution will be welcome!


Regarding the additional filter hooks: it's not a big deal to add these filters to the DRI resource URLs but i'm unable to test it (not using public CDN plugins..) - in case you'll do some beta testing i can add them soon - otherwise add them on-top of the enlighter resource filters on your own.

But this will only filter the URLs with DRI=on - all other resources are using the internal wp_enqueue_ - if this is not working with your CDN plugin there might be something other wrong...