Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent https://www.gstatic.com/charts/loader.js URL from minification #202

Merged
merged 1 commit into from
Sep 5, 2018

Conversation

yyevgenii
Copy link

This PR is a duplicate of the #91 which was overridden by @tnikcevs in #92

The fix for a dashboard graph:
When 'JS minification' is enabled in config (Stores>Configuration>Advanced>Developer>JavaScript Settings>Minify JavaScript Files = Yes),
Magento2 tries to load all resources (that loaded via require.js) using .min.js suffix and URL, which is used for building graph (https://www.gstatic.com/charts/loader.js)
will be replaced by 'https://www.gstatic.com/charts/loader.min.js' and the customer gets 404.
Here is an example: magento/magento2#5835 (comment)

@vvuksan vvuksan merged commit 379f924 into fastly:master Sep 5, 2018
@hostep
Copy link

hostep commented Sep 5, 2018

@yyevgenii & @vvuksan : this is the wrong way to do this, can we please please revert this, it will break the tinymce editor in the backend of Magento when you do this and have minify js enabled.

But first verify if that's actually the case, I'm pretty sure it will, since I'm speaking from a lot of experience here.

More context in this thread: magento/magento2#11577 (comment)

And a fix which comes in Magento 2.3, which will allow you to do what you proposed, but will require you to add an extra level of xml nodes, then the minify_exclude values will be actually merged instead of being overwritten: magento/magento2#13687

To have it fixed in Magento 2.1 and 2.2, the best way I've found is by using a plugin, like this:
https://magento.stackexchange.com/a/198480/2911

@vvuksan
Copy link
Contributor

vvuksan commented Sep 6, 2018

It has been reverted.

@yyevgenii
Copy link
Author

Thank you @hostep and @vvuksan

@yyevgenii yyevgenii deleted the excludejsurl branch September 6, 2018 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants