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 expressive #87

Closed
wants to merge 4 commits into from

Conversation

Projects
None yet
3 participants
@lpj145
Copy link

commented Apr 12, 2019

Whats think about to user improve the list of commands ? sometimes, the dev need only two or three commands..

Provide a narrative description of what you are trying to accomplish:

  • Are you fixing a bug?

    • Detail how the bug is invoked currently.
    • Detail the original, incorrect behavior.
    • Detail the new, expected behavior.
    • Base your feature on the master branch, and submit against that branch.
    • Add a regression test that demonstrates the bug, and proves the fix.
    • Add a CHANGELOG.md entry for the fix.
  • Are you creating a new feature?

    • Why is the new feature needed? What purpose does it serve?
    • How will users use the new feature?
    • Base your feature on the develop branch, and submit against that branch.
    • Add only one feature per pull request; split multiple features over multiple pull requests
    • Add tests for the new feature.
    • Add documentation for the new feature.
    • Add a CHANGELOG.md entry for the new feature.
  • Is this related to quality assurance?

  • Is this related to documentation?

lpj145 added some commits Apr 12, 2019

Update expressive
Whats think about to user improve the list of commands ? sometimes, the dev need only two or three commands..
@weierophinney

This comment has been minimized.

Copy link
Member

commented Apr 12, 2019

I'm not entirely sure what you're trying to accomplish; can you provide some examples of how you would consume the changes you're proposing here? And are these necessary — could they be accomplished by adding a composer script, or writing an app-specific symfony/console command?

@lpj145

This comment has been minimized.

Copy link
Author

commented Apr 12, 2019

yep, i think some micro services needed a little cli commands, and if needed add to expressive existent commands, needed only add commands php array with symfony/console.

@lpj145

This comment has been minimized.

Copy link
Author

commented Apr 12, 2019

you needed anytime add more commands on you application ?

@weierophinney

This comment has been minimized.

Copy link
Member

commented Apr 12, 2019

@lpj145 Can you provide examples of usage, please?

  • What the config/commands.php file would look like.
  • How you would invoke the commands (I have a good idea already, but we need this information for the documentation).

Thanks!

@lpj145

This comment has been minimized.

Copy link
Author

commented Apr 12, 2019

//in config/commands.php

return [
// phinx migrations based ?
    new Support\MigrationsCreate('migrations:create'),
    new Support\MigrationsMigrate('migrations:migrate'),
    new Support\MigrationsRollback('migrations:rollback'),
// Some commands.
    new Users\CLI\UsersAdd('users:add'),
    new Users\CLI\UsersDeactive('users:deactive'),
];

i think invoke command by add expressive file without extension like a laravel 'app', and...

//in root project dir
expressive %someargs%

IMHO expressive have strong, easily curve and good way to work, but he need some improvments, he is a micro, but with some tooling, he can powerup!

@lpj145

This comment has been minimized.

Copy link
Author

commented Apr 12, 2019

or what do you think ? is best on container aliased 'commands' ?

@weierophinney

This comment has been minimized.

Copy link
Member

commented Apr 15, 2019

I definitely think this is interesting. One thing I'd like to have, however, is access to the container, so that commands can utilize dependencies; otherwise, anything like you propose has some pretty big limitations.

I think that could likely be accommodated by some minor changes to the bin/expressive script, and to how the config/commands.php script would be formed:

// in bin/expressive.php, replacing lines 54 - 57:
$containerConfig = $cwd . '/config/container.php';
if (file_exists($containerConfig)) {
    $container = require $containerConfig;
    $commandList[] = new CreateHandler\CreateHandlerCommand('action:create', $cwd, $container);
    $commandList[] = new CreateHandler\CreateHandlerCommand('handler:create', $cwd, $container);
}

// Then, after current line 62:
$commandsFile = $cwd . '/config/commands.php';
if (file_exists($commandsFile)) {
    (include $commandsFile)($application, $container ?? null);
}
$application->run()

The commands.php file would look like this:

use Psr\Container\ContainerInterface;
use Symfony\Component\Console\Application;

