Skip to content

Conversation

@hostep
Copy link
Contributor

@hostep hostep commented Feb 17, 2018

Description

This is suggested change to how the log levels of the static content deploy is mapped to the verbosity of the output.

When deploying the static content, currently Magento outputs two messages using the log level 'alert':

  • Deploy using {some-strategy} strategy
  • Execution time: {time-it-took-to-run-the-command}

In my opinion, this is incorrect, 'alert' means that something is not quite right, PSR-3 describes the alert log level as 'Action must be taken immediately.' which is very incorrect in this case.
And this also results in outputting the messages with a red background in the terminal, which looks very scary.

This PR proposes to change the log level to 'notice' for these two messages.
But when doing this, the messages are no longer outputted, because they are mapped to the output's verbosity level of 'verbose', so they only show up when providing the -v flag.
I propose to increase the verbosity of the 'notice' log level to 'normal' so those messages get displayed, without providing the -v flag.

I've also increased the verbosity level of the 'info' log level to 'verbose', and changed the output of a bunch of other messages from 'notice' to 'info' to keep them behaving the same as how they behave currently (= only outputted when providing -v flag). Examples of these messages:

  • Processing file 'app/design/frontend/Magento/luma/Magento_Checkout/web/css/source/module/checkout/_progress-bar.less' for area 'frontend', theme 'Magento/luma', locale 'default'module 'Magento_Checkout'
  • Processing file 'app/design/frontend/Magento/luma/web/images/select-bg.svg' for area 'frontend', theme 'Magento/luma', locale 'default'
  • Processing file 'app/design/frontend/Magento/luma/web/fonts/Luma-Icons.woff' for area 'frontend', theme 'Magento/luma', locale 'default'
  • ...

If we want to follow the PSR-3 log levels strictly, we probably should even do more drastic changes:

  • Instead of 'notice', we should use 'info' for those first two messages, and should still be outputted without providing verbosity flags on the cli
  • Instead of 'info', we should use 'debug' for all those other messages, and they should only show up when the -vvv is used
  • This means that we have to also increase the verbosity level from 'verbose' to 'normal' for the 'info' log level

I'm a bit hesitant to already do this, as it leaves very little moving space for some in-between verbosity level mapping, since all but one log level would all be mapped to verbosity 'normal', only 'debug' log level would be mapped to the 'debug' verbosity output, which is maybe not flexible enough for future additions.

Does this makes sense? What do you guys think?

Results

Before:
screen shot 2018-02-17 at 18 29 06

After:
screen shot 2018-02-17 at 18 29 38

Fixed Issues (if relevant)

  1. Output of setup:static-content:deploy contains red color, should be a friendlier color #12404: Output of setup:static-content:deploy contains red color, should be a friendlier color
  2. Output of setup:static-content:deploy contains red color, should be a friendlier color community-features#15: Output of setup:static-content:deploy contains red color, should be a friendlier color

Manual testing scenarios

  1. Run the command: bin/magento setup:static-content:deploy -f with different verbosity flags: -v, -vv or -vvv
  2. Watch the output

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)

@hostep hostep force-pushed the fix-for-issue-12404 branch from 8a935c7 to d549014 Compare February 17, 2018 18:29
…hey cause the Magento/blank theme to be outputted twice for some reason...
@hostep
Copy link
Contributor Author

hostep commented Feb 17, 2018

As you may have noticed, the Magento/blank theme was outputted twice in my second screenshot, I've added a second commit to fix this, even though I don't quite understand why this happens exactly.

Bit offtopic, but another thing I've noticed is those two sleep function calls in the Magento\Deploy\Process\Queue class
There are no comments to explain what these calls are supposed to do. My guess is that they are implemented so the terminal where the command runs in isn't being hogged by the while loop. But why are they set to 3 or 5 seconds? Shouldn't 1 second be enough? This will slightly improve the speed of the static content deployment from some quick tests.

@dmanners dmanners self-assigned this Mar 1, 2018
@dmanners
Copy link
Contributor

dmanners commented Mar 1, 2018

Hi @hostep thank you for this PR. I agree that the static content deploy script does seem odd being in red. I do have a question with regards to other commands. How will this change effect them? Are there any other commands that will also need to be updated? Or will this update other commands to now show red text or more detailed messages when not desired?

@hostep
Copy link
Contributor Author

hostep commented Mar 1, 2018

Hi David

Quite frankly: I only tested the setup:static-content:deploy command, because all code changes were done in the Magento_Deploy module or in the Magento_Setup module but only in the DeployStaticContentCommand class.
So I assumed no other command were touched by this, but I haven't verified it to be honest.

Do you happen to know if other commands also use code from the Magento_Deploy module?

@hostep
Copy link
Contributor Author

hostep commented Mar 1, 2018

From a quick look through the code, it looks like these commands are being defined in the Magento_Deploy module:

  • deploy:mode:set
  • deploy:mode:show
  • app:config:dump
  • app:config:import
  • app:config:status
  • setup:static-content:deploy

I'll try to find some time to test if these are getting affected by the changes proposed here.
Maybe tonight, maybe later this week ...

@hostep
Copy link
Contributor Author

hostep commented Mar 1, 2018

@dmanners: ok, I took another look at the code, searched through all Magento files and tried to figure out in which commands the touched code is being used, and it looks like it is only used in the setup:static-content:deploy command.

Just to be sure, I executed all the above commands from my previous post in various ways with my current commit and checked against commit 9d9a6c3 (which is the parent of my commits), and I can see zero differences in the output, except for the setup:static-content:deploy command obviously.

So should be good to go I believe, but feel free to verify these things yourselves :)

@dmanners
Copy link
Contributor

dmanners commented Mar 2, 2018

Thank you @hostep for the investigation. I will take this pr and have a look also just to make sure :)

@ihor-sviziev
Copy link
Contributor

Hi @hostep,
Thank you for your contribution. Could you please create PR with the same changes to 2.3-develop branch?

@hostep
Copy link
Contributor Author

hostep commented Mar 6, 2018

Nice thanks! Yes I can create a PR for 2.3 as well, probably tonight.

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.

4 participants