Skip to content

Add type declarations to code examples #12235

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

Closed
3 tasks
derrabus opened this issue Aug 25, 2019 · 10 comments
Closed
3 tasks

Add type declarations to code examples #12235

derrabus opened this issue Aug 25, 2019 · 10 comments

Comments

@derrabus
Copy link
Member

derrabus commented Aug 25, 2019

The Symfony codebase is being modernized by adding type declarations. I think that we should do the same here as well.

My suggestions:

  • Example code that does not override/implement a Symfony method, should be typed.
  • Example code that overrides/implements a Symfony method should be updated for 5.0 to match the added parameter type declarations.
  • Example code that overrides/implements a Symfony method should turn an upstream @return annotation into an explicit return type declaration if possible. In most cases, we can do this for the 4.4 docs already.

Related:

@javiereguiluz
Copy link
Member

I'd love to have this ... but I'd hate to do this. Adding types at once would create millions of conflicts when merging this in all the upper branches. We don't have resources to do that, so that's why we can't do it.

However, we should start adding types to the new code and new examples added in 5.0 and higher branches.

@wouterj
Copy link
Member

wouterj commented Oct 13, 2019

I don't think this will create merge conflicts: (1) will be updated in all maintained versions, so there are no conflicts. For (2) and (3), it can cause some merge conflicts between 4.3 and 4.4, but this is minimal as method signatures do not change much between Symfony versions (BC promise), thus changes in these lines for 4.3 hardly conflict with change in the same line in 4.4.

I'm happy to start working on this, promoting industry best practices is the best thing we can do in the docs :).

@derrabus you said "In most cases, we can do this for the 4.4 docs already." about the return type hints. Can you explain/show some examples of which cases this is not possible? (I could not find it in symfony/symfony#33228 )

@derrabus
Copy link
Member Author

derrabus commented Oct 13, 2019

(1) will be updated in all maintained versions, so there are no conflicts.

By "all maintained versions" you also mean 3.4? Since Symfony 3.4 still supports php 5, I'm not sure if we should have code examples there using syntax exclusive to php 7.

For (2) and (3), it can cause some merge conflicts between 4.3 and 4.4, but this is minimal as method signatures do not change much between Symfony versions (BC promise), thus changes in these lines for 4.3 hardly conflict with change in the same line in 4.4.

(2) can only be done on 5.0 because the added parameter type declarations are not present in 4.4 (mainly because php 7.1 doesn't allow parameter type widening).

I'm happy to start working on this, promoting industry best practices is the best thing we can do in the docs :).

I'd also be happy to help here. Maybe we can find a way to organize/split the work here, in case others want to join as well.

@derrabus you said "In most cases, we can do this for the 4.4 docs already." about the return type hints. Can you explain/show some examples of which cases this is not possible? (I could not find it in symfony/symfony#33228 )

If we add return types to implementations of Symfony interfaces, we would anticipate the return type declaration that Symfony 6 will add eventually. That's a deep dive into the crystal ball, so in cases where we're not sure how a @return will be turned into a return type declraration, we should probably not add it.

In all other cases, we should be pretty safe now because on the 4.4 and 5.0 branches, Travis runs Symfony's test suite on php 7.4 against a version of Symfony that is patched with return types inferred from @return annotations.

@wouterj
Copy link
Member

wouterj commented Oct 28, 2019

Alright, I shortly discussed this with the rest of the team and we agreed on keeping PHP 7.1 compatible code in the Symfony 4.x docs (many people are probably still on PHP 7.1, so it's better to avoid confusion).

So this means the work that has to be done is:

  1. All types that do not extend Symfony code: Fix in 4.4 and master
  2. Parameter types: Fix in master
  3. Return types: Fix in 4.4 and master

I would suggest to split this work on the "topics" and to start focussing on the main guides in the root and waiting for the components docs till the very end.

@wouterj
Copy link
Member

wouterj commented Apr 19, 2020

Fyi, I started working on this issue in #13564 . For now, the changes seems quite minor (but getting started guides mostly discuss the controller).

@wouterj
Copy link
Member

wouterj commented Oct 21, 2020

Related to (2) #12867

@derrabus
Copy link
Member Author

Great, thanks @atailouloute!

wouterj added a commit that referenced this issue Oct 21, 2020
…outerj)

This PR was merged into the 4.4 branch.

Discussion
----------

Added PHP type declarations to getting started guides

Relates to #12235

I've scanned all PHP examples in the getting started guides and updated them to use PHP typing consistently (most already used typing in some way or another).

Practical things:
* Once approved, I'll take care of merging this into 5.0 and master and open new PRs for new code blocks in those branches;
* I suggest not touching 3.4 at all, we should not update that version anymore unless there is a major bug (which is unlikely, as it's around for years already);
* Method declarations almost never change (because of the BC promise), and as we're fixing these in all active branches (except 3.4), this change will not lead to persistent merge conflicts in the future.
* I don't think we can add a DOCbot rule for this one, as we cannot introduce typing in all PHP examples everywhere (see the mentioned issue).

Commits
-------

e1277a5 Added PHP typehints to getting started guides
@OskarStark
Copy link
Contributor

I think you meant "@wouterj" instead? 😋

wouterj added a commit that referenced this issue Nov 21, 2020
…ons to Form, DI and Doctrine articles (wouterj)

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

Discussion
----------

[DependencyInjection][Doctrine][Form] Add type declarations to Form, DI and Doctrine articles

Fixes part of #12235

Now the getting started articles are fixed, this PR continues with the 3 most popular doc topics.

Commits
-------

8f8621d Added PHP types to the DI related articles
aec1016 Added PHP types to the Form related articles
90c83b2 Added PHP types to the Doctrine related articles
javiereguiluz added a commit that referenced this issue Apr 21, 2021
…(wouterj)

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

Discussion
----------

[Console][Mailer][Security] Added PHP type declarations

Continues some work on #12235 for the next 3 most popular topics: security, mailer and console.

Commits
-------

23dc098 [Console][Mailer][Security] Added PHP type declarations
@javiereguiluz
Copy link
Member

Can we close this issue? We've already introduced hundreds of types in #13564, #14525 and #14591. We can add the remaining types in future PRs. Thanks.

@derrabus
Copy link
Member Author

All right.

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

4 participants