Skip to content

[ErrorHandler] make DebugClassLoader able to add return type declarations #33283

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

Merged
merged 1 commit into from
Aug 26, 2019

Conversation

nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Aug 21, 2019

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.1

@nicolas-grekas nicolas-grekas added this to the next milestone Aug 21, 2019
@nicolas-grekas nicolas-grekas force-pushed the dbc++ branch 2 times, most recently from ecec254 to fea3574 Compare August 26, 2019 09:43
@nicolas-grekas nicolas-grekas changed the title [ErrorHandler] improve DebugClassLoader patching logic [ErrorHandler] make DebugClassLoader able to add return type declarat Aug 26, 2019
@nicolas-grekas nicolas-grekas changed the title [ErrorHandler] make DebugClassLoader able to add return type declarat [ErrorHandler] make DebugClassLoader able to add return type declarations Aug 26, 2019
@nicolas-grekas
Copy link
Member Author

/cc @mdlutz24 @alexpott this might be of interest for Drupal

nicolas-grekas added a commit that referenced this pull request Aug 26, 2019
…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.1

Commits
-------

72f6a97 [ErrorHandler] make DebugClassLoader able to add return type declarations
@nicolas-grekas nicolas-grekas merged commit 72f6a97 into symfony:4.4 Aug 26, 2019
nicolas-grekas added a commit that referenced this pull request Aug 26, 2019
…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`~ #33332

What'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/Contracts

Commits
-------

11149a1 Add return-types with help from DebugClassLoader in the CI
@nicolas-grekas nicolas-grekas deleted the dbc++ branch September 1, 2019 12:27
@nicolas-grekas nicolas-grekas modified the milestones: next, 4.4 Oct 27, 2019
This was referenced Nov 12, 2019
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.

2 participants