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

[META] Symfony 6 return type declarations #43021

Closed
Tracked by #81
wouterj opened this issue Sep 14, 2021 · 30 comments
Closed
Tracked by #81

[META] Symfony 6 return type declarations #43021

wouterj opened this issue Sep 14, 2021 · 30 comments

Comments

@wouterj
Copy link
Member

wouterj commented Sep 14, 2021

Symfony 6 comes (almost) fully typed. If you're an open source package maintainer, adding a return type might cause BC breaks (as it forces your users to add the type as well if they implement or override the method).
We might consider, on a case by case basis, whether it's worth to postpone adding a type declaration for a specific method to Symfony 7. This might help you, as a maintainer, to release a new minor instead of a new major version to add support for Symfony 6.

  1. In order to properly judge this, we ask you to install 5.4-dev in your package and run the type patch utility twice (also temporarily install the symfony/error-handler component if it's not available already):

    $ SYMFONY_PATCH_TYPE_DECLARATIONS=force=1 ./vendor/bin/patch-type-declarations
    $ SYMFONY_PATCH_TYPE_DECLARATIONS=force=1 ./vendor/bin/patch-type-declarations
    
  2. This script patches any methods that are private, final, @internal or tests. Please double check if the unpatched methods should not be final or internal (i.e. should a user really extend or use this class?).

  3. If the second run produces any deprecations in the output for methods that must be considered public/protected, please copy paste them as a comment to this issue (including a link to the package). E.g:

    https://github.com/wouterj/WouterJEloquentBundle

    WouterJ\EloquentBundle\Command\SeedCommand
    Method "Symfony\Component\Console\Command\Command::execute()" might add "int" as a native return type declaration in the future. Do the same in child class "WouterJ\EloquentBundle\Command\SeedCommand" now to avoid errors or add an explicit @return annotation to suppress this message.
    

    Note that the current 6.0 branch reflects which types are not going to be included in that release. These still produce deprecations in 5.4+, which can be resolved by adding the @return PHPdoc.

    (please only post deprecations mentioning classes from Symfony, other deprecations should be reported in the relevant package issue tracker)

We don't promise anything, but we might postpone some types based on the feedback in this issue. For instance, we already did for the Command::execute(): int method shown in the example.

@jordisala1991

This comment has been minimized.

@wouterj

This comment has been minimized.

nicolas-grekas added a commit that referenced this issue Sep 17, 2021
…) (derrabus)

This PR was merged into the 6.0 branch.

Discussion
----------

[Console] Relax return type on HelperInterface::getName()

| Q             | A
| ------------- | ---
| Branch?       | 6.0
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Tickets       | Part of #43021
| License       | MIT
| Doc PR        | N/A

Removing this one return type would allow Doctrine ORM 2.9 and DBAL 2.13 to work with Symfony 6.

Commits
-------

3fcecfe [Console] Relax return type on HelperInterface::getName()
@drupal-daffie
Copy link

@wouterj Thank you for asking for feedback. I am working in the Drupal core project and we are preparing for using Symfony 6 in our next major release. We are getting the following deprecation warning: "Method "Symfony\Component\Serializer\Normalizer\NormalizerInterface::normalize()" will return "array|string|int|float|bool|\ArrayObject|null" as of its next major version." The problem for us is that we have the method return an object. We have now 3 options: rewrite the normalize system to return an allowed value, or detach it from the Symfony method or ask if the return type hint can be changed to "mixed". Changing the return type hint to "mixed" would save us a lot of time. Our issue for the problem is: https://www.drupal.org/project/drupal/issues/3232074.

@jordisala1991

This comment has been minimized.

@derrabus
Copy link
Member

The problem for us is that we have the method return an object.

What kind of object is this? The problem is that the allowed return types define what kind of input the encoders can expect. It's a contract between normalizers and encoders. That is why it has been narrowed down to what all encoders understand.

@wouterj

This comment has been minimized.

@jordisala1991

This comment has been minimized.

@bbrala
Copy link

bbrala commented Sep 22, 2021

@derrabus while thinking about your comment one thing came to mind, since php8+ is required, shouldn't that interface support Stringable?

