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

Update console command definition to SF 4.4 standards #207

Merged
merged 2 commits into from
Sep 6, 2022

Conversation

diimpp
Copy link
Member

@diimpp diimpp commented Feb 19, 2021

Non-BC maintenance change.

  1. While command was already lazy loaded via DI definition, I think it's a better practice to keep command name in class itself + removal of misuse of constructor call.
    https://symfony.com/doc/current/console/commands_as_services.html#lazy-loading
  2. It's good practice to define parent::__construct() last. No practical difference for the current setup though.
    image
    https://symfony.com/doc/4.4/console.html#configuring-the-command

On separate note, will you accept progress bar for this command? It will require to do something with Generators like that

// src/Cli/GenerateInvoicesCommand.php
$this->massInvoicesCreator->setGenerateResult(true);

foreach ($this->massInvoicesCreator->__invoke($orders) as $result) {
    $progressBar->advance();
}

// src/Creator/MassInvoicesCreator.php
     {   
         /** @var OrderInterface $order */
         foreach ($orders as $order) {
             try {
                 $this->invoiceCreator->__invoke($order->getNumber(), $this->dateTimeProvider->__invoke());
                 if (true === $this->generateResult) {
                     yield true;
                 }
             } catch (InvoiceAlreadyGenerated $exception) {
                 continue;
             }
         }
     }   

or let me know if there is standard practice on how to expose progress to console from other services.

@diimpp diimpp requested a review from a team as a code owner February 19, 2021 08:02
@GSadee GSadee merged commit ca12bcf into Sylius:master Sep 6, 2022
@GSadee
Copy link
Member

GSadee commented Sep 6, 2022

Thank you, Dmitri! 🥇

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.

2 participants