Skip to content

Handle the error properly when LESS source file is empty or having compilation error, and make the error message clear and clean. #22182

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

Conversation

Hailong
Copy link
Member

@Hailong Hailong commented Apr 6, 2019

Description (*)

Sometimes developer may want to put a <css src=... reference in the layout xml file first to get it ready, while leave an empty LESS source file (or with some comments) there to work later on.

Now the setup:static-content:deploy command would fail and print a full callstack of exception, while the callstack doesn't really show the real problem. Moreover, the deployment is actually moving on, until it gets a PHP error that failed to open CSS file, which it should never try since its LESS source is empty already.

This fix is to address such issue, print a clear error message, and to avoid the final PHP open file error. It covers the following two conditions:

  1. LESS file is empty.
  2. LESS file has compilation errors.

Fixed Issues (if relevant)

  1. Empty LESS source file causes setup:static-content:deploy command fail #22183: Empty LESS source file causes setup:static-content:deploy command fail

Manual testing scenarios (*)

  1. Clone the 2.3-develop branch, and install a new site.
  2. Add a LESS file to app/design/frontend/Magento/blank/web/css/test-empty.less, which has NO content or just some comments.
  3. Add <css src="css/test-empty.css"/> to app/design/frontend/Magento/blank/Magento_Theme/layout/default_head_blocks.xml
  4. Now run the setup:static-content:deploy command to compile.
  5. We should be able to see the error message, and the compilation should complete with no other errors.

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)

@m2-assistant
Copy link

m2-assistant bot commented Apr 6, 2019

Hi @Hailong. 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 2.3-develop instance - deploy vanilla Magento instance

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

@josefbehr
Copy link
Contributor

@Hailong great PR, thank you. Not sure why the travis build failed, error message had nothing to do with this change, I'll trigger the build again and approve.

@magento-engcom-team
Copy link
Contributor

Hi @josefbehr, thank you for the review.
ENGCOM-5612 has been created to process this Pull Request
✳️ @josefbehr, could you please add one of the following labels to the Pull Request?

Label Description
Auto-Tests: Covered All changes in Pull Request is covered by auto-tests
Auto-Tests: Not Covered Changes in Pull Request requires coverage by auto-tests
Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests

@josefbehr josefbehr added the Auto-Tests: Not Covered Changes in Pull Request requires coverage by auto-tests label Aug 13, 2019
@engcom-Alfa engcom-Alfa self-assigned this Aug 20, 2019
@engcom-Alfa
Copy link
Contributor

Hi @Hailong !

During testing we faced the issue

Problem: When LESS file is empty, compilation stops with error message

Steps to reproduce:

  1. Clone the 2.3-develop branch;
  2. Add a LESS file to app/design/frontend/Magento/blank/web/css/test-empty.less, which has NO content
  3. Add <css src="css/test-empty.css"/> to app/design/frontend/Magento/blank/Magento_Theme/layout/default_head_blocks.xml
  4. Now run the setup:static-content:deploy command to compile.

Actual Result:
after2

@Hailong Is it correct message?

If we add some comments to test-empty.less then it will display correct message and continue compilation
after

@Hailong Could you take a look, please?

Thanks!

@Hailong
Copy link
Member Author

Hailong commented Aug 20, 2019

Hi @engcom-Alfa , yes your first screenshot is the error message I want to remove with this pull request. While I think no matter there is comment in the empty file, we would get the first screenshot as I described in the issue report.

@engcom-Alfa
Copy link
Contributor

Hi @Hailong , the error message from first screenshot occurs on branch with PR changes when test-empty.css file still empty, but if we add some comments to this file we got the result from second screenshot.

Could you take a look?

Thanks!

@Hailong Hailong force-pushed the empty-less-source-error-handling branch from 2465a9a to 0512158 Compare August 22, 2019 17:36
@Hailong
Copy link
Member Author

Hailong commented Aug 22, 2019

Hi @engcom-Alfa , I have updated the code so that it prints the error message properly in both of these two cases.

@josefbehr
Copy link
Contributor

@Hailong Thank you. It seems tests have to be adapted, too. Can you do this?

@Hailong Hailong force-pushed the empty-less-source-error-handling branch from 0512158 to 198ba26 Compare August 25, 2019 23:35
…mpilation error, and make the error message clear and clean.
@Hailong Hailong force-pushed the empty-less-source-error-handling branch from f1e7ce4 to 517788a Compare August 26, 2019 17:17
@Hailong
Copy link
Member Author

Hailong commented Aug 27, 2019

Hi @josefbehr , I have adapted the tests accordingly.

@magento-engcom-team
Copy link
Contributor

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

@engcom-Alfa
Copy link
Contributor

✔️ QA Passed

@engcom-Foxtrot
Copy link
Contributor

@magento run all tests

magento-engcom-team pushed a commit that referenced this pull request Sep 4, 2019
… or having compilation error, and make the error message clear and clean. #22182
@magento-engcom-team magento-engcom-team merged commit b4a7ca4 into magento:2.3-develop Sep 4, 2019
@m2-assistant
Copy link

m2-assistant bot commented Sep 4, 2019

Hi @Hailong, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

@magento-engcom-team magento-engcom-team added this to the Release: 2.3.4 milestone Sep 4, 2019
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.

5 participants