The awnser about the returned object required a little more typing, which i can't do right now :)

@derrabus
Copy link
Member

shouldn't that interface support Stringable?

No, because casting a stringable object to a string should be the concern of the normalizer, not the encoder.

fabpot added a commit that referenced this issue Sep 23, 2021
…me() (jordisala1991)

This PR was merged into the 6.0 branch.

Discussion
----------

[Templating] Relax return type on HelperInterface::getName()

| Q             | A
| ------------- | ---
| Branch?       | 6.0
| Bug fix?      | no
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tickets       | Part of #43021 <!-- prefix each issue number with "Fix #", no need to create an issue if none exist, explain below instead -->
| License       | MIT
| Doc PR        | N/A <!-- required for new features -->
<!--
Replace this notice by a short README for your feature/bugfix. This will help people
understand your PR and can be used as a start for the documentation.

Additionally (see https://symfony.com/releases):
 - Always add tests and ensure they pass.
 - Never break backward compatibility (see https://symfony.com/bc).
 - Bug fixes must be submitted against the lowest maintained branch where they apply
   (lowest branches are regularly merged to upper ones so they get the fixes too.)
 - Features and deprecations must be submitted against branch 5.x.
 - Changelog entry should follow https://symfony.com/doc/current/contributing/code/conventions.html#writing-a-changelog-entry
-->

Removing this one return type would allow KnpMenuBundle to work with Symfony 6. See: #43021 (comment)

Commits
-------

0cd0c0b [Templating] Relax return type on HelperInterface::getName()
@wouterj
Copy link
Member Author

wouterj commented Sep 23, 2021

Thanks @jordisala1991!

Fyi, I've hidden all comments as "resolved" when we've resolved that return type. This way, I hope this issue can be kept a bit organized. This has nothing to do with the validity of the comments.

@dunglas
Copy link
Member

dunglas commented Oct 7, 2021

There is a problem with @return annotations and union types

For instance, if the interface has @return void|object, the script adds : void to the method instead of : mixed or nothing.

Reproducer: https://github.com/api-platform/core/blob/2.6/src/Bridge/Doctrine/Common/DataPersister.php#L50

@dunglas
Copy link
Member

dunglas commented Oct 10, 2021

We identified another problem while running the tool on the API Platform codebase. When using the tool with the force=phpdoc mode, sometimes annotations aren't inserted at a good place and create syntax errors:

Exemple:

// ...
         */
        public function parseLiteral(/*Node */ $valueNode, ?array $variables = null)
     * @return mixed
        {

Reproducer: run SYMFONY_PATCH_TYPE_EXCLUDE='/Uuid|NelmioApiDoc|ArraySubsetLegacy|DataPersister/' SYMFONY_PATCH_TYPE_DECLARATIONS='force=phpdoc&php=7.1' ./vendor/bin/patch-type-declarations on api-platform/core#4495.

@kbond
Copy link
Member

kbond commented Oct 13, 2021

Symfony\Component\Console\Input\InputInterface is proving troublesome to support in 7.4 and 8.0 (the mixed returns).

@derrabus
Copy link
Member

@kbond Do you maintain a package that provides its own implementations of that interface?

@kbond
Copy link
Member

kbond commented Oct 13, 2021

It is an implementation in a yet to be released package I'm working on - I was planning on supporting 7.4 & 8.0.

@derrabus
Copy link
Member

Can you provide a PR against 6.0 that removes the return types that are in your way? Maybe it's better to discuss this on actual code.

@mbabker
Copy link
Contributor

mbabker commented Oct 14, 2021

Can we re-evaluate the return typehint on Symfony\Component\Serializer\Normalizer\NormalizerInterface::normalize()? I finally was able to unblock some of my own dependencies and get 6.0 components installed, and the one change that is proving to be the most painful in everything I've worked on so far is that typehint.
BabDev/PagerfantaBundle@6983c66 is actually the final working version that I landed on without dropping PHP 7.4 support (there was a simpler version in this commit but CI failed with parse errors on the PHP 7.4 builds).

@bbrala
Copy link

bbrala commented Oct 14, 2021

I'd like to add how we use the normalize() function in Drupal and why it is as it is. I also have a possible solution that might be usefull.

I'll open a PR that targets the 6.0 branch so we can explore the possible implications.

@derrabus
Copy link
Member

@mbabker If you revert that commit and simply add an array return type to the normalize() method – wouldn't that class work with Symfony 5 and 6?

@mbabker
Copy link
Contributor

mbabker commented Oct 14, 2021

@mbabker If you revert that commit and simply add an array return type to the normalize() method – wouldn't that class work with Symfony 5 and 6?

Huh, TIL PHP's type variance will allow that - https://3v4l.org/E1Rng

nicolas-grekas added a commit that referenced this issue Oct 15, 2021
…` (kbond)

This PR was squashed before being merged into the 6.0 branch.

Discussion
----------

[Console] remove mixed return types from `InputInterface`

| Q             | A
| ------------- | ---
| Branch?       | 6.0
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Tickets       | Ref: #43021 (comment)
| License       | MIT
| Doc PR        | n/a

As discussed in [this issue](#43021 (comment)), I'm having some trouble supporting an `InputInterface` decorator in PHP 7.4/8.0 & Symfony >=6/<6.

The library where this is being used is not yet released and after discussing with `@wouterj`, I don't really have a great reason for not supporting PHP 8+ only. That being said, the console component is one of the most widely used so maybe this is too aggressive?

Commits
-------

006a9b6 [Console] remove mixed return types from `InputInterface`
@franmomu
Copy link
Contributor

franmomu commented Oct 27, 2021

Hi, while trying to test SncRedisBundle with Symfony 6, an error appeared in CI because of adding return type declarations to

abstract protected function doRead(string $sessionId): string;
abstract protected function doWrite(string $sessionId, string $data): bool;
abstract protected function doDestroy(string $sessionId): bool;

is it ok to relax return type declaration on those methods?

@stof
Copy link
Member

stof commented Oct 27, 2021

@franmomu you can add the string return type in the child class even when running on Symfpny 5 (on PHP 7.2+). That variance rule of PHP is precisely what allows adding return types in Symfony 6 while packages can support Symfony 5 and 6.

@franmomu
Copy link
Contributor

franmomu commented Oct 27, 2021

@franmomu you can add the string return type in the child class even when running on Symfpny 5 (on PHP 7.2+). That variance rule of PHP is precisely what allows adding return types in Symfony 6 while packages can support Symfony 5 and 6.

Sorry, my explanation was vague, I was referring to BC break for someone extending the RedisSessionHandler from that bundle if we add return types declarations in a minor version.

@stof
Copy link
Member

stof commented Oct 27, 2021

Well, you indeed need to do a major version if you want to add the return type in a class that is not final (which is why Symfony does those additions in Symfony 6). However, that's totally something that a bundle could do (and if we say that bundles must be able to upgrade to Symfony 6 in minor versions, then we must cancel the whole typing initiative, as there is no way to guarantee that a bundle extending one of our classes does that in a final class only)

@derrabus
Copy link
Member

derrabus commented Oct 27, 2021

AbstractSessionHandler is one of those classes where we had to add return types to some methods becuase PHP 8.1 urges us to do so. This of course does not affect the methods you've described, but I don't think we do ourselves a favor if we rollback the return types just for those methods and keep them on other methods of the very same class.

@franmomu
Copy link
Contributor

However, that's totally something that a bundle could do (and if we say that bundles must be able to upgrade to Symfony 6 in minor versions, then we must cancel the whole typing initiative, as there is no way to guarantee that a bundle extending one of our classes does that in a final class only)

yeah, that's why I asked, because I think this issue is about that:

Symfony 6 comes (almost) fully typed. If you're an open source package maintainer, adding a return type might cause BC breaks (as it forces your users to add the type as well if they implement or override the method).
We might consider, on a case by case basis, whether it's worth to postpone adding a type declaration for a specific method to Symfony 7. This might help you, as a maintainer, to release a new minor instead of a new major version to add support for Symfony 6.

anyway, we'll go with the minor BC break, thanks.

@stof
Copy link
Member

stof commented Oct 28, 2021

@franmomu for the case of the SessionHandler, I think it is not worth delaying the type addition. There are very few bundles that implement custom handlers, and SncRedisBundle could release a major version IMO (if we delay it, the bundle will still need to release a major version at some point, as it would still happen later)

@wouterj
Copy link
Member Author

wouterj commented Oct 28, 2021

Thanks for sharing the finding it in this issue! This kind of discussion is exactly what this issue is about, even if we ultimately decide not to loosen the type.

@jordisala1991
Copy link
Contributor

jordisala1991 commented Oct 31, 2021

For Symfony Panther I am trying the following PR: symfony/panther#509

As you can see, there is a commit (symfony/panther@31d379e) with the problematic changes, I will list them here so you can decide wether is worth it removing them (I can do the PR). They are more related to the fact that Symfony Panther currently supports from PHP 7.2 and Symfony 6 is forcing me to add return typehints only compatible with PHP 8.

Here is the list:

Symfony\Component\Panther\Client
Method "Symfony\Component\BrowserKit\AbstractBrowser::getServerParameter()" might add "mixed" as a native return type declaration in the future. Do the same in implementation "Symfony\Component\Panther\Client" now to avoid errors or add an explicit @return annotation to suppress this message.
Symfony\Component\Panther\DomCrawler\Field\ChoiceFormField
Method "Symfony\Component\DomCrawler\Field\FormField::getValue()" might add "array|string|null" as a native return type declaration in the future. Do the same in implementation "Symfony\Component\Panther\DomCrawler\Field\ChoiceFormField" now to avoid errors or add an explicit @return annotation to suppress this message.
Symfony\Component\Panther\DomCrawler\Field\FileFormField
Method "Symfony\Component\DomCrawler\Field\FormField::getValue()" might add "array|string|null" as a native return type declaration in the future. Do the same in implementation "Symfony\Component\Panther\DomCrawler\Field\FileFormField" now to avoid errors or add an explicit @return annotation to suppress this message.
Symfony\Component\Panther\DomCrawler\Field\FormFieldTrait
Method "Symfony\Component\DomCrawler\Field\FormField::getValue()" might add "array|string|null" as a native return type declaration in the future. Do the same in implementation "Symfony\Component\Panther\DomCrawler\Field\FormFieldTrait" now to avoid errors or add an explicit @return annotation to suppress this message.
Symfony\Component\Panther\DomCrawler\Form
Method "Symfony\Component\DomCrawler\Form::get()" might add "FormField|array" as a native return type declaration in the future. Do the same in implementation "Symfony\Component\Panther\DomCrawler\Form" now to avoid errors or add an explicit @return annotation to suppress this message.
Symfony\Component\Panther\DomCrawler\Form
Method "Symfony\Component\DomCrawler\Form::offsetGet()" might add "FormField|array" as a native return type declaration in the future. Do the same in implementation "Symfony\Component\Panther\DomCrawler\Form" now to avoid errors or add an explicit @return annotation to suppress this message.

There are 4 methods that need a typehint removal (one of them affect 3 classes for Panther)
Another thing that I don't know if has to be fixed here or in Symfony Panther is this change:

createClient in SymfonyPanther is returning AbstractBrowser: https://github.com/symfony/panther/blob/main/src/WebTestAssertionsTrait.php#L280
createClient in Symfony is returning KernelBrowser: https://github.com/symfony/symfony/blob/6.0/src/Symfony/Bundle/FrameworkBundle/Test/WebTestCase.php#L38

@nicolas-grekas
Copy link
Member

Now closing as resolved, thanks to everyone involved!

nicolas-grekas added a commit that referenced this issue Dec 1, 2021
…patibility (PierreRebeilleau)

This PR was squashed before being merged into the 6.0 branch.

Discussion
----------

[Serializer] Remove some type hints for API Platform compatibility

| Q             | A
| ------------- | ---
| Branch?       | 6.0
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #43021, api-platform/core#4495
| License       | MIT

Since Symfony 6.0 and the addition of type hinting, API Platform's tests fails. After a talk with `@dunglas` , we decide to remove some type hinting in the serializer component in order for api-platform to work correctly.

Commits
-------

e541973 [Serializer] Remove some type hints for API Platform compatibility
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests