8000 [5.0] Add parameter type-hints · Issue #32179 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[5.0] Add parameter type-hints #32179

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.

Sign up for GitHub

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

Jump to bottom
Closed
57 tasks done
derrabus opened this issue Jun 25, 2019 · 20 comments
Closed
57 tasks done

[5.0] Add parameter type-hints #32179

derrabus opened this issue Jun 25, 2019 · 20 comments
Milestone
5.0

Comments

@derrabus
Copy link
Member
derrabus commented Jun 25, 2019 •
edited by nicolas-grekas
Loading

The current master branch (which will become Symfony 5) raised the minimum requirement of php to 7.2. That php version introduced a feature called Parameter Type Widening that allows a class to drop parameter type constraints when overriding/implementing a method from a parent class/interface.

This opens up an interesting upgrade path for us: We can add new type-hints to interfaces without breaking implementations that were written against the non-typed version on that interface.

Things to watch out for:

  • Start with interfaces and classes without parent.
  • Don't add type-hints to contracts interfaces: we need php 7.1 compatibility here!
  • You can only add type-hints, not change or remove existing ones.
  • Don't add return types. They require a different strategy.
  • Consult the docblock to find out if you can add a type-hint. But keep in mind that the docblock might lie to you.
  • Adding a type-hint might break code that calls the method: Have a look at the implementations of the interface and their tests to find out what kind of values can be passed to the method right now.
  • Check if null is a valid value. That case is often forgotten in type-hints.
  • Adding a callable type-hint triggers autoloading if the passed value contains a class reference as string. This is a side-effect that should be considered.
  • The method might contain now-obsolete type-checks. You may remove them.
  • The added type-hints might obsolete @param annotations. Remove them if they only contain information that is already reflected by the type-hint and the parameter name.
  • If you find an implementation or child class that is part of another component, don't upgrade the signature just yet if that components should remain compatible with the 4.4 version of the component you're currently working on.

In order to keep the changes reviewable, we should iterate over the components and open one PR per package.

Components

Bundles

Bridges
This was referenced Jun 25, 2019
Merged
Merged
fabpot added a commit that referenced this issue Jun 26, 2019
@fabpot
minor #31984 [EventDispatcher] Add type-hints to EventDispatcherInter…
150f0f2 
…face (derrabus)

This PR was merged into the 5.0-dev branch.

Discussion
----------

[EventDispatcher] Add type-hints to EventDispatcherInterface

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #32179
| License       | MIT
| Doc PR        | N/A

This PR adds type-hints to `EventDispatcherInterface` to give it a more php7-like feeling. Adding these type-hints on master should not be a breaking change because we require php 7.2 there, which allows downstream classes/interfaces to remove a type-hint from a method signature.

Commits
-------

2bc9472 [EventDispatcher] Add type-hints to EventDispatcherInterface.
fabpot added a commit that referenced this issue Jun 26, 2019
@fabpot
minor #32176 [Routing] Add type-hints to all public interfaces (derra…
f123436 
…bus)

This PR was merged into the 5.0-dev branch.

Discussion
----------

[Routing] Add type-hints to all public interfaces

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #32179
| License       | MIT
| Doc PR        | N/A

This PR adds type-hints to all interfaces of the Routing component to give them a more php7-like feeling. Adding these type-hints on master should not be a breaking change because we require php 7.2 there, which allows downstream classes/interfaces to remove a type-hint from a method signature.

Notes:
* There is also an implementation of `RedirectableUrlMatcherInterface` in the Framework Bundle. I did not upgrade its interface in order to keep the bundle compatible with Routing 4.4.
* We could apply similar changes to the `Route`, `RouteCollection` etc. in a follow-up PR. This PR only concentrated on the interfaces and their implementations.

Commits
-------

457b322 [Routing] Add type-hints to all public interfaces.
@nicolas-grekas
Copy link
Member

If we finish #30323, we might have a way to also add return types. I think @fancyweb would appreciate some help there.

@nicolas-grekas nicolas-grekas pinned this issue Jun 26, 2019
@derrabus derrabus mentioned this issue Jun 26, 2019
Merged
@mdeboer
Copy link
Contributor
mdeboer commented Jun 26, 2019

@nicolas-grekas

If we finish #30323, we might have a way to also add return types. I think @fancyweb would appreciate some help there.

I'd be happy to help! As some PRs are already merged, does this mean work on this can be started?

@xabbuh xabbuh added this to the 5.0 milestone Jun 26, 2019
This was referenced Jun 26, 2019
Merged
Merged
@julien57 julien57 mentioned this issue Jun 27, 2019
Merged
This was referenced Jun 27, 2019
Merged
Closed
Merged
@vudaltsov
Copy link
Contributor

