laminas / laminas-cli

Console command runner, exposing commands written in Laminas MVC and Mezzio components and applications
https://docs.laminas.dev/laminas-cli
BSD 3-Clause "New" or "Revised" License
55 stars 22 forks source link

Adds support for `symfony/console:^6` #87

Closed internalsystemerror closed 2 years ago

internalsystemerror commented 2 years ago
Q A
Documentation yes/no
Bugfix no
BC Break yes
New Feature yes
RFC no
QA no

Addresses https://github.com/laminas/laminas-cli/issues/86.

Description

I'm requesting feedback regarding this implementation as it requires a BC break. In order to upgrade symfony/console to ^6.0 there is a contract change at https://github.com/symfony/console/blob/675a4e6b05029a02ac43d3daf5aa73f039e876ea/Input/InputInterface.php#L131 where InputInterface::isInteractive() now contains a return type hint of bool. As a result I've had to add change the return type hint at https://github.com/internalsystemerror/laminas-cli/blob/725d5dbc791dd8c60865309fb35b5030a4a07a77/src/Input/AbstractParamAwareInput.php#L157.

The more pressing change is that Question now only accepts a scalar value as the default value (whereas an array was allowed before). It is my "belief" (as I have not been able to confirm this specifically) that Question does allow multiple values on multiple lines, so $defaultValue = implode(PHP_EOL, $defaultValue) where $defaultValue is an array should resolve this. It is this solution that I have implemented and all tests pass as before, however I've not yet confirmed this by jumping through the code itself.

Since this does require a BC break with the type hint change, I propose that this is merged into a v2.0.0 of this package, along with any other BC breaks waiting (such as no longer using composer/package-versions-deprecated). Documentation for this change also requires adding.

internalsystemerror commented 2 years ago

I've updated the version constraints to allow symfony v5 to still be used. The code has also been updated/reverted to reflect this.

internalsystemerror commented 2 years ago

@froschdesign There is an abstract class AbstractParamAwareInput which extends a Symfony interface InputInterface. In symfony/console v6, a return type is added meaning it needed to be changed here. Any classes based on AbstractParamAwareInput would also need to change their return type if they override this method. Does this not constitute a BC break? I believe upgrading without changing this on the user side would cause a fatal error?

internalsystemerror commented 2 years ago

More specifically for the above:

On symfony's side:

Symfony\Component\Console\Input\InputInterface v5

/**
 * Is this input means interactive?
 *
 * @return bool
 */
public function isInteractive();

Symfony\Component\Console\Input\InputInterface v6

/**
 * Is this input means interactive?
 */
public function isInteractive(): bool;

On laminas's side:

Laminas\Cli\Input\AbstractParamAwareInput v1

public function isInteractive(): ?bool
{
    return $this->input->isInteractive();
}

Laminas\Cli\Input\AbstractParamAwareInput proposed v2

 public function isInteractive(): bool
{
    /** @psalm-suppress RedundantCastGivenDocblockType */
    return (bool) $this->input->isInteractive();
}
froschdesign commented 2 years ago

@internalsystemerror I have not read the whole code yet, therefore thanks for the explanation! 👍

michalbundyra commented 2 years ago

@Ocramius / @froschdesign / @internalsystemerror Probably does not need to target v2 as the change of the interface is on class marked as @internal, so we can do this change.

froschdesign commented 2 years ago

@michalbundyra That's exactly what I was hoping for, but I didn't read the code.

Ocramius commented 2 years ago

Should I push + release? Any other reviews? :D

Ocramius commented 2 years ago

:ship: thanks @internalsystemerror!

internalsystemerror commented 2 years ago

@michalbundyra Oh nice, I can't believe I missed that :D.

Thanks all.