return function (Application $application, ?ContainerInterface $container) : void {
    $application->addCommands([
        // Either pull commands from the container, or instantiate them directly.
        // If the container is not present, and needed, raise an exception.
    ]);
};

Would you be willing to make those changes as well, @lpj145 ?

@lpj145

This comment has been minimized.

Copy link
Author

commented Apr 15, 2019

Yes of course, this is pretty good, i proposed without include container because thought a minor change from online editor... but, yes, container can be exposed to tooling, and, in the future thought tooling with expose routes command and expose container command...

@lpj145

This comment has been minimized.

Copy link
Author

commented Apr 15, 2019

following you way.

//config/commands.php

return function (\Symfony\Component\Console\Application $application, \Psr\Container\ContainerInterface $container) {
    $application->addCommands([
        new \Support\CLI\ExposeRoutesCommand($container->get(\Zend\Expressive\Application::class), 'routes:show')
    ]);
};
//Expose routes command
declare(strict_types=1);
namespace Support\CLI;

use Symfony\Component\Console\Command\Command;
use Symfony\Component\Console\Helper\Table;
use Symfony\Component\Console\Input\InputInterface;
use Symfony\Component\Console\Output\OutputInterface;
use Zend\Expressive\Application;
use Zend\Expressive\Router\Route;

class ExposeRoutesCommand extends Command
{
    /**
     * @var Application
     */
    private $application;

    public function __construct(Application $application, $name = null)
    {
        parent::__construct($name);
        $this->application = $application;
    }

    protected function execute(InputInterface $input, OutputInterface $output)
    {
        $table = new Table($output);
        $table
            ->setHeaders([
                'name',
                'method',
                'invoker',
                'url'
            ]);

        $routes = $this->application->getRoutes();
        $tableRows = array_map(function (Route $router) {
            $middlewareClass = $router->getMiddleware();
            $handlerProperty = (new \ReflectionClass($middlewareClass))
                ->getProperty('handler');
            $handlerProperty->setAccessible(true);

            return [
                $router->getName(),
                implode(', ', $router->getAllowedMethods()),
                get_class($handlerProperty->getValue($middlewareClass)),
                $router->getPath()
            ];
        }, $routes);


        $table->setRows($tableRows);
        $table->render();
        $output->writeln(sprintf('Founded %d registered route(s)!', count($routes)));
    }
}
Update expressive
now work's expected!
@lpj145

This comment has been minimized.

Copy link
Author

commented Apr 15, 2019

image

$containerConfig = $cwd . '/config/container.php';
if (file_exists($containerConfig)) {
$container = require $containerConfig;
$commandList[] = new CreateHandler\CreateHandlerCommand('action:create');
$commandList[] = new CreateHandler\CreateHandlerCommand('handler:create');

This comment has been minimized.

Copy link
@weierophinney

weierophinney Apr 15, 2019

Member

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

$commandList[] = new CreateHandler\CreateHandlerCommand('action:create', $cwd, $container); // etc.
Show resolved Hide resolved bin/expressive Outdated
Update expressive
is adequate ?
$application = new Application('expressive', $version);
$application->addCommands($commandList);

This comment has been minimized.

Copy link
@weierophinney

weierophinney Apr 15, 2019

Member

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! 👍

This comment has been minimized.

Copy link
@lpj145

lpj145 Apr 15, 2019

Author

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

This comment has been minimized.

Copy link
@weierophinney

weierophinney Apr 15, 2019

Member

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.

This comment has been minimized.

Copy link
@timothyfisherdev

timothyfisherdev Apr 15, 2019

Contributor

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.

This comment has been minimized.

Copy link
@lpj145

lpj145 Apr 15, 2019

Author

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...

This comment has been minimized.

Copy link
@weierophinney

weierophinney Apr 15, 2019

Member

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

@lpj145

This comment has been minimized.

Copy link
Author

commented Apr 16, 2019

Wait a momment, i'm improve other approach and pr again! :)

@lpj145 lpj145 closed this Apr 16, 2019

@timothyfisherdev timothyfisherdev referenced this pull request Apr 18, 2019

Open

Extending with tooling cli object #88

3 of 17 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.