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

Remove proxy manager #435

Merged
merged 1 commit into from
Feb 23, 2018
Merged

Conversation

Landerstraeten
Copy link
Contributor

Q A
Branch master
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Documented? no
Fixed tickets /

As the title says, remove the proxy manager.

@veewee (and everyone else), please leave your remaks below.

@@ -51,6 +52,8 @@ public function __construct()
$this->filesystem = new Filesystem();
$this->container = $this->getContainer();
$this->setDispatcher($this->container->get('event_dispatcher'));
$this->container->set('console.input', new ArgvInput());
Copy link
Contributor

Choose a reason for hiding this comment

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

This won't work since the output / input wont be configured meaning the verbosity level is not available on the output in the service grumphp.io.console (which is still marked as lazy). The grumphp.io is required before the configureIO() method was called.
You can validate this by running the code with the verbose flag -v: You won't see the additional information.

@Landerstraeten Landerstraeten force-pushed the remove-proxy-manager branch 2 times, most recently from 7a469bb to a1f992e Compare January 17, 2018 10:39
@veewee veewee added this to the Version 0.14.0 milestone Feb 1, 2018
@veewee
Copy link
Contributor

veewee commented Feb 1, 2018

Hi @Landerstraeten,
Can you fetch the latest master branch and rebase your branch against master?

@EugeneZub
Copy link
Contributor

Hi @Landerstraeten,
Could you give me some forecasts about date when the new version will be released (0.14.0)?
Because we are waiting for this new version with our team and in another case we will have to refuse using of this vendor((

@veewee
Copy link
Contributor

veewee commented Feb 7, 2018

@EugeneZub,
Lander told me there are still some minor issues with this PR when using low dependencies. We would like to get this one in for 0.14. Like most open-source projects, we don't set ourselves deadlines and won't be able to answer to your forecast question at this point.

@EugeneZub
Copy link
Contributor

Hi @veewee,
Thanx for your explanations, we'll be watching for changes because this vendor is really good!)

// Register the console input and output to the container
$container->set('console.input', $input);
$container->set('console.output', $output);
parent::configureIO($input, $output);
Copy link
Contributor

Choose a reason for hiding this comment

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

This won't work since you now configure the input and output from the container and not the ones that are available in the commands. This means the arguments and options won't work in the commands. As you can see, the configureIO() does not return anything and should just configure the arguments that are being passed to the method.

/**
* {@inheritdoc}
*/
public function setDispatcher(EventDispatcherInterface $dispatcher)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you move this one outside the constructor? It seems better to set a dispatcher then to overwrite thet set* method.

/**
* @return InputInterface
*/
public function getInput()
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you add these getters? They are not being used at the moment, so I would not make them available at this point.

@Landerstraeten Landerstraeten force-pushed the remove-proxy-manager branch 2 times, most recently from 67a5f33 to da216d1 Compare February 8, 2018 09:53
Copy link
Contributor

@veewee veewee left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks

@veewee veewee merged commit 7f482ef into phpro:master Feb 23, 2018
@Landerstraeten Landerstraeten deleted the remove-proxy-manager branch October 28, 2018 12:17
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.

3 participants