Static asset compilation errors are ignored

[erikhansen]

I'm using Magento EE 2.1.1 with Capistrano 0.5.1. Capistrano is ignoring static asset compilation errors. Screenshot demonstrating problem (I changed command_output to true to see all output for the sake of debugging):

In this commit, the explicit check for the "Compilation from source" error message was removed. In order to fix this issue, I propose that this check gets added back (and possibly ALSO check for the presence of the "New version of deployed files" success message).

If you'd like me to submit this as a PR, let me know.

[erikhansen]

I accidentally created this task without a body. Just updated it with content.

[erikhansen]

I just did some tests and it actually appears that the bin/magento setup:static-content:deploy command returns a non-zero exit code ("1") when there are compilation errors. Take a look at this code and you'll see it references this issue. Per @davidalger's latest comment, it appears this is now in place in 2.1.0+.

@davidalger In order to fully support 2.0.x, we'll need to do two checks:

For 2.0.x, revert to checking for "Compilation from source"
For 2.1.0+, switch from the current check for "New version of deployed files" to checking the return code of the setup:static-content:deploy command.

I wonder how much longer capistrano-magento2 should continue to support 2.0…

[davidalger]

I wonder how much longer capistrano-magento2 should continue to support 2.0…

For as long as 2.0 is a supported major release ;)

For 2.1.0+, switch from the current check for "New version of deployed files" to checking the return code of the setup:static-content:deploy command.

This is actually supposed to be automatic. If the exit code of a command is non-zero, the deploy fails…I just verified this is the case with capture as well as execute where it's fairly well assumed. How did you check the error codes the console returns when receiving this error? I can't seem to get the command to throw any errors for my immediate testing. Can you run the command on the broken code and then run echo $? on the CLI to see what the return code is?

For 2.0.x, revert to checking for "Compilation from source"

I believe the reason I switched away from this in the first place is because that is only one potential error it could spit out. Whats to say there couldn't be other error related strings in the output? Hence the switch to a positive check, which I assumed? worked when I implemented it having no broken code to test with :) based on the output above, this assumed to be positive message shows regardless of there being errors or not.

I'm considering implementing a check for the number of errors reported. It would iterate over each line and check the errors: 123 text for a number greater than zero. This should catch it regardless of the console exit code.

[davidalger]

@erikhansen See above pushed commit for the type of implementation I'm suggesting. Open to any feedback. Want to make sure I get it right this time! ;)

[davidalger]

@erikhansen I've made some additional changes and submitted a PR to develop. Please review. The positive check for presence of the "New version…" text is necessary, as it won't be there (and neither will the "errors: xyz" text) in the case of an Exception being thrown. This can be easily seen if you (for example) run the console command with an invalid option triggering a RuntimeException.

[erikhansen]

@davidalger I look forward to reviewing your PR. I won't be able to review it until early next week, but when I do, I can test it locally to ensure it works with the specific use that prompted me to create this issue.

[erikhansen]

@davidalger I checked out your issue-53 branch locally, ran bundle exec rake install to deploy it locally, and went through the same deployment steps that prompted the creation of this task. I'm happy to report that Capistrano did catch the compilation error:

FYI, I did not test this on a 2.0.x install.

[davidalger]

The changes have been pushed up as part of v0.5.4.