Skip to content

Include Constructor to be a part of CommandListInterface API, extend inline documentation #37901

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.

Sign up for GitHub

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

Open
wants to merge 8 commits into
base: 2.4-develop
Choose a base branch
from
Open

Include Constructor to be a part of CommandListInterface API, extend inline documentation #37901

Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
dacfbe2
Turn CommandList into `@api` to ensure backwards compatibility
lbajsarowicz Aug 26, 2020
664014b
Merge branch '2.4-develop' into refactor/command-list-api
engcom-Hotel Jan 19, 2024
3c53f89
Merge branch '2.4-develop' into refactor/command-list-api
sidolov Mar 19, 2024
03ca61f
Merge branch '2.4-develop' into refactor/command-list-api
engcom-Hotel Apr 14, 2025
706addb
Merge branch '2.4-develop' into refactor/command-list-api
engcom-Dash May 6, 2025
ac753ca
Fixed Static Failure & review comments
engcom-Dash May 6, 2025
bf24b54
Merge branch '2.4-develop' into refactor/command-list-api
engcom-Hotel May 12, 2025
ca7741c
Merge branch '2.4-develop' into refactor/command-list-api
engcom-Dash May 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Turn CommandList into @api to ensure backwards compatibility
  • Loading branch information
@lbajsarowicz @engcom-Echo
lbajsarowicz authored and engcom-Echo committed Aug 21, 2023
commit dacfbe2e372e21ea5e5daebbec81b98a0536b2a2
16 changes: 13 additions & 3 deletions lib/internal/Magento/Framework/Console/CommandList.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

/**
* Class CommandList has a list of commands, which can be extended via DI configuration.
* @api
*/
class CommandList implements CommandListInterface
{
Expand All @@ -17,7 +18,16 @@ class CommandList implements CommandListInterface
protected $commands;

/**
* Constructor
* CommandList constructor is being used for injecting new Commands
*
* Registration of new Commands can be done using `di.xml`:
* <type name="Magento\Framework\Console\CommandList">
* <arguments>
* <argument name="commands" xsi:type="array">
* <item name="your-command-name" xsi:type="object">Vendor\Module\Console\Command\YourCommand</item>
* </argument>
* </arguments>
* </type>
*
* @param array $commands
*/
Expand All @@ -27,9 +37,9 @@ public function __construct(array $commands = [])
}

/**
* {@inheritdoc}
* @inheritdoc
*/
public function getCommands()
public function getCommands(): array
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this return type as this change is Backward incompatible.

Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't it make sense to slowly add in type hinting like this? PHP 5.3 is quite some time ago. And you can always add stronger typing to a class without breaking the interface. As I see it, nothing is breaking backward compatibility here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hello @jissereitsma, I guess this change breaks compatibility with existing code that:

  • Calls getCommands() and expects a return type other than an array.
  • Extends or implements the CommandList class or CommandListInterface without updating overridden/implemented methods.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure whether these arguments make sense. Yes, indeed somebody could create a DI plugin and return a string from the getCommands() method. But that will break the entire Magento CLI anyway! The same way counts for rewriting the CommandList class or CommandListInterface interface. So, the choice is between opting for the possibility that somebody has hacked the Magento core to an extend where the CLI does not work properly anymore, or sticking to modern-day PHP standards. I would opt for the future instead.

Copy link
Contributor

@ihor-sviziev ihor-sviziev May 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Calls getCommands() and expects a return type other than an array.

It was always returning an erray, so no code can expect it

Extends or implements the CommandList class or CommandListInterface without updating overridden/implemented methods.

Technically you’re right, but if we’ll treat all changes as this - we can’t add return types to anywhere --> we’ll never have strict types

Since this class was previously not marked as API, we can still change its interface.
But after merging this PR, we can’t. So I suggest adding the return type now

Copy link
Contributor

@engcom-Hotel engcom-Hotel May 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Hello @ihor-sviziev & @jissereitsma,

Let me discuss this with the internal team. Meanwhile moving this PR On Hold.

Thanks

Copy link
Contributor

Choose a reason for hiding this comment

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

Hello @ihor-sviziev & @jissereitsma,

We got reply from the internal team, according to them adhering to BIC checks should remain the standard unless there is a highly compelling justification to deviate from it. In this instance, I don't find the justification sufficiently convincing.

{
return $this->commands;
}
Expand Down