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

[5.0] Add parameter type-hints #32179

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

Comments

@derrabus
Copy link
Member

derrabus commented Jun 25, 2019

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

fabpot added a commit that referenced this issue Jun 26, 2019
…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
…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.

@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?

@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
@Tobion
Copy link
Member

Tobion commented Jun 27, 2019

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

doing lock when #32198 is merged :).

@derrabus
Copy link
Member Author

Updated the list, looking into DI now.

nicolas-grekas added a commit that referenced this issue Aug 19, 2019
…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
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! 😃

nicolas-grekas added a commit that referenced this issue Aug 20, 2019
…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.
nicolas-grekas added a commit that referenced this issue Aug 21, 2019
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.
fabpot added a commit that referenced this issue Aug 30, 2019
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.
nicolas-grekas added a commit that referenced this issue Sep 8, 2019
…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
…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.
nicolas-grekas added a commit that referenced this issue Sep 9, 2019
…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.
fabpot added a commit that referenced this issue Sep 27, 2019
…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)
Tobion added a commit that referenced this issue Oct 3, 2019
…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
…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)
nicolas-grekas added a commit to derrabus/symfony that referenced this issue Nov 8, 2019
…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
…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
…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
…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
…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
Labels
None yet
Projects
None yet
Development

No branches or pull requests