HTML minification breaks PHP code #32153
Comments
|
Hi @jtomaszewski. Thank you for your report.
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:
For more details, please, review the Magento Contributor Assistant documentation. Please, add a comment to assign the issue:
🕙 You can find the schedule on the Magento Community Calendar page. 📞 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. 🎥 You can find the recording of the previous Community Contributions Triage on the Magento Youtube Channel ✏️ Feel free to post questions/proposals/feedback related to the Community Contributions Triage process to the corresponding Slack Channel |
m2-community-project
bot
added this to Ready for Confirmation
in Issue Confirmation and Triage Board
|
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 |
|
I'm not sure but this problem can be reproduce in vanilla code base magento without any 3rd-party modules/themes ? |
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. |
…s style to avoid Magento html minify error Workaround until magento/magento2#32153 gets resolved
|
We got similar issue with the file. Had to remove problem part of file |
|
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! |
mrtuvn
added
the
Issue: Cannot Reproduce
|
Since this problem may related from another modules and not related with core modules |
|
@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 |
|
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. 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. |
mrtuvn
added
Component: Framework/View
good first issue
and removed
Issue: Cannot Reproduce
|
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. |
|
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? |
Den4ik
added
the
Reproduced on 2.4.x
|
Hi @Den4ik. Thank you for working on this issue.
|
Den4ik
added
Issue: Clear Description
m2-community-project
bot
moved this from Ready for Confirmation
to Confirmed
in Issue Confirmation and Triage Board
Den4ik
added
Priority: P3
m2-community-project
bot
removed this from Confirmed
in Issue Confirmation and Triage Board
|
Hi team. |
m2-community-project
bot
moved this from Good first Issue
to Pull Request In Progress
in Low Priority Backlog
m2-community-project
bot
added
Progress: PR in progress
and removed
good first issue
labels
|
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. |
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. |
|
@DanielRuf: does #33016 solve your issue? |
We should get rid of this weird template-minifier in general. I'm currently testing this patch for 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. |
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 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. |
Ah, now I see. Hyva Themes uses inline style like this: 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 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 ;) |
We already use Hyva ;-)
Well, as you can see specific corner cases like optional trailing semicolon and others are not (yet) covered.
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:
The 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, Patching last regex like: 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 once minified, as expected, all PHP code between the I can see in https://github.com/magento/magento2/blob/2.4-develop/lib/internal/Magento/Framework/View/Template/Html/Minifier.php that |
|
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: 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 // ... ?> <?phpwith<?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
.phtmlfiles 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).The text was updated successfully, but these errors were encountered: