-
-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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
Comments
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. |
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 ) |
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.
(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'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.
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
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 |
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:
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. |
Fyi, I started working on this issue in #13564 . For now, the changes seems quite minor (but getting started guides mostly discuss the controller). |
Related to (2) #12867 |
Great, thanks @atailouloute! |
…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
I think you meant "@wouterj" instead? 😋 |
…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
…(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
All right. |
The Symfony codebase is being modernized by adding type declarations. I think that we should do the same here as well.
My suggestions:
@return
annotation into an explicit return type declaration if possible. In most cases, we can do this for the 4.4 docs already.Related:
The text was updated successfully, but these errors were encountered: