- Issue created by @mondrake
- 🇮🇹Italy mondrake 🇮🇹
Needs tests for the rule and some cleanup, but I am opening it for review of the concept.
This shows how adding the
@return-type-will-be-added
to some methods ofImageInterface
, PHPStan identifies that the implementing methods on the concreteImage
class need to get the return type. - 🇮🇹Italy mondrake 🇮🇹
Reverted changes to code that were meant to showcase the rule. The example pipeline were the PHPStan output is visible is https://git.drupalcode.org/issue/drupal-3486376/-/pipelines/333846
Added PHPUnit tests for the PHPSTan rule.
- 🇺🇸United States smustgrave
Wonder if this counts as an API change?
Or least warrants a CR?
Only moving to NR for the pipeline revert as not sure it's in scope.
If can rebase too since it's kinda older (sorry took so long to look at)
If you are another contributor eager to jump in, please allow the original poster @mondrake at least 48 hours to respond to feedback first, so they have the opportunity to finish what they started!
- 🇮🇹Italy mondrake 🇮🇹
It certainly needs a CR to explain how to use the annotation, but let's get more reviews and consensus before writing one and letting it go stale because of later changes of direction.
The path addition in CI is needed to get the rule tested (HEAD is currently checking only changes to file in the base path of Drupal's PHPStan-related stuff).
The Symfony classloader proposed changes were closed without commit; this remains the only approach currently workable to address this issue.
- 🇮🇹Italy mondrake 🇮🇹
BTW I am not even sure this is for core or rather for mglaman/phpstan-drupal.
- Issue was unassigned.
- Status changed to Needs review
3 months ago 7:35pm 13 January 2025 - First commit to issue fork.
- 🇺🇸United States cmlara
I have put some comments on Slack however making them more formal:
As a Contrib maintainer I do not see the value.
For me PHPStan exists to identify bugs in code, not future compatibility problems. Not having a return type when the parent method itself does not have a return type is not a bug.
PHPStan scans your whole codebase and looks for both obvious & tricky bugs. Even in those rarely executed if statements that certainly aren't covered by tests.
Looking at a few of the projects I maintain there is a good chance there will be a different branch for D12 and my current branches will never need to implement a hard return type (although in many cases I already added strong code typing as part of implementing Level 9 PHPStan).
A 'feature' like this will just be noise to me, and distract from working on issues more pressing (security hardening, bug fixes, and features).
My opinion is that 🌱 Consider always having the next major branch open Active is the better method to implement, it would allow modules to opt-in to scanning against the next major core branch early to identify where they need to make changes, and reduce creating future technical debt (no need to come back in D12 to remove the type-hint warning).
As an aside. I have suggested in Slack that if this is felt to be useful it might be worth considering putting this in its own third party repository to allow it to be available to others as a way for the Drupal Community to "give back" for all it receives. If this does not make sense to be an independent project I would suggest that should be treated as a warning that indicates other methods should be preferred.
Looking at the MR itself: I note the presence of
// @phpstan-ignore phpstanApi.method
, should code that uses PHPStan off-api be allowed considering the maintenance burden that could create? - 🇺🇸United States smustgrave
Should this be postponed on 🌱 Consider always having the next major branch open Active