Skip to content
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

use symfony process to run shell command #13

Conversation

piotrkwiecinski
Copy link
Contributor

@piotrkwiecinski piotrkwiecinski commented Jul 7, 2024

Todo:

  • Clean up imports
  • Add dependency to composer.json
  • Update exception docblock or map to LocalizedException


$process->run();

if (!$process->isSuccessful()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This bit is taken from the Process exception. I didn't want to expose a Symfony exception here as it's an implementation detail.

@piotrkwiecinski piotrkwiecinski marked this pull request as ready for review July 9, 2024 20:25
@hostep
Copy link
Member

hostep commented Jul 10, 2024

Thanks, I'm currently cleaning it up a bit so it works on older versions of PHP and Magento as to not break backwards compatibility and will then test it out a bit.
I'm also considering adding a configuration flag to allow or not allow errors, in case somebody updates this module and suddenly needs to rework their less code, that they have the ability to ignore errors and not crash their deploy flow.

I'm working on all this, on the following branch: https://github.com/baldwin-agency/magento2-module-less-js-compiler/commits/symfony-process/

@piotrkwiecinski
Copy link
Contributor Author

It's good point about having a config option to display errors. I was considering it too. I just wanted to push the code to get your early feedback.

I forgot people still may use stores on PHP pre 8.

@hostep
Copy link
Member

hostep commented Jul 10, 2024

Alrighty, I think I have something that works, see the changes here: v1-develop...symfony-process

Could you give it a try, by using the dev-symfony-process composer constraint?

It improves some things:

  • stops compilation when something is wrong in the lessc process itself and outputs a bunch of info to the console
  • introduces a throw_on_error config flag:
    • when disabled (default): outputs any warnings/errors lessc runs into while compiling to var/log/system.log (so no hard errors, but more like linting errors, the ones you ran into in Missing icons in admin #11)
    • when enabled: outputs those warnings/errors to the console when running SCD process or you'll see those same errors being outputted to css files when just browsing the website in developer mode and will see a more or less unstyled website
  • also throws errors when less or css files are empty, see Investigate if we should keep up with Magento core changes from version 2.3.4 #8

I've tested it with:

  • Magento 2.1.16 | PHP 7.1 | symfony/process 2.8.52
  • Magento 2.4.6-p6 | PHP 8.2 | symfony/process 5.4.23
  • Magento 2.4.7-p1 | PHP 8.3 | symfony/process 6.4.8

@piotrkwiecinski
Copy link
Contributor Author

@hostep I had a chance to test your changes with Magento 2.4.6-p6 | PHP 8.1 | symfony/process 5.4.23 and configuration works.

If you want a bit more flexibility for the flag you could use:
filter_var($throwOnError, FILTER_VALIDATE_BOOLEAN) in v1-develop...symfony-process#diff-d25d3a8c2c8ef97d9372180d5c469a164e76d929607e073f11961bc847522763R303

Rest looks fine to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants