search

cloudflare / Cloudflare-Magento

A Cloudflare plugin for Magento2.
https://www.cloudflare.com/integrations/magento/
BSD 3-Clause "New" or "Revised" License
50 stars 17 forks source link

PI-1234 fix magento auto-minify bug #64

Closed thellimist closed 6 years ago

thellimist commented 7 years ago

@garrettgalow

navarr commented 7 years ago

After doing some more research, this shouldn't be merged in. config.xml definetly just defines default values for configuration, and what this does is it overrides Magento's default of excluding /tiny_mce/ from minification to add CFs.

Adding both in this would work but it would also not be a good solution.

So there are one of two appropriate ways to tackle the same problem:

  1. Write an UpgradeData file that modifies the config using ScopeConfigInterface (not great)
  2. Write an around interceptor for \Magento\Framework\View\Asset\Minification::getExcludes($contentType) that simply adds Cloudflare's to it if the $contentType is 'js'

I think Option #2 is best, as it means that if removed there are no harsh side-effects from the data, and it should play well with any other extensions that are taking any of the other approaches.

thellimist commented 7 years ago

Adding both in this would work but it would also not be a good solution.

@navarr can you explain why this would not be a good solution compared to option #2?

navarr commented 7 years ago

@thellimist Another module with a similar problem could change the same default through an upgrade script, and if done poorly remove Cloudflare's change.

Additionally, it'd leave Cloudflare's change in the database even if the module is uninstalled

navarr commented 6 years ago

Looks like #72 has been merged in to "fix" this issue.

manatarms commented 6 years ago

Declining because fixed in #72