[Console] introduce `__toString` `method` annotation to `InputInterface` #42056

boesing · 2021-07-11T10:31:07Z

Q	A
Branch?	5.4
Bug fix?	no
New feature?	yes
Deprecations?	no
Tickets	Fix #42042
License	MIT
Doc PR	none yet, please ping me if this will be necessary

As a preparation for 6.0, we are adding __toString as method declaration. This avoids having BC-breaks in upstream projects as suggested by @wouterj.
In 6.0, the Stringable interface will be implemented instead.

Fixes #42042

If this feature will be accepted, I'm willing to create the PR for 6.0 which adds the Stringable interface. 👍🏼

@wouterj

As a preparation for 6.0, we are adding `__toString` as method declaration. This avoids having BC-breaks in upstream projects as suggested by @wouterj. In 6.0, the `Stringable` interface will be implemented instead. Fixes symfony#42042 Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com>

carsonbot · 2021-07-11T10:31:10Z

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has a contribution guide which I suggest you to read.

In short:

Always add tests
Keep backward compatibility (see https://symfony.com/bc).
Bug fixes must be submitted against the lowest maintained branch where they apply (see https://symfony.com/releases)
Features and deprecations must be submitted against the 5.4 branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com>

nicolas-grekas · 2021-07-11T17:18:16Z

Please add a changelog entry in the component.
It could also be useful to document what the method is supposed to return, because it's not obvious to me at least.

boesing · 2021-07-11T18:01:12Z

@nicolas-grekas 🤨 it already exists, I just added it to the interface. could you give me a little hint on how you expect that being documented?
Will create it afterwards.

nicolas-grekas · 2021-07-11T18:07:30Z

If we add the method, it means we're expect implementations to return something useful.
We should tell what kind of useful output is expected here.
You could take inspiration from what existing implementations do already I suppose.
I don't know (and I didn't had a look :) )

src/Symfony/Component/Console/Tests/EventListener/ErrorListenerTest.php

Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com>

…when calling that method Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com>

…oString` method annotation Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com>

boesing · 2021-07-12T17:53:52Z

I've created a CHANGELOG entry as well as the method description.
If something is missing, please let me know.

@derrabus I've added the return type-hint to the newly created method within the test-asset.

src/Symfony/Component/Console/Input/InputInterface.php

OskarStark · 2021-07-13T19:36:30Z

Could you please open a PR on the symfony/docs repo and add a test case? Thanks

boesing · 2021-07-13T20:28:57Z

So just to clarify so I understand the open TODOs:

I have added exactly zero new functionality here and there are existing unit tests for those __toString methods in current implementations. What kind of unit tests do you want me to create?

Regarding the docs - I will check that out tomorrow to see how this project is structured. Do you want me to add the description I've added to the interface to the documentation or do you have something specific in mind?

OskarStark · 2021-07-13T20:39:27Z

src/Symfony/Component/Console/Input/InputInterface.php

@@ -18,6 +18,9 @@
 * InputInterface is the interface implemented by all input classes.
 *
 * @author Fabien Potencier <fabien@symfony.com>
+ *
+ * @method string __toString() Returns a stringified representation of the args passed to the command.


AFAIK if someone does not implement this method, a deprecation is thrown.

I would like to have a test case which asserts this deprecation (a missing implementation) and has the "@group legacy".

Not sure I missed sth.

I'll see how to achieved that. Thanks for clarification.
~~The unit test to verify the deprecation should be part of the docs repo?~~

derrabus

Please also adjust UPGRADE-5.4.md and UPGRADE-6.0.md. If we introduce a deprecation, it must be documented there.

derrabus · 2021-07-14T15:47:04Z

src/Symfony/Component/Console/CHANGELOG.md

@@ -5,6 +5,7 @@ CHANGELOG
 ---

 * Add `TesterTrait::assertCommandIsSuccessful()` to test command
+ * Add `InputInterface::__toString()` method annotation. This will be replaced by the extension of the `Stringable` interface in 6.0.


Suggested change

* Add `InputInterface::__toString()` method annotation. This will be replaced by the extension of the `Stringable` interface in 6.0.

* Add method `__toString()` to `InputInterface`

nicolas-grekas · 2021-09-08T10:22:44Z

Friendly ping @boesing

boesing · 2021-09-08T12:06:17Z

Thanks for pinging me, @nicolas-grekas
I dont think I will work more on this because of the following reason:

I dont really need the feature that much that I want to work on multiple repositories to add a method which is already present in almost all implementations
I have plenty of open work to do in another framework to be ready for PHP 8.1 and thus the amount of time is very limited
I dont get what unit test has to be created to cover the deprecation/legacy group stuff requested by @OskarStark

so in the end, the amount of work I have to put into this is not aligned with the benefit of the outcome. Thank you all for the guiding, sorry if I wasted your time with this.

stof · 2021-09-08T12:10:33Z

@boesing writing a unit test that pass a Input implementation without __toString to the Application, and expecting a deprecation message.

derrabus · 2021-09-08T12:41:31Z

@OskarStark Do you have time to take over?

…` (boesing) This PR was merged into the 6.1 branch. Discussion ---------- [Console] Add method `__toString()` to `InputInterface` | Q | A | ------------- | --- | Branch? | 6.1 | Bug fix? | no | New feature? | no | Deprecations? | no | Tickets | Fix #42042 | License | MIT | Doc PR | - Continues #42056. No test added because adding one for the deprecation feels like testing DebugClassLoader, which is not the point here. Commits ------- 224f3e3 [Console] Add method `__toString()` to `InputInterface`

