- Issue created by @mstrelan
- Status changed to Needs review
about 2 months ago 12:34am 15 July 2024 - π«π·France andypost
Stats for baseline looks big but looks like it should be better then growing amount of newcode
+ 75718 β 1390
- Status changed to Needs work
about 2 months ago 3:10pm 17 July 2024 - Status changed to Needs review
about 2 months ago 8:23pm 17 July 2024 - π¦πΊAustralia mstrelan
Obviously it will need constant rebasing. Setting back to NR to get actual review.
- Status changed to Needs work
about 2 months ago 1:42pm 18 July 2024 The Needs Review Queue Bot β tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide β to find step-by-step guides for working with issues.
- Status changed to Needs review
about 2 months ago 10:48pm 18 July 2024 - π¦πΊAustralia mstrelan
Let's just review the idea and the changes to
phpstan.neon.dist
. If we're happy with that then we can regenerate the baseline. We can also review what's currently being added to the baseline, even if it's not mergeable, to verify that we're onlyidentifier: missingType.return
. - πΊπΈUnited States smustgrave
Enable MissingFunctionReturnTypehintRule and MissingMethodReturnTypehintRule phpstan rules
What if we broke these out into 2 tickets would that be easier?
- π¦πΊAustralia mstrelan
$ grep "Method" 8763.diff | wc -l 10753 $ grep "Function" 8763.diff | wc -l 7335
Not sure there's any difference reviewing 10000 or 17000 changes.
- πΊπΈUnited States smustgrave
Would it be easier to breakout into groups of components or group of modules?
Just brainstorming to help keep the idea moving.
- π¦πΊAustralia mstrelan
I appreciate that the MR is massive but really the only change that needs review at all is to phpstan.neon.dist. Everything else is proven by the phpstan job passing. I don't think committing in chunks will help, this probably just needs "Needs ____ review" but I don't know what ____ is, maybe FM?
- πΊπΈUnited States smustgrave
Framework would probably be best. And would just ping them.
- π³πΏNew Zealand quietone New Zealand
Can you add the current size of the baseline and the size with this MR?
- π¦πΊAustralia mstrelan
Rebased, re-baselined and rewrote the issue summary. FWIW I don't know why some lines were removed from the baseline, but that can only be a good thing as long as the phpstan job still passes.
- πΊπΈUnited States smustgrave
So what would be the scope or game plan to address these eventually? So people arenβt doing a bunch of tickets
- π¦πΊAustralia mstrelan
This issue is not about adding the return types to existing methods, it's just about preventing new methods from being added that don't have a return type. I think adding return types to methods in run time code needs BC consideration most of the time.
- π¦πΊAustralia acbramley
Big +1 on this and agree with #12. The longer we wait to do this the larger the potential baseline as more and more code gets added, I know we're pretty good at asking for return types but this would enforce it.
- π³π±Netherlands Spokje
*jumps on the +1 bandwagon*
I think adding return types to methods in run time code needs BC consideration most of the time.
Having tried to work on PHPStan lvl 1 issues in the current baseline in a previous life, I very much think this is the case.
I see a slight risk of people adding a new
missingType.return
error to the baseline, since they're seeing the current ones also being in the baseline, but with a explanatory CR and, since almost nobody will read that, a vigilant NR and core committers crew (so no problem there :) we will be fine. - Status changed to RTBC
11 days ago 12:33pm 28 August 2024 - πΊπΈUnited States smustgrave
Also imagine if this gets in a number of tickets may get sent back
- π³π±Netherlands bbrala Netherlands
This is a great step imo to start enforcing.
This might also need an update to the Coding Standards. And if this is enforced do we need to say something about new classes that might not be able to add since they for example implement an interface or extend a base class? There might be place where this is imossoble.
- π¦πΊAustralia mstrelan
Due to covariance you can add a return type to a child class when the parent or interface doesn't have one. See https://www.php.net/manual/en/language.oop5.variance.php