- Issue created by @mstrelan
- π¦πΊAustralia acbramley
Based on https://www.drupal.org/project/drupal/issues/3529510#comment-16141888 π Determine if there are other TestTraits we can add return types to. Active
However, I'm pretty sure if you override a method from a trait then the trait method effectively does not exist, so you can do whatever you want with the signature and PHP won't care. See https://3v4l.org/kW8DR
All but 2 of these functions are adding void so those are all safe.
The other 2 already had @return docs so they aren't technically required here to cleanup the baseline, should we exclude them to make this easier to get in?
- π¦πΊAustralia mstrelan
Good point, thanks for pointing that out. I've reverted those two.
- πΊπΈUnited States xjm
I'd expect an RTBC for this to describe how precisely the many many methods included in this MR were reviewed to ensure that it was actually intentional that they had no return value. :)
- πΊπΈUnited States smustgrave
28 methods none had @return
Trait itself getUpdatedEntityTypeDefinition and getUpdatedFieldStorageDefinitions are the only methods that have a return and those were untouched.
- πΊπΈUnited States xjm
@smustgrave, I was implying that you should actually read the methods (as I stated explicitly on the new sub-meta). :) In general, for any issues that add typehinting, you should actually read the code (at least briefly) rather than relying solely on the docs or static analysis, unless it's impossible in context for the typehint to be anything other than what was added (as it was for actual test runner methods in test classes). As per #3529510-11: [meta] Add return types to test traits because there is no BC break! β :
We simply need to open the methods and verify that they don't return anything under any circumstance (and moreover weren't supposed to, which static analysis can't tell us).
Since we need to review the whole methods to verify that they are, indeed, intended to be
void
, the line length of the methods is the factor we want to look atI will be reading (or at least scanning) every method in the trait to review this for commit.
- πΊπΈUnited States xjm
I read over the full trait. The majority of methods on the trait simply set something-or-other in state; the others perform various operations like entity updates or changes to the entity storage. None had any conceivable code path that could ever have contained a return value.
Saving credits. I'm granting credit for #8 as a best effort, although I would prefer to see a little more detail (and actual review of the code, not just the docs) in future issues for this meta in order to receive credit. Thanks!