Skip to content
This repository has been archived by the owner on Jan 21, 2020. It is now read-only.

Update expressive #87

Closed
wants to merge 4 commits into from
Closed

Update expressive #87

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b323b47
Update expressive
lpj145 Apr 12, 2019
d3f291d
Update expressive
lpj145 Apr 12, 2019
29e56fa
Update expressive
lpj145 Apr 15, 2019
c33772d
Update expressive
lpj145 Apr 15, 2019
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
16 changes: 13 additions & 3 deletions bin/expressive
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,13 +51,23 @@ $commandList = [
new Module\RegisterCommand('module:register'),
];

if (file_exists($cwd . '/config/container.php')) {
$version = strstr(Versions::getVersion('zendframework/zend-expressive-tooling'), '@', true);
//Instace console application
$application = new Application('expressive', $version);

$containerConfig = $cwd . '/config/container.php';
if (file_exists($containerConfig)) {
$container = require $containerConfig;

$commandList[] = new CreateHandler\CreateHandlerCommand('action:create');
$commandList[] = new CreateHandler\CreateHandlerCommand('handler:create');
Copy link
Member

Choose a reason for hiding this comment

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

Update these two lines, please, to add the $cwd and $container arguments:

$commandList[] = new CreateHandler\CreateHandlerCommand('action:create', $cwd, $container); // etc.

}

$version = strstr(Versions::getVersion('zendframework/zend-expressive-tooling'), '@', true);
//Customize commands by use to inject on tooling
lpj145 marked this conversation as resolved.
Show resolved Hide resolved
$commandsFile = $cwd.'/config/commands.php';
if (file_exists($commandsFile)) {
(include $commandsFile)($application, $container ?? null);
}

$application = new Application('expressive', $version);
$application->addCommands($commandList);
Copy link
Member

Choose a reason for hiding this comment

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

Move this line up, to below the original conditional that checks for the container file (after line 64). That way users will get an error if they try to add commands that have the same name as built-ins.

Otherwise, looking good! 👍

Copy link
Author

Choose a reason for hiding this comment

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

If this moved to below, line 67 launch a exception because $application now exist, you can see ?

Copy link
Member

Choose a reason for hiding this comment

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

I looked through the logic of Symfony/Components/Console/Application, and it looks like addCommands() delegates to add(), which, in turn will grab the command name and alias to set an entry in its internal $commands. What this means is that if a command with the same name registers later, it will override the functionality of one registered earlier.

So, that means if we leave the line here, the built-in commands cannot be overwritten.

The question is then: do we want to provide that ability? This would allow developers to extend our commands or provide replacements specific to their needs, but could cause confusion if a developer is not aware that somebody on their team made those changes.

Copy link
Contributor

@timothyfisherdev timothyfisherdev Apr 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Could we create a new Application instance from the config/commands.php file so the built-in commands are preserved, and then use some sort of console flag/option to force the script to run the built-in command even if there are custom ones provided? Idk if that's possible to do a conditional $application->run() based on a console option since the application isn't running yet.

Copy link
Author

Choose a reason for hiding this comment

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

hmm... on my continuous tests lab, i see a wrong on tooling construction... if i need extend or add more commands, the tooling need is a Application based on symfony application, and always needed extend, create other object based on ToolingApplication.... oh my mind...

Copy link
Member

Choose a reason for hiding this comment

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

@lpj145 I'm not understanding what you're saying... Is this comment addressing @timothyfisherdev or something else? Can you give more context, please?

$application->run();