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

Register console commands also in non-sapi mode #14

Merged
merged 1 commit into from May 7, 2018

Conversation

@f3l1x
Copy link
Member

commented Apr 6, 2018

I've changed default behaviour, because it prevents to pre-compile container with all console services.

It's similar as #12 PR, but with more friendly way. I hope so.

Could you please elaborate guys?

Does it solve your problems @mabar @paveljanda @pilec?

@f3l1x f3l1x added the enhancement label Apr 6, 2018

@f3l1x f3l1x added this to the v0.4 milestone Apr 6, 2018

@f3l1x f3l1x self-assigned this Apr 6, 2018

@f3l1x f3l1x requested review from enumag, paveljanda and mabar Apr 6, 2018

@f3l1x f3l1x force-pushed the feature/autoregister branch from 2052271 to 1a975f0 Apr 6, 2018

@mabar
Copy link
Member

left a comment

Definitely better than cliOnly.
Just one more change to have it complete - http.request service should be overwritten in console mode only.

if($builder->parameters['consoleMode'] === true) {
    if ($builder->hasDefinition('http.request') && $config['url'] !== NULL) {
        $builder->getDefinition('http.request')
            ->setClass(Request::class, [new Statement(UrlScript::class, [$config['url']])]);
    }
}
@f3l1x

This comment has been minimized.

Copy link
Member Author

commented Apr 7, 2018

You're right, I've totally forgot it.

We cannot use $builder->parameters['consoleMode'], it makes unvisible dependency on consoleMode and Nette\Configurator.

We cannot use PHP_SAPI condition.

I see 2 approaches. 1st is I wanna use console in CLI mode, 2nd one is I wanna warmup container.

1st:

console:
  enabled: %consoleMode%
  url: foo.bar

2nd:

$configurator->addParameters(['console' => ['enabled' => true, 'url' => 'foo.bar']]);

We can maybe change enabled by default to false, to force user to setup enabled: %consoleMode%.

@mabar

This comment has been minimized.

Copy link
Member

commented Apr 7, 2018

I want to use console in both cli and not cli mode and be able to warmup container in both of them to cover all use cases.
Solution with consoleMode parameter is imho required. I disagree there is hidden dependency on Nette\Configurator. Configurator adds parameters, but is not really required as direct Nette\DI user can add them itself. So we should find how to make it more friendly for users without Nette\Configurator. But not in that package, it is design problem.
Maybe $builder->getParameter($parameter) which would throw an exception like %s require parameter %s with suggestion how to fix it.

@f3l1x f3l1x force-pushed the feature/autoregister branch from 1a975f0 to e916648 Apr 7, 2018

@f3l1x

This comment has been minimized.

Copy link
Member Author

commented Apr 7, 2018

I went in the same way as in TracyExtension (https://api.nette.org/2.4/source-Bridges.Nette.TracyExtension.php.html#45). Check the last update please.

@mabar

mabar approved these changes Apr 7, 2018

Copy link
Member

left a comment

I like it 👍

@f3l1x

This comment has been minimized.

Copy link
Member Author

commented Apr 7, 2018

@@ -33,13 +34,24 @@ console:
- Contributte\Console\Helper\ContainerHelper
```

In fact in console mode / SAPI mode is no http request and thus no URL address. This inconvenience you have to solve by yoursolve.
In fact in SAPI (CLI) mode there is no http request and thus no URL address. This inconvenience you have to solve by yoursolve. Well, via `console.url` option.

This comment has been minimized.

Copy link
@enumag

enumag Apr 7, 2018

Contributor

by yourself

/**
* @param bool $cliMode
*/
public function __construct($cliMode = FALSE)

This comment has been minimized.

Copy link
@enumag

enumag Apr 7, 2018

Contributor

I'm quite confused by this parameter + the enabled config. Are you certain we need both? I can tell already that it would take me a few minutes to determine how things work. (Too lazy to do it now.) The most confusing thing is that you actually used %consoleMode% for both in the readme.

This comment has been minimized.

Copy link
@f3l1x

f3l1x Apr 7, 2018

Author Member

Well, I don't know how to solve this example: enable extension for cli/non-cli and does not override http URL in non-cli mode.

This comment has been minimized.

Copy link
@mabar

mabar Apr 7, 2018

Member

Hmm. I agree that it must look little bit weird. Behavior for enabled is the same as (not) registering extension. Extension should probably be registered in CLI-only config file if you don't want console in HTTP mode.

This comment has been minimized.

Copy link
@enumag

enumag Apr 7, 2018

Contributor

Anyone knows how this works in Symfony?

This comment has been minimized.

Copy link
@mabar

mabar Apr 7, 2018

Member

And maybe $cliMode should be without default value to force user configure if it is really cli mode or not. Using just true/false could create weird behavior.

This comment has been minimized.

Copy link
@pilec

pilec Apr 10, 2018

I came up with something maybe crazy/magic, but it can works.

You can use initialize method in container like this:

	public function afterCompile(Nette\PhpGenerator\ClassType $class)
	{
		parent::afterCompile($class);

		$initialize = $class->methods['initialize'];
		$config = $this->validateConfig($this->defaults);

		if (isset($config['url'])) {
			$initialize->addBody(sprintf("
			if (!$this->hasService('http.request')) {
				$this->addService('http.request', new \Nette\Http\Request("%s"));
			}
		", $config['url']));
		}
	}

This comment has been minimized.

Copy link
@mabar

mabar Apr 10, 2018

Member

It is not clear from api how it works. But I have another idea. We can use afterCompile to overwrite http request only if PHP_SAPI === 'cli' at runtime.
What do you think @f3l1x?

This comment has been minimized.

Copy link
@f3l1x

f3l1x Apr 12, 2018

Author Member

Well @pilec, it's a brave idea :-D

This comment has been minimized.

Copy link
@f3l1x

f3l1x Apr 12, 2018

Author Member

@mabar PHP_SAPI is problematic, because it's useful when you can control by yourself when it's generated. You need some parameters for that.

@pilec

This comment has been minimized.

Copy link

commented Apr 10, 2018

For us it works as a charm, great job, @f3l1x!

@paveljanda

This comment has been minimized.

Copy link
Member

commented Apr 10, 2018

I am also a bit confused by the enabled parameter. Will deep into it later today.

@mabar

This comment has been minimized.

Copy link
Member

commented Apr 10, 2018

My proposal is to remove enabled parameter. If extension should be registered only for CLI only, so why not register it in CLI-only config file?
We need just cliMode parameter to not overwrite real http request. And it should be without default parameter to prevent misconfiguration.

@f3l1x f3l1x force-pushed the feature/autoregister branch from e916648 to e5d533b Apr 10, 2018

@f3l1x

This comment has been minimized.

Copy link
Member Author

commented Apr 10, 2018

Could you please @pilec test it with latest update? I've removed enabled option.

@f3l1x f3l1x force-pushed the feature/autoregister branch from e5d533b to ddc4cc2 May 7, 2018

@f3l1x

This comment has been minimized.

Copy link
Member Author

commented May 7, 2018

ping @pilec

@f3l1x

This comment has been minimized.

Copy link
Member Author

commented May 7, 2018

I've ended up with ConsoleExtension(%consoleMode%), it's full features with all possibilities.

  • With Nette-based Configurator: no changes
  • With own Configurator: provide %consoleMode% or set ConsoleExtension(true/false).

@f3l1x f3l1x force-pushed the feature/autoregister branch from ddc4cc2 to ec50116 May 7, 2018

@f3l1x f3l1x force-pushed the feature/autoregister branch from ec50116 to 6c1ad01 May 7, 2018

@pilec

This comment has been minimized.

Copy link

commented May 7, 2018

It works! Thanks a lot.

@f3l1x f3l1x merged commit bab493d into master May 7, 2018

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+23.7%) to 76.344%
Details

@f3l1x f3l1x deleted the feature/autoregister branch May 7, 2018

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.