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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add standard Symfony commands #1117

Merged
merged 4 commits into from Dec 28, 2021
Merged

Conversation

umulmrum
Copy link

@umulmrum umulmrum commented Dec 9, 2021

This PR replaces Silly commands with default Symfony commands. As both Bref and Silly are maintained by @mnapoli, they might not be amused, so let me explain my thoughts:

Silly indeed removes some boilerplate code required by Symfony, turning command definitions significantly easier. So there is definitely benefit in using it, but I think it doesn't justify the related cost.

  • Silly doesn't improve UX or DX directly, but is just an internal tool to simplify Bref development slightly.
  • Silly hasn't evolved in more than 1,5 years. It isn't the maintainers fault if no one helps out, but still, the result is an outdated lib that needs quite some work to be done. My personal conclusion was that the required time could be invested better somewhere else. Maybe everyone else thinks alike, which would mean Bref is hindered by a lib that is farther down on everyone's list.
  • Removing the dependency from Bref would not only allow complete upgrades to Symfony 6, psr/container 2, and psr/log 3 instantly, but also lower the complexity/file sizes which isn't that bad in Lambda environments (not that much, but still - php-di/invoker as well as polyfills are also removed).

As a final point, I'd say that the new command classes in this PR make it easier to write tests for the commands (and I think separate classes are way easier to understand, but this might be a matter of opinion), but this could also be reached by simple refactoring.

All that said, I'm relatively new to Bref and might have overlooked significant things. Also, the changes are mostly untested yet and I'm sure that arguments/options aren't correct yet. I'm happy to complete the PR if you are 馃憤 in general. Thanks!

Copy link
Contributor

@GromNaN GromNaN left a comment

Choose a reason for hiding this comment

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

I don't know if silly supports autocompletion, but this is a feature that we can provide to bref by using the standard Symfony Console v5.4+.

demo/console.php Outdated
});
$silly->command('sleep', function () {
sleep(120);
$app->add(new class('hello') extends Command {
Copy link
Contributor

Choose a reason for hiding this comment

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

The register method should be used for this case.

$app->register('hello')
    ->addArgument('name', InputArgument::OPTIONAL, '', 'World!')
    ->setCode(function (InputInterface $input, OutputInterface $output): int {
        $output->writeln('Hello, ' . $input->getArgument('name'));
        
        return Command::SUCCESS;
    });

Copy link
Author

Choose a reason for hiding this comment

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

Way better, thanks!

@mnapoli
Copy link
Member

mnapoli commented Dec 9, 2021

I do appreciate the effort of thinking out of the box to address the problem, instead of fixing all the dependencies upstream :D

The PR looks clean, have you had a chance to test all (most?) commands? Is this easy to merge, or are manual tests required?

@mnapoli
Copy link
Member

mnapoli commented Dec 9, 2021

Ohh tests on master fail, probably because https://github.com/shivammathur/setup-php changed the way PHP-FPM installs again. I think we need to revert some parts of #991 I can't address this right now (I have a live stream tonight), maybe tomorrow if someone else doesn't get to this first.

@@ -22,7 +22,7 @@
"php": ">=7.3.0",
"ext-curl": "*",
"ext-json": "*",
"mnapoli/silly": "^1.7",
"symfony/console": "^3.0|^4.0|^5.0|^6.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it necessary to support old and unmaintained versions? ^4.4|^5.0|^6.0 should be enough.

Copy link
Author

Choose a reason for hiding this comment

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

I just copied the requirement from Silly, and then added ^6.0. I'd suggest to keep support for the old releases for now, and to think about it together with the other Symfony requirements in another issue/PR.

{
protected static $defaultName = 'local';

public const SIGNATURE = 'local [function] [data] [--file=] [--handler=] [--config=]';
Copy link
Contributor

Choose a reason for hiding this comment

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

This const is not used.

Copy link
Author

Choose a reason for hiding this comment

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

Strictly speaking this is a BC break but should be ok anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

Removing __invoke from this class is a more risked breaking change. I don't know what's the policy here.

Copy link
Author

Choose a reason for hiding this comment

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

Fair enough :-) I didn't find statements on BC in the docs, so am unsure, too.

Copy link
Member

Choose a reason for hiding this comment

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

Feel free to remove, no BC break guarantee for console commands.

@umulmrum
Copy link
Author

umulmrum commented Dec 9, 2021

@mnapoli Sorry 馃憖

I tested the init and layers commands but would be grateful for help on the more complex ones. In general, the changes are relatively easy, but I'd still feel better after tests.

The requirement on symfony/console 3.x breaks the tests on --prefer-lowest as this Symfony version doesn't recognize the $defaultName property yet. Should support be removed as @GromNaN suggested, or should I switch to old-school command definitions?

@t-richard
Copy link
Member

AFAIK defining the static property is only useful when you want to be able to lazy-load commands if using the Symfony framework (see https://symfony.com/doc/current/console/commands_as_services.html#lazy-loading)

I think it's OK to switch to the "old-school way" as Symfony 3 is not a huge obstacle here apparently.

@mnapoli
Copy link
Member

mnapoli commented Dec 28, 2021

Let's give this a try, thank you @umulmrum!

@mnapoli mnapoli merged commit 7603fbd into brefphp:master Dec 28, 2021
mnapoli added a commit that referenced this pull request Dec 28, 2021
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.

None yet

4 participants