@derrabus , I am ready to help with Form component

@derrabus
Copy link
Member Author

A small reminder: This ticket is about parameter type-hints. Please do not add return types (yet).

@derrabus derrabus changed the title [5.0] Upgrade public interfaces with scalar type-hints [5.0] Upgrade public interfaces with scalar parameter type-hints Jun 27, 2019
@lyrixx lyrixx mentioned this issue Jun 27, 2019
Merged
@Tobion
Copy link
Contributor
Tobion commented Jun 27, 2019 •
edited
Loading

The title is wrong and people copy-paste this: It's not only about scalar types. We also want to add object etc parameter types.

@lyrixx lyrixx changed the title [5.0] Upgrade public interfaces with scalar parameter type-hints [5.0] Add parameter type-hints Jun 27, 2019
@lyrixx
Copy link
Member
lyrixx commented Jun 27, 2019

Fixed

@Simperfit
Copy link
Contributor
Simperfit commented Jun 27, 2019 •
edited
Loading

doing lock when #32198 is merged :).

@Simperfit Simperfit mentioned this issue Jun 27, 2019
Merged
@derrabus
Copy link
Member Author

Updated the list, looking into DI now.

This was referenced Jun 27, 2019
Merged
Merged
Merged
@vudaltsov vudaltsov mentioned this issue Jun 27, 2019
Merged
@derrabus derrabus mentioned this issue Jun 27, 2019
Merged
nicolas-grekas added a commit that referenced this issue Aug 19, 2019
@nicolas-grekas
minor #33192 [HttpKernel] Bump dependencies and apply upstream parame…
0b6f45e 
…ter types (derrabus)

This PR was merged into the 5.0-dev branch.

Discussion
----------

[HttpKernel] Bump dependencies and apply upstream parameter types

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #32179
| License       | MIT
| Doc PR        | N/A

This PR bumps some dependencies of the HttpKernel component in order to apply parameter type declarations.

Commits
-------

aa0fc6f [HttpKernel] Bump dependencies and apply upstream parameter types.
nicolas-grekas added a commit that referenced this issue Aug 19, 2019
@nicolas-grekas
minor #33191 [Routing] Add more parameter types (derrabus)
bbea1e6 
This PR was merged into the 5.0-dev branch.

Discussion
----------

[Routing] Add more parameter types

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #32179
| License       | MIT
| Doc PR        | N/A

Commits
-------

daf402e [Routing] Add more parameter types.
@derrabus
Copy link
Member Author

All right, I think we can close the issue now. Thanks a lot again to everyone who contributed to making the Symfony codebase a type-safer place! 😃

@derrabus derrabus mentioned this issue Aug 20, 2019
Merged
nicolas-grekas added a commit that referenced this issue Aug 20, 2019
@nicolas-grekas
minor #33261 Add types to routing and DI configuration traits. (derra…
7eb5fee 
…bus)

This PR was merged into the 4.4 branch.

Discussion
----------

Add types to routing and DI configuration traits.

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | I don't think so.
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #32179, #33228
| License       | MIT
| Doc PR        | N/A

This PR backports the type declarations added to the configurator traits of the Routing and DI components. These traits expose only final methods, so it should be pretty safe to add return types to them.

The only scenario I could make up where this change will break something is if a trait is used to override a method: https://3v4l.org/EAsk8 But I doubt that those traits are used that way.

On master, we've used the `object` return type for the fluent methods. That type is not available on 4.4 where we have to support php 7.1. I'm using `self` instead. Since the methods are final and thus cannot be overridden, I believe that we shouldn't run into covariance issues here, so `self` should be safe.

Commits
-------

1ca30c9 Add types to roting and DI configuration traits.
This was referenced Aug 20, 2019
Merged
Merged
nicolas-grekas added a commit that referenced this issue Aug 21, 2019
@nicolas-grekas
minor #33154 Parameter type leftovers (derrabus)
f83ebd4 
This PR was merged into the 5.0-dev branch.

Discussion
----------

Parameter type leftovers

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #32179
| License       | MIT
| Doc PR        | N/A

Commits
-------

34eda04 Added more parameter type declarations.
@derrabus derrabus mentioned this issue Aug 25, 2019
Closed
3 tasks
fabpot added a commit that referenced this issue Aug 30, 2019
@fabpot
minor #33267 Add parameter type declarations to magic methods (derrabus)
b273561 
This PR was merged into the 5.0-dev branch.

Discussion
----------

Add parameter type declarations to magic methods

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #32179
| License       | MIT
| Doc PR        | N/A

Commits
-------

ec8215c Add parameter type declarations to magic methods.
This was referenced Sep 1, 2019
Merged
Merged
nicolas-grekas added a commit that referenced this issue Sep 8, 2019
@nicolas-grekas
minor #33266 [DependencyInjection] Add types to private/final/interna…
200eae3 
…l methods and constructors (derrabus)

This PR was merged into the 4.4 branch.

Discussion
----------

[DependencyInjection] Add types to private/final/internal methods and constructors

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #32179, #33228
| License       | MIT
| Doc PR        | NA/

Commits
-------

def0ac7 Add types to private/final/internal methods and constructors.
nicolas-grekas added a commit that referenced this issue Sep 8, 2019
@nicolas-grekas
minor #33410 [HttpFoundation] Add types to private/final/internal met…
26954bc 
…hods and constructors (derrabus)

This PR was merged into the 4.4 branch.

Discussion
----------

[HttpFoundation] Add types to private/final/internal methods and constructors

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #32179, #33228
| License       | MIT
| Doc PR        | N/A

Commits
-------

1978d88 [HttpFoundation] Add types to private/final/internal methods and constructors.
@derrabus derrabus mentioned this issue Sep 8, 2019
Merged
nicolas-grekas added a commit that referenced this issue Sep 9, 2019
@nicolas-grekas
minor #33508 [Cache] Add types to constructors and private/final/inte…
312cbf9 
…rnal methods (derrabus)

This PR was merged into the 4.4 branch.

Discussion
----------

[Cache] Add types to constructors and private/final/internal methods

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #32179, #33228
| License       | MIT
| Doc PR        | N/A

I'm currently preparing a large PR collecting changes like these. However, the changeset for the Cache component was large enough to justify a dedicated PR, imho.

Commits
-------

919afd2 [Cache] Add types to constructors and private/final/internal methods.
@derrabus derrabus mentioned this issue Sep 9, 2019
Merged
@derrabus derrabus mentioned this issue Sep 25, 2019
Merged
fabpot added a commit that referenced this issue Sep 27, 2019
@fabpot
minor #33519 Add types to constructors and private/final/internal met…
b1f6d0d 
…hods (Batch I) (derrabus)

This PR was squashed before being merged into the 4.4 branch (closes #33519).

Discussion
----------

Add types to constructors and private/final/internal methods (Batch I)

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #32179, #33228
| License       | MIT
| Doc PR        | N/A

As promised, now a larger batch with the following components:
* Asset
* BrowserKit
* Config
* Console
* ~~CssSelector~~
* Debug
* ~~DomCrawler~~
* DotEnv
* ErrorHandler
* ErrorRenderer
* ExpressionLanguage
* ~~Filesystem~~
* ~~Finder~~

Commits
-------

4039b95 Add types to constructors and private/final/internal methods (Batch I)
This was referenced Sep 30, 2019
Merged
Closed
Tobion added a commit that referenced this issue Oct 3, 2019
@Tobion
minor #33709 Add types to constructors and private/final/internal met…
b2d6c10 
…hods (Batch II) (derrabus)

This PR was squashed before being merged into the 4.4 branch (closes #33709).

Discussion
----------

Add types to constructors and private/final/internal methods (Batch II)

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Tickets       | #32179, #33228
| License       | MIT
| Doc PR        | N/A

Followup to #33519, this time with:
* Form
* HttpClient
* HttpKernel
* intl
* Ldap
* Ldap
* Lock
* Messenger
* Processor
* PropertyInfo
* Routing
* Security
* Serializer
* Stopwatch
* Translation

Commits
-------

9378eb4 Add types to constructors and private/final/internal methods (Batch II)
nicolas-grekas added a commit that referenced this issue Oct 7, 2019
@nicolas-grekas
minor #33770 Add types to constructors and private/final/internal met…
62216ea 
…hods (Batch III) (derrabus)

This PR was squashed before being merged into the 4.4 branch (closes #33770).

Discussion
----------

Add types to constructors and private/final/internal methods (Batch III)

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Tickets       | #32179, #33228
| License       | MIT
| Doc PR        | N/A

Followup to #33709, this time with:
* Validator
* VarDumper
* Workflow
* Yaml
* all bridges
* all bundles

That should be the final batch. 😃

Commits
-------

6493902 Add types to constructors and private/final/internal methods (Batch III)
@LeJeanbono LeJeanbono mentioned this issue Oct 14, 2019
Closed
nicolas-grekas added a commit to derrabus/symfony that referenced this issue Nov 8, 2019
@nicolas-grekas
feature symfony#33497 [Contracts] Add parameter type declarations to …
65c2a11 
…contracts (derrabus)

This PR was merged into the 5.0-dev branch.

Discussion
----------

[Contracts] Add parameter type declarations to contracts

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | symfony#32179
| License       | MIT
| Doc PR        | N/A

This PR proposes to create a php 7.2 version of the contracts that maintains BC with Symfony 4. The PR suggests to bump the contracts version to ~~1.2~~ 2.0 on the master branch. We would still be able to maintain the contracts 1.1 branch on Symfony's 4.4 branch, should we need to patch the current contracts in the future.

This move would allow us to add parameter type declarations to existing contracts interfaces and make use of them in Symfony 5. Especially the Translation and EventDispatcher components benefit a lot from this bump, imho.

Contracts that will be added on the road to Symfony 6 wouldn't be restricted to the capabilities of php 7.1, which would be another benefit in my opinion.

~~<sup>1</sup> Test currently fail because the translator is called with `null` as translation key. That possibility should be deprecated imho.~~

Commits
-------

bf515e1 Add parameter type declarations to contracts.
fabpot added a commit that referenced this issue Nov 9, 2019
@fabpot
feature #33497 [Contracts] Add parameter type declarations to contrac…
9aa7492 
…ts (derrabus)

This PR was merged into the 5.0-dev branch.

Discussion
----------

[Contracts] Add parameter type declarations to contracts

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #32179
| License       | MIT
| Doc PR        | N/A

This PR proposes to create a php 7.2 version of the contracts that maintains BC with Symfony 4. The PR suggests to bump the contracts version to ~~1.2~~ 2.0 on the master branch. We would still be able to maintain the contracts 1.1 branch on Symfony's 4.4 branch, should we need to patch the current contracts in the future.

This move would allow us to add parameter type declarations to existing contracts interfaces and make use of them in Symfony 5. Especially the Translation and EventDispatcher components benefit a lot from this bump, imho.

Contracts that will be added on the road to Symfony 6 wouldn't be restricted to the capabilities of php 7.1, which would be another benefit in my opinion.

~~<sup>1</sup> Test currently fail because the translator is called with `null` as translation key. That possibility should be deprecated imho.~~

Commits
-------

4de3773 Add parameter type declarations to contracts.
hultberg pushed a commit to hultberg/symfony that referenced this issue Sep 17, 2021
@fabpot
minor symfony#32229 [Serializer] [5.0] Add type-hints to serializer i…
b253020 
…nterface (Simperfit)

This PR was merged into the 5.0-dev branch.

Discussion
----------

[Serializer] [5.0] Add type-hints to serializer interface

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | symfony#32179   <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | none <!-- 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/roadmap):
 - 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 4.4.
 - Legacy code removals go to the master branch.
-->

This PR adds type-hints to the serializer component ;).

Commits
-------

68b588c [Serializer] [5.0] Add type-hints to serializer interface
hultberg pushed a commit to hultberg/symfony that referenced this issue Sep 17, 2021
@Tobion
minor symfony#33709 Add types to constructors and private/final/inter…
14c0ace 
…nal methods (Batch II) (derrabus)

This PR was squashed before being merged into the 4.4 branch (closes symfony#33709).

Discussion
----------

Add types to constructors and private/final/internal methods (Batch II)

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Tickets       | symfony#32179, symfony#33228
| License       | MIT
| Doc PR        | N/A

Followup to symfony#33519, this time with:
* Form
* HttpClient
* HttpKernel
* intl
* Ldap
* Ldap
* Lock
* Messenger
* Processor
* PropertyInfo
* Routing
* Security
* Serializer
* Stopwatch
* Translation

Commits
-------

9378eb4 Add types to constructors and private/final/internal methods (Batch II)
PhilETaylor pushed a commit to PhilETaylor/symfony that referenced this issue Sep 6, 2023
@fabpot
minor symfony#33519 Add types to constructors and private/final/inter…
a4db89f 
…nal methods (Batch I) (derrabus)

This PR was squashed before being merged into the 4.4 branch (closes symfony#33519).

Discussion
----------

Add types to constructors and private/final/internal methods (Batch I)

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | symfony#32179, symfony#33228
| License       | MIT
| Doc PR        | N/A

As promised, now a larger batch with the following components:
* Asset
* BrowserKit
* Config
* Console
* ~~CssSelector~~
* Debug
* ~~DomCrawler~~
* DotEnv
* ErrorHandler
* ErrorRenderer
* ExpressionLanguage
* ~~Filesystem~~
* ~~Finder~~

Commits
-------

4039b95 Add types to constructors and private/final/internal methods (Batch I)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
5.0
Development

No branches or pull requests
0