Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
Add return-types with help from DebugClassLoader in the CI#33236
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
fabpot commentedAug 19, 2019
Sounds like a solid plan to me. We are not in a hurry. We need to carefully build a plan and targeting 6.0 is probably the thing to do. |
derrabus commentedAug 19, 2019 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
First of all, thank you very much for all the effort you have put into this!
That's a sad and disappointing conclusion. 😞 But I refuse to give up at this point – especially after the wall of text that I posted last night as#33228. 😉 We might not get fully typed, but I'm still convinced we can get further than we are at the moment. We already have added a few return types (mainly because we were somewhat forced to:#27905) and probably will be required to add a few more (#33039).
Yes, but that'll still be the case two years from now. The main difference I see is that we won't have a maintained Symfony version anymore that still supports php 5. I admit, that might help a little. 🤔
Why's that? Having types in Symfony does not require the whole world to release typed versions of their libraries as well. Third-party libraries only need to be updated if they implement/extend Symfony interfaces/classes that received types with 5.0. Perhaps we can find some problematic hot spots and exclude them when adding return types, such as (list might be incomplete):
That could ease the migration for many third-party bundles.
Can you elaborate on this point? I mean, having full covariance is certainly nice to have, but wouldn't call it a requirement. Apart from fluent interfaces (for which we have found a reasonable compromise), where would we actually depend on covariance? Maybe we can exclude those classes when adding return types or work with a combination of a more generic return type declaration and a more specific I mean, there might always be features in newer php releases that might come handy. Who knows, maybe in two years from now, generics have been added to php 8.1 and then we say: "Hey, we can't add return types because we desperately need generics to go fully typed!"
For 5.0: At least add return types to everything that is flagged as
I expect to find most of these cases when actually adding return types. 🤔
Yes! Even if we will never add types, we would end up with properly configured mocks and a codebase without inconsistent return points. Wouldn't be too bad after all. 😎 In preparation of the issue I wrote yesterday, I've done some experiments on my own. I followed a more manual approach, using PhpStorm inspections and refactorings. One result that I already have is a typed version of the EventDispatcher. I will post a PR later – even if we don't merge it, it might still be a good reference. Also, we might want to cherry-pick the changes I've made to the tests. 😃 |
f6ab891 to70f6e37Comparenicolas-grekas commentedAug 19, 2019 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
The issue is complex enough, let's keep a cool head. I don't think it's sad when it's a well-crafted engineering decision, and when there's a plan forward to bring return-types in a less brutal way.
True, that's the cost of having third-party dependencies. Yet the scope is very limited, it's not the same story than doing it for the whole codebase.
Absolutely not, you missed the last item on my plan above: we would throw deprecation notices starting from 5.1 (the same as
That's enough a blocker. ALL lines changed in this PR are individual BC breaks. That's way too abrupt. This can make v5 fail totally, as far as adoption is concerned.
That's what I was reminded when working on this PR: e.g.
Yes, or 4.4.
😎 |
Tobion commentedAug 19, 2019
…rekas)This PR was merged into the 4.4 branch.Discussion----------[DI] add return-types to generated containers| Q | A| ------------- | ---| Branch? | 4.4| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | -| License | MIT| Doc PR | -A chunk of#33236 ready for 4.4Commits-------9170919 [DI] add return-types to generated containers
derrabus commentedAug 19, 2019
Sure, I meant no offense. After all the effort that has gone into this already, of course I am disappointed if the conclusion is that we won't proceed for now. That doesn't mean that you've made a bad decision. Your plan sounds reasonable and we really might be in a better position two years from now. Yet, you make it sound like we have to decide between two extreme options: Leaving everything as it is or going 100%. I was trying to find a middle ground. Adding types where the impact is low. Of course, estimating the impact of each change is hard and nothing we could do fully automated. But this is ultimately not my decision. If the plan is to make adding return types a smooth two-year project, I'm still in and would offer my help with executing the plan, if I may. Also, the more I think about your suggestion of a CI run with an automatically patched typed version of the current codebase, the more I like it. That way, we might already get some benefits of adding return types without actually adding them. Can you that that up? I would offer to help fixing those tests.
Yes, you are right. I might have underestimated that danger. |
derrabus commentedAug 19, 2019
As promised:#33248 |
… (nicolas-grekas)This PR was merged into the 5.0-dev branch.Discussion----------[5.0] Add return types on internal|final|private methods| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#33228#33236| License | MIT| Doc PR | -I think some changes here can be backported to 4.4Also, this fails with e.g. `Return value of Symfony\Component\Serializer\Normalizer\ArrayDenormalizer::denormalize() must be an object, array returned`Which means we need to update `DenormalizerInterface::denormalize()` with `@return object|array` in 3.4.Commits-------6f303b4 [5.0] Add return types on internal|final|private methods
…hods (nicolas-grekas, derrabus)This PR was merged into the 4.4 branch.Discussion----------Add return types to tests and final|internal|private methods| Q | A| ------------- | ---| Branch? | 4.4| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#33228#33236| License | MIT| Doc PR | symfony/symfony-docs#... <!-- required for new features -->#33266 made me spot an issue with my logic to add return types with help from `DebugClassLoader`.Here is a completing PR, with the logic expanded to test classes.I need help to fix tests, /cc@derrabus :)Commits-------c39fd9c Fixed tests on the Security and Form componentsfc186bb Add return types to tests and final|internal|private methods
70f6e37 to8c8534eCompareThis PR was merged into the 3.4 branch.Discussion----------Use PHP 7.4 on deps=low| Q | A| ------------- | ---| Branch? | 3.4| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | -| License | MIT| Doc PR | -Having a job on PHP 7.4 will allow using return type covariance as described in#33236Commits-------21b8702 Use PHP 7.4 on deps=low
1c32a23 tod066b06Comparenicolas-grekas commentedAug 25, 2019
PR now green on Travis, help wanted for fixing failures on appveyor. |
6b73f04 to557dfb1Compare…rabus)This PR was merged into the 3.4 branch.Discussion----------[3.4] Fix return types declarations| Q | A| ------------- | ---| Branch? | 3.4| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | -| License | MIT| Doc PR | -This is the subset of#33236 that applies to branch 3.4.This PR ensures return types are correctly declared, and adjust some implementations that return incorrect types.Commits-------2ceb453 [SecurityBundle] fix return type declarationsc1d7a88 [BrowserKit] fix return type declarations07405e2 [PropertyInfo] fix return type declarations5f3b4b6 [Bridge/Doctrine] fix return type declarations8706f18 [Form] fix return type declarationsa32a713 [Console] fix return type declarations523e9b9 [Intl] fix return type declarations73f504c [Templating] fix return type declarations2b8ef1d [DomCrawler] fix return type declarations2ea98bb [Validator] fix return type declarations6af0c80 [Process] fix return type declarations28646c7 [Workflow] fix return type declarations5f9aaa7 [Cache] fix return type declarations5072cfc [Serializer] fix return type declarations70feaa4 [Translation] fix return type declarationsca1fad4 [DI] fix return type declarations9c63be4 [Config] fix return type declarations05fe553 [HttpKernel] Fix return type declarationse0d79f7 [Security] Fix return type declarationsc1b7118 [Routing] Fix return type declarationsef5ead0 [HttpFoundation] fix return type declarations
4589b55 to670dd85Compare670dd85 to47b03afCompare…n type declarations (nicolas-grekas)This PR was merged into the 4.4 branch.Discussion----------[ErrorHandler] make DebugClassLoader able to add return type declarations| Q | A| ------------- | ---| Branch? | 4.4| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#33236| License | MIT| Doc PR | -This is the current state of the patching logic I'm using in `DebugClassLoader`.I run it after a `composer i -o` with the excluded classes emptied.It's not perfect, it requires manual changes/review, but it did the job so far.Patching is enabled with the `SYMFONY_PATCH_TYPE_DECLARATIONS` env var.The value of the env var is an URL-encoded array, with the following parameters as of now:- `force=0/1/docblock`: - `0` adds return types only to non-BC breaking places: internal/final/test methods; - `1` adds return types everywhere possible, potentially breaking BC with child classes that didn't declare the return types before; - `docblock` adds `@return` annotations to opt-out from deprecation notices that `DebugClassLoader` trigger otherwise - the annotation meaning: "I know a return type should be used here instead of an annotation, but I can't add it for BC reasons for now".- `php71-compat=0/1`: to prevent using the `object` type when the code must be compatible with PHP 7.1Commits-------72f6a97 [ErrorHandler] make DebugClassLoader able to add return type declarations
1e1e0e7 to528083dCompare528083d to11149a1Compare…CI (nicolas-grekas)This PR was merged into the 4.4 branch.Discussion----------Add return-types with help from DebugClassLoader in the CI| Q | A| ------------- | ---| Branch? | 4.4| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#33228| License | MIT| Doc PR | -I've spent a great deal of time on this PR, experimenting with adding return types to the codebase.TL;DR: my conclusion is that we cannot make it for 5.0.There are two reasons for this:1. The burden this will put on the community is immense, especially when considering that third party libs must also be updated for any apps to work at all on a return-typed 5.0. Symfony must add them last, not first.2. We need return type covariance, yet this won't be available before PHP 7.4, while 5.0 supports 7.2.What's attached?- ~a draft patching logic in `DebugClassLoader` to add return-type where it discovers this should be done~- return types added automatically thanks to#33283- ~manual fixes for situations not handled (yet, if possible at all) by that logic in `DebugClassLoader`~#33332What's achieved? Tests are green \o/At this stage, I think we have to acknowledge we won't add return-types in 5.0 but prepare a serious plan to add them in 6.0.This plan could be:- [x] make DebugClassLoader able to automate adding return types.- [x] in 4.4: add all possible return types that don't break BC, e.g. in `Tests` and in generated code- [x] spot and fix places where annotations aren't accurate, add more annotations where possible.- [x] ensure `DebugClassLoader` triggers the best possible deprecations that encourage ppl to add return-types in their libs/apps. This means we could decide to disable the current ones (see#33235) and to re-enable them in 5.1. This will also give us the time to fine-tune the tooling (item 1. on this list)Ideally, we could reach a point where we could test branch 4.4 *with* return-types: we'd use the tooling to add them automatically in the CI job, then we'd run tests and they should be green. Let's do this?Help Wanted, here is how:*With PHP 7.4*, run `php .github/patch-types.php`. This will add return types everywhere possible.Then run tests, e.g. `./phpunit src/Symfony/Component/HttpFoundation --exclude-group legacy,issue-32995`Here are the components that fail with return types added, please help me check them all with a PR on [my fork](https://github.com/nicolas-grekas/symfony/tree/eh-return-types): - [x] src/Symfony/Bridge/Doctrine - [x] src/Symfony/Bridge/Monolog - [x] src/Symfony/Bridge/PhpUnit - [x] src/Symfony/Bridge/ProxyManager - [x] src/Symfony/Bridge/Twig - [x] src/Symfony/Bundle/DebugBundle - [x] src/Symfony/Bundle/FrameworkBundle - [x] src/Symfony/Bundle/SecurityBundle - [x] src/Symfony/Bundle/TwigBundle - [x] src/Symfony/Bundle/WebProfilerBundle - [x] src/Symfony/Bundle/WebServerBundle - [x] src/Symfony/Component/Asset - [x] src/Symfony/Component/BrowserKit - [x] src/Symfony/Component/Cache - [x]nicolas-grekas#28 src/Symfony/Component/Config - [x] src/Symfony/Component/Console - [x] src/Symfony/Component/CssSelector - [x] src/Symfony/Component/Debug - [x]nicolas-grekas#28 src/Symfony/Component/DependencyInjection - [x] src/Symfony/Component/DomCrawler - [x] src/Symfony/Component/Dotenv - [x] src/Symfony/Component/ErrorHandler - [x] src/Symfony/Component/ErrorRenderer - [x]nicolas-grekas#24 src/Symfony/Component/EventDispatcher - [x] src/Symfony/Component/ExpressionLanguage - [x] src/Symfony/Component/Filesystem - [x] src/Symfony/Component/Finder - [x] src/Symfony/Component/Form - [x] src/Symfony/Component/HttpClient - [x] src/Symfony/Component/HttpFoundation - [x] src/Symfony/Component/HttpKernel - [x] src/Symfony/Component/Inflector - [x] src/Symfony/Component/Intl - [x] src/Symfony/Component/Ldap - [x] src/Symfony/Component/Lock - [x] src/Symfony/Component/Mailer - [x] src/Symfony/Component/Messenger - [x] src/Symfony/Component/Mime - [x] src/Symfony/Component/OptionsResolver - [x] src/Symfony/Component/Process - [x] src/Symfony/Component/PropertyAccess - [x] src/Symfony/Component/PropertyInfo - [x]nicolas-grekas#25 src/Symfony/Component/Routing - [x]nicolas-grekas#26 src/Symfony/Component/Security - [x] src/Symfony/Component/Security/Core - [x] src/Symfony/Component/Security/Guard - [x] src/Symfony/Component/Security/Http - [x]nicolas-grekas#29 src/Symfony/Component/Serializer - [x] src/Symfony/Component/Security/Csrf - [x] src/Symfony/Component/Stopwatch - [x] src/Symfony/Component/Templating - [x]nicolas-grekas#27 src/Symfony/Component/Translation - [x] src/Symfony/Component/Validator - [x] src/Symfony/Component/VarDumper - [x] src/Symfony/Component/VarExporter - [x] src/Symfony/Component/WebLink - [x] src/Symfony/Component/Workflow - [x] src/Symfony/Component/Yaml - [x] src/Symfony/ContractsCommits-------11149a1 Add return-types with help from DebugClassLoader in the CI
Uh oh!
There was an error while loading.Please reload this page.
I've spent a great deal of time on this PR, experimenting with adding return types to the codebase.
TL;DR: my conclusion is that we cannot make it for 5.0.
There are two reasons for this:
What's attached?
a draft patching logic inDebugClassLoaderto add return-type where it discovers this should be donemanual fixes for situations not handled (yet, if possible at all) by that logic in[3.4] Fix return types declarations #33332DebugClassLoaderWhat's achieved? Tests are green \o/
At this stage, I think we have to acknowledge we won't add return-types in 5.0 but prepare a serious plan to add them in 6.0.
This plan could be:
Testsand in generated codeDebugClassLoadertriggers the best possible deprecations that encourage ppl to add return-types in their libs/apps. This means we could decide to disable the current ones (seeReturn type deprecations for library tests #33235) and to re-enable them in 5.1. This will also give us the time to fine-tune the tooling (item 1. on this list)Ideally, we could reach a point where we could test branch 4.4with return-types: we'd use the tooling to add them automatically in the CI job, then we'd run tests and they should be green. Let's do this?
Help Wanted, here is how:
With PHP 7.4, run
php .github/patch-types.php. This will add return types everywhere possible.Then run tests, e.g.
./phpunit src/Symfony/Component/HttpFoundation --exclude-group legacy,issue-32995Here are the components that fail with return types added, please help me check them all with a PR onmy fork: