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

Adds support for symfony/console:^6 #87

Merged
merged 1 commit into from
Nov 30, 2021
Merged

Adds support for symfony/console:^6 #87

merged 1 commit into from
Nov 30, 2021

Conversation

internalsystemerror
Copy link
Member

@internalsystemerror internalsystemerror commented Nov 30, 2021

Q A
Documentation yes/no
Bugfix no
BC Break yes
New Feature yes
RFC no
QA no

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

@froschdesign froschdesign linked an issue Nov 30, 2021 that may be closed by this pull request
composer.json Outdated Show resolved Hide resolved
@internalsystemerror
Copy link
Member Author

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

@froschdesign froschdesign requested a review from boesing November 30, 2021 13:59
@froschdesign froschdesign added this to the 1.3.0 milestone Nov 30, 2021
@froschdesign froschdesign changed the title feat: add support for symfony v6 (BC Break) Adds support for Symfony v6 Nov 30, 2021
@internalsystemerror
Copy link
Member Author

internalsystemerror commented Nov 30, 2021

@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
Copy link
Member Author

internalsystemerror commented Nov 30, 2021

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();
}

Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

Needs to target the 2.0.x branch (which we haven't opened yet).

This review is just a placeholder for that - code-wise, patch seems ok.

@froschdesign froschdesign modified the milestones: 1.3.0, 2.0.0 Nov 30, 2021
@froschdesign
Copy link
Member

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

@michalbundyra
Copy link
Member

@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
Copy link
Member

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

@michalbundyra michalbundyra modified the milestones: 2.0.0, 1.3.0 Nov 30, 2021
@Ocramius
Copy link
Member

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

@Ocramius Ocramius self-assigned this Nov 30, 2021
@Ocramius Ocramius changed the title Adds support for Symfony v6 Adds support for symfony/console:^6 Nov 30, 2021
@Ocramius Ocramius merged commit f108b8d into laminas:1.3.x Nov 30, 2021
@Ocramius
Copy link
Member

🚢 thanks @internalsystemerror!

@internalsystemerror
Copy link
Member Author

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

Thanks all.

@internalsystemerror internalsystemerror deleted the feature/symfony-6 branch November 30, 2021 15:31
@internalsystemerror internalsystemerror restored the feature/symfony-6 branch December 22, 2021 14:47
@internalsystemerror internalsystemerror deleted the feature/symfony-6 branch December 22, 2021 14:48
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.

Add support for Symfony v6
5 participants