Issue 5316

[lisovyievhenii]

Description

Fix HTML minification problem, when there is a php a comment and no space at the php end tag

Fixed Issues (if relevant)

1. [2.1.0] HTML minification problem with php tag with a comment and no space at the end #5316: [2.1.0] HTML minification problem with php tag with a comment and no space at the end

Manual testing scenarios

1. Create a theme and a template file which contains this:

<?php //if ($showDescription):?>
    <div class="product description product-item-description">
        <?php /* @escapeNotVerified */ echo $_helper->productAttribute($_product, $_product->getShortDescription(), 'short_description') ?>
    </div>
<?php //endif; ?>

2. Enable HTML minification in the backend
3. Re-deploy static content and view_preprocessed in order to generate minified templates
4. Check the front page, where template is used.

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-cicd2]

All committers have signed the CLA.

[magento-engcom-team]

Hi @lisovyievhenii. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

• @magento-engcom-team give me test instance - deploy test instance based on PR changes
• @magento-engcom-team give me {$VERSION} instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Assistant documentation

[phoenix128]

Hello @lisovyievhenii ,
thank you for your contribution, would you please check my reviews? Thank you.

[phoenix128]

Why you did not simpy change the regular expression in #(?<!:|'|")//[^\n\r]*(\s*\?\>)#, instead of adding the prefixing space in the replacement?

[lisovyievhenii]

In case if the code will be like this one:

<?php//else:?>

if the replacement string will be just '$1', we will get the result:

<?php?>

So it will cause calling of undefined conftant php and to prevent it I added the prefixing space in the replacement.

[phoenix128]

Got it, right point.

[phoenix128]

@lisovyievhenii ,
I think we should also change \Magento\Framework\View\Test\Unit\Template\Html\MinifierTest::testMinify to cover both cases. Do you think you can do it?
Thank you.

[lisovyievhenii]

Ok, I will adjust \Magento\Framework\View\Test\Unit\Template\Html\MinifierTest::testMinify to cover possible cases.

[phoenix128]

Thank you @lisovyievhenii for your work and for your time. We appreciate it.

[magento-engcom-team]

Hi @phoenix128, thank you for the review.
ENGCOM-2112 has been created to process this Pull Request

[magento-engcom-team]

Hi @lisovyievhenii. Thank you for your contribution.
Changes from your Pull Request will be available with the upcoming 2.3.0 release.