boesing requested a review from chalasr as a code owner July 11, 2021 10:31

carsonbot added Status: Needs Review Console labels Jul 11, 2021

carsonbot added the Feature label Jul 11, 2021

qa: add __toString to test asset

e59336d

Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com>

boesing mentioned this pull request Jul 11, 2021

[Console] InputInterface extends Stringable #42042

Closed

nicolas-grekas added this to the 5.4 milestone Jul 11, 2021

nicolas-grekas changed the title ~~[Console] feature: introduce __toString method annotation to InputInterface~~ [Console] introduce __toString method annotation to InputInterface Jul 11, 2021

derrabus reviewed Jul 11, 2021

View reviewed changes

src/Symfony/Component/Console/Tests/EventListener/ErrorListenerTest.php Outdated Show resolved Hide resolved

qa: add return type to test asset

f93f3f1

Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com>

boesing force-pushed the feature/stringable-inputinterface branch from ef230ca to 7516c10 Compare July 12, 2021 17:52

boesing added 2 commits July 12, 2021 19:52

documentation: add __toString method description on what to expect …

246e081

…when calling that method Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com>

documentation: add CHANGELOG entry for the new `InputInterface::__t…

7516c10

…oString` method annotation Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com>

boesing requested a review from derrabus July 12, 2021 17:54

boesing commented Jul 12, 2021

View reviewed changes

src/Symfony/Component/Console/Input/InputInterface.php Show resolved Hide resolved

OskarStark reviewed Jul 13, 2021

View reviewed changes

derrabus requested changes Jul 14, 2021

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels Jul 14, 2021

derrabus added the Deprecation label Jul 14, 2021

boesing closed this Sep 8, 2021

nicolas-grekas mentioned this pull request Dec 21, 2021

[Console] Add method __toString() to InputInterface #44746

Merged

	* Add `InputInterface::__toString()` method annotation. This will be replaced by the extension of the `Stringable` interface in 6.0.
	* Add method `__toString()` to `InputInterface`

Uh oh!

[Console] introduce __toString method annotation to InputInterface #42056

[Console] introduce __toString method annotation to InputInterface #42056

Uh oh!

Conversation

boesing commented Jul 11, 2021

Uh oh!

carsonbot commented Jul 11, 2021

Uh oh!

nicolas-grekas commented Jul 11, 2021

Uh oh!

boesing commented Jul 11, 2021

Uh oh!

nicolas-grekas commented Jul 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

boesing commented Jul 12, 2021

Uh oh!

Uh oh!

OskarStark commented Jul 13, 2021

Uh oh!

boesing commented Jul 13, 2021

Uh oh!

OskarStark Jul 13, 2021

Choose a reason for hiding this comment

Uh oh!

boesing Jul 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

derrabus left a comment

Choose a reason for hiding this comment

Uh oh!

derrabus Jul 14, 2021

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas commented Sep 8, 2021

Uh oh!

boesing commented Sep 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stof commented Sep 8, 2021

Uh oh!

derrabus commented Sep 8, 2021

Uh oh!

Uh oh!

[Console] introduce `__toString` `method` annotation to `InputInterface` #42056

[Console] introduce `__toString` `method` annotation to `InputInterface` #42056

nicolas-grekas commented Jul 11, 2021 •

edited

Loading

boesing Jul 13, 2021 •

edited

Loading

boesing commented Sep 8, 2021 •

edited

Loading