Open jtomaszewski opened 3 years ago
Hi @jtomaszewski. Thank you for your report. To help us process this issue please make sure that you provided the following information:
Please make sure that the issue is reproducible on the vanilla Magento instance following Steps to reproduce. To deploy vanilla Magento instance on our environment, please, add a comment to the issue:
@magento give me 2.4-develop instance
- upcoming 2.4.x release
For more details, please, review the Magento Contributor Assistant documentation.
Please, add a comment to assign the issue: @magento I am working on this
Join Magento Community Engineering Slack and ask your questions in #github channel.
:warning: According to the Magento Contribution requirements, all issues must go through the Community Contributions Triage process. Community Contributions Triage is a public meeting.
:clock10: You can find the schedule on the Magento Community Calendar page.
:telephone_receiver: The triage of issues happens in the queue order. If you want to speed up the delivery of your contribution, please join the Community Contributions Triage session to discuss the appropriate ticket.
:movie_camera: You can find the recording of the previous Community Contributions Triage on the Magento Youtube Channel
:pencil2: Feel free to post questions/proposals/feedback related to the Community Contributions Triage process to the corresponding Slack Channel
i think this problem related with magesuite. We need refactor with template code for such case
Why would that be magesuite's problem? Do you think it alters anyhow the way.phtml
files are minified?
I'm not sure but this problem can be reproduce in vanilla code base magento without any 3rd-party modules/themes ? Maybe with approach php comments like this in one place will break other code after it. Should avoid use double backslash in comment , especially when have 2 php block sit next to each other
I'm not sure but this problem can be reproduce in vanilla code base magento without any 3rd-party modules/themes ? Maybe with approach php comments like this in one place will break other code after it. Should avoid use double backslash in comment , especially when have 2 php block sit next to each other
This code is a valid php code - minifying it shouldn't break it.
Not sure if it can be reproduced in vanilla magento, unfortunately I don't have time now to set one up.
We got similar issue with the file. Had to remove problem part of file
vendor/dotpay/magento2-payment/view/adminhtml/templates/system/config/information.phtml
This issue has been automatically marked as stale because it has not had recent activity. It will be closed after 14 days if no further activity occurs. Is this issue still relevant? If so, what is blocking it? Is there anything you can do to help move it forward? Thank you for your contributions!
Since this problem may related from another modules and not related with core modules Can we close this?
@mrtuvn: I strongly disagree. This is valid PHP code. The html minifier that Magento uses should be able to handle this. It doesn't matter if such code is part of custom code or not.
Also: please don't add a label "Cannot Reproduce" if you haven't properly tested this.
i'm sure with tested in vanilla magento setup in minify mode. Should be update from 3rd-party modules mentioned for work with the case minify
https://github.com/magesuite/theme-creativeshop/blob/v11.3.0/src/Magento_Catalog/templates/addtocart/button.phtml By the way this issue mentioned by author already fixed by magesuite team
But it's still a bug in core Magento's html minifier, people can waste many hours in researching why at a certain point their perfectly valid custom code is getting corrupt by the html minifier...
How can it be 3rd-party modules fault that valid PHP code is not properly handled by the html minifier? Any and all valid PHP code should be handled by the minifier, if it cannot it is clearly a bug.
Just because 3rd party worked around it by modifying their source it does not mean the minifier is bug-free. I've seen at least 3 modules that ran into this bug with newer magento versions.
Edit: I agree with hostep: It is very difficult to debug and not always catched by CI/CD processes, especially if it happens in templates that are not easily tested like checkout or other.
ok i agree with your point if magento phtml renderer can handle that minify case related with this issue report. But that may not easy to fix. @hostep If you have solution feel free to create pull request for such issue
The problem comes from this set of commits, but it is likely that any attempt to fix it will in turn bring some other problems, because regexes are not the right tool for the job.
Could a solution based for example on nikic/php-parser be considered instead?
Hi @Den4ik. Thank you for working on this issue. In order to make sure that issue has enough information and ready for development, please read and check the following instruction: :point_down:
[ ] 1. Verify that issue has all the required information. (Preconditions, Steps to reproduce, Expected result, Actual result).Details
If the issue has a valid description, the label Issue: Format is valid
will be added to the issue automatically. Please, edit issue description if needed, until label Issue: Format is valid
appears.
[ ] 2. Verify that issue has a meaningful description and provides enough information to reproduce the issue. If the report is valid, add Issue: Clear Description
label to the issue by yourself.
[ ] 3. Add Component: XXXXX
label(s) to the ticket, indicating the components it may be related to.
[ ] 4. Verify that the issue is reproducible on 2.4-develop
branchDetails
- Add the comment @magento give me 2.4-develop instance
to deploy test instance on Magento infrastructure.
- If the issue is reproducible on 2.4-develop
branch, please, add the label Reproduced on 2.4.x
.
- If the issue is not reproducible, add your comment that issue is not reproducible and close the issue and stop verification process here!
[ ] 5. Add label Issue: Confirmed
once verification is complete.
[ ] 6. Make sure that automatic system confirms that report has been added to the backlog.
Hi team.
Issue really exists.
Magento correctly remove inline comments due to minification, but if comment contain html tags like <span>
in example you will see issue.
Same issue will be presented for comments that contain html tags in multiline format but in one string /** Test comment <span> */
Ran into same issue with 2.4.2 CE and Minify HTML option enabled. For now removed the comments from the .phtml file I was seeing this error.
Same here with heredoc in phtml files. It seems we may have to downgrade or otherwise patch thise.
Compressing PHP code generally makes not much sense and leads to such issues. Removing newlines does not make the PHP interpreter faster.
Could a solution based for example on nikic/php-parser be considered instead?
Is definitely better maintained by one of the core PHP maintainers (Nikita) but keep in mind that it will probably remove comments and a few other things. At least the parser by Nikita does not try to minify PHP code and does not remove newlines.
Regarding the 3 commits (https://github.com/magento/magento2/commit/b308c33d0e0eeebbe5e3019263d4fcc724f36c86, https://github.com/magento/magento2/commit/4ecc5d2eb3b55c61fd4f013579da0232a79bd900, https://github.com/magento/magento2/commit/000a1d3bc0e887814d3ba72e2523d3036f6d0665), I will try to revert them locally with some patch files using composer-patches.
Dear Magento 2 devs, please do not reinvent the wheel with such a problematic minifier, regex is voodoo and should be properly tested.
@DanielRuf: does https://github.com/magento/magento2/pull/33016 solve your issue?
@DanielRuf: does #33016 solve your issue?
We should get rid of this weird template-minifier in general.
I'm currently testing this patch for vendor/magento/framework
:
diff --git a/View/Template/Html/Minifier.php b/View/Template/Html/Minifier.php
--- a/View/Template/Html/Minifier.php
+++ b/View/Template/Html/Minifier.php
@@ -3,7 +3,6 @@
* Copyright © Magento, Inc. All rights reserved.
* See COPYING.txt for license details.
*/
-declare(strict_types=1);
namespace Magento\Framework\View\Template\Html;
@@ -141,10 +140,10 @@
. '(?:<(?>textarea|pre|script)\b|\z))#',
' ',
preg_replace(
- '#(?<!:|\\\\|\'|"|/)//(?!/)(?!\s*\<\!\[)(?!\s*]]\>)[^\n\r]*#',
+ '#(?<!:|\\\\|\'|")//(?!\s*\<\!\[)(?!\s*]]\>)[^\n\r]*#',
'',
preg_replace(
- '#(?<!:|\'|")//[^\n\r<]*(\?\>)#',
+ '#(?<!:|\'|")//[^\n\r]*(\?\>)#',
' $1',
preg_replace(
'#(?<!:)//[^\n\r]*(\<\?php)[^\n\r]*(\s\?\>)[^\n\r]*#',
I will test this patch now and will look at the linked PR afterwards. But in general: stop doing this regex stuff, this is black magic and voodoo at the least.
@DanielRuf: does #33016 solve your issue?
Thanks, I took a brief look. It seems only a part uses the parser from Nikita and this patch is bigger than I can currently test at the moment (not much time since I only work on the frontend side). Not sure if this will cover our issue (we use Hyva Theme, a child theme based on it and there we have heredoc statements for their modals in two template files).
"Improve template minifier" is not very descriptive since it is mostly replaced with a correct parser and minifier instead of relying on regex tricks ;-)
Still testing. Not sure if https://github.com/magento/magento2/commit/74727b2515d9b17f650bcf7a581d06a8c2358616 is also relevant in our case regarding heredoc statements.
Still testing. Not sure if 74727b2 is also relevant in our case regarding heredoc statements.
Ok, seems we also have to revert this one too since the heredoc statements are still compressed / on one line.
Still testing. Not sure if 74727b2 is also relevant in our case regarding heredoc statements.
Ah, now I see. Hyva Themes uses inline style like this:
->methodName(<<<END_OF_CONTENT
sdfsdf
END_OF_CONTENT
)
So there is no trailing semicolon since this is optional by design.
See https://www.php.net/manual/en/language.types.string.php#language.types.string.syntax.heredoc
So '/<<<([A-z]+).*?\1?;/ims',
is probably the correct code.
Besides this we will still revert the other three commits due to the reported problems here in the issue.
Final patch that works for us:
diff --git a/View/Template/Html/Minifier.php b/View/Template/Html/Minifier.php
--- a/View/Template/Html/Minifier.php
+++ b/View/Template/Html/Minifier.php
@@ -3,7 +3,6 @@
* Copyright © Magento, Inc. All rights reserved.
* See COPYING.txt for license details.
*/
-declare(strict_types=1);
namespace Magento\Framework\View\Template\Html;
@@ -119,7 +118,7 @@
//Storing Heredocs
$heredocs = [];
$content = preg_replace_callback(
- '/<<<([A-z]+).*?\1;/ims',
+ '/<<<([A-z]+).*?\1?;/ims',
function ($match) use (&$heredocs) {
$heredocs[] = $match[0];
@@ -141,10 +140,10 @@
. '(?:<(?>textarea|pre|script)\b|\z))#',
' ',
preg_replace(
- '#(?<!:|\\\\|\'|"|/)//(?!/)(?!\s*\<\!\[)(?!\s*]]\>)[^\n\r]*#',
+ '#(?<!:|\\\\|\'|")//(?!\s*\<\!\[)(?!\s*]]\>)[^\n\r]*#',
'',
preg_replace(
- '#(?<!:|\'|")//[^\n\r<]*(\?\>)#',
+ '#(?<!:|\'|")//[^\n\r]*(\?\>)#',
' $1',
preg_replace(
'#(?<!:)//[^\n\r]*(\<\?php)[^\n\r]*(\s\?\>)[^\n\r]*#',
Now seriously, Magento 2 should only compress / minify files after they were processed by PHP (source => data => result as static html => compress / minify).
Please don't minify / touch source files which contain PHP code. That makes absolutely no sense and leads to such (corner) cases where it leads to problems.
Would Hugo or any other static site builder do the same then this would lead to broken sourcecode.
@DanielRuf Magento has ton of files and I believe that including minified files is better solution than minify result html. Also I saw many sites where some modules breaks cache and solution that you recommend totally kill this sites
And as you mentioned best solution is use great theme like Hyva and spent much time for optimize site performance, deployment process and so on.
PS. Regex is not a vodoo magic if you know how work with it ;)
And as you mentioned best solution is use great theme like Hyva and spent much time for optimize site performance, deployment process and so on.
We already use Hyva ;-)
@DanielRuf Magento has ton of files and I believe that including minified files is better solution than minify result html
Well, as you can see specific corner cases like optional trailing semicolon and others are not (yet) covered.
PS. Regex is not a vodoo magic if you know how work with it ;)
Doesn't seem to be the case here as the reverted commits are just trying around with regex (same commit message) and have no clear explanation what they should do and use very complex code which has ultimately lead to these issues.
One example to show that there worked people with not that much experience:
<<<([A-z]+).*?\1;/ims
The i
flag is not needed. See https://regex101.com/r/jECWPk/1
Sorry to say that but this minifier is just too complex, introduces too many issues, minification in general should not work like this (PHP does not run faster by trying to compress the HTML code) and the code is not really documented (especially the regular expressions) and extremely hard to review and understand.
You have my full respect if you understand and if you can explain in simple words what all these regular expressions in this file do, what they cover (especially corner cases) and what the last changes did in detail.
Hi all, that's a related issue: #34880
Patching last regex like:
'#(?<!:)//[^\n\r]*(\<\?php)[^\n\r]*(\s\?\>)[^\n\r]*#'
'#(?<!:)//[^\n\r]*(\<\?php)[^\n\r\s]*(\s\?\>)[^\n\r]*#'
fixes it
hello,
as an addition to this discussion, I had the case of a phtml template file containing an inline comment starting with a #
instead of a classic //
once minified, as expected, all PHP code between the #
and the end of the PHP block was commented, leading to errors
I can see in https://github.com/magento/magento2/blob/2.4-develop/lib/internal/Magento/Framework/View/Template/Html/Minifier.php that //
is handled but not #
Replace it with nikic/php-parser as someone says.
In the meantime worth noting that strings containing "//" are also caught right now, which you could not really fix by modifying the regex. e.g.:
<?php
$title = iconv('UTF-8', 'ASCII//TRANSLIT//IGNORE', $block->getTitle());
?>
<h1><?=$title?></h1>
results: <?php $title = iconv('UTF-8', 'ASCII?><h1><?=$title?></h1>
Clearly this should encourage the good behaviour of adding a function to the block but I do agree with other commenters that if the code is valid a regex should not break it.
After upgrading from Magento 2.4.1 to 2.4.2 we started getting such an error:
It happens only when html minification is enabled.
This happens for the following file:
It gets minified to this:
After we've split the minified file into several lines, we found that the error comes down to this part:
The original code for this looks like this:
So as you can see, it incorrectly replaces
<?php // ... ?> <?php
with<?php <?php
.It should either drop the php comment line completely, or leave it untouched with its' closing
?>
php tag, which it currently "swallows".Preconditions (*)
Steps to reproduce (*)
Run magento with that file in a html minification mode.
Current workarounds
Change your
.phtml
files so that they do not contain<?php // ... ?>
lines at all, or at least, change their comment format to a different one (e.g./* ... */
asterisk one).