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

Change the way the minify_exclude configurations can be used. #13687

Conversation

hostep
Copy link
Contributor

@hostep hostep commented Feb 15, 2018

Description

This is a first proposition at changing how the configuration value minify_exclude can be used.
I have the feeling Magento's purpose of this configuration value was to be able to be used in multiple modules. But currently this is not possible, since it is a simple value in the xml config. Which means that when multiple modules define this configuration, the value of the last module in the sequence wil be used. I think Magento's purpose was to make this configuration mergeable, which it currently isn't.

Concrete example, module A defines:

<config>
    <default>
        <dev>
            <js>
                <minify_exclude>
                    /regex1/
                </minify_exclude>
            </js>
            <css>
                <minify_exclude>
                    /regex1/
                </minify_exclude>
            </css>
        </dev>
    </default>
</config>

and module B defines:

<config>
    <default>
        <dev>
            <js>
                <minify_exclude>
                    /regex2/
                </minify_exclude>
            </js>
            <css>
                <minify_exclude>
                    /regex2/
                </minify_exclude>
            </css>
        </dev>
    </default>
</config>

Then the result of this should be that these values get merged, but now they are overwritten by one of the two modules and only /regex1/ or /regex2/ is used depending on which module is loaded as last.

In practice, I've encountered modules which add a single line to this configuration, which then removes the default /tiny_mce/ regex which in turn causes the wysiwyg fields in the backend to no longer work.

This happens because the values of xml nodes are overwritten and not merged when identical nodes are encountered.
The proposition here, is to add a bit more structure to the xml, and to define a node per value, where each node name should be unique as to allow them to be merged.

The current solution to this problem which is circulating is a workaround and not very "user friendly" for developers to implement: https://magento.stackexchange.com/a/198480/2911

Some other remarks:

  • I created this PR against the 2.3-develop branch as this introduces a backwards incompatible change and this PR should not be backported to an older version of Magento.
  • Is it somehow possible that these configuration values are stored in the database? If yes: then we should add an upgrade script as well, to change those database values to the new format.
  • If this gets accepted, this should be clearly noted in the release notes, so that developers are aware of this change
  • I'm pretty sure some tests will fail, and I'll do my best to fix them, but some help might be needed, and maybe some extra tests should be added to verify that the merging of this configuration actually works

I'm open to remarks or other ideas to improve this! :)

Fixed Issues (if relevant)

  1. TinyMCE (WYSIWYG) doesn't work in admin when JS minify is enabled #11577: TinyMCE (WYSIWYG) doesn't work in admin when JS minify is enabled

3rd party modules which run against this problem:

Contribution checklist

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds on Travis CI are green)

@magento-engcom-team
Copy link
Contributor

Hi @okorshenko, thank you for the review.
MAGETWO-87938 has been created to process this Pull Request

*/
private function getMinificationExcludeValues($key)
{
$configValues = $this->scopeConfig->getValue($key, $this->scope) ?? [];
Copy link
Contributor

@arendarenko arendarenko Sep 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getValue returns string.
Why don't you exploding this string like it was in original implementation?
Looks like it's an error.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @alexeya-ven

getValue doesn't always return a string, the return value is being defined as mixed

In this case it returns an array instead of a string, because an extra level of nodes was added in the xml tree. So it's no longer a long string which could consist of multiple lines. But zero, one or multiple xml nodes instead.
By then using array_values, I'm getting the contents of those xml nodes which gives us an array of strings again, like the initial implementation did (which used explode).

The idea was to make this configuration mergeable so you can add multiple exclusions from different modules, which wasn't possible before, one module overwrote the configuration of another module.

I hope this makes it a bit more clear?

@sjgdk
Copy link

sjgdk commented Jul 27, 2019

What's the logic reasoning to add <tiny_mce> tag?

What kind of <tag> I should use if the module is using another 3rd party library? And how specific /regex/ should be use?

@hostep
Copy link
Contributor Author

hostep commented Jul 28, 2019

@wltprgm: the tag name itself is not important, as long as it is unique over all modules which specify one. The contents of the tag however is important, and it should be a regex like you said.

If you can't get it working for some reason, I would suggest you try to debug the method Magento\Framework\View\Asset\Minification::isExcluded and see if your regex matches and maybe by debugging you can figure out why it doesn't match.

Good luck!

@sjgdk
Copy link

sjgdk commented Jul 28, 2019

@hostep
Thanks. I realized what is happening after all, the static content needed to be cleared properly. Here is my question and answer in magento stackexchange.

https://magento.stackexchange.com/q/283539/80197

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants