Problem/Motivation

Part of #3404246: [META] Fix strict type errors detected by phpstan.

By correcting 22 incorrect documented types we resolve 90 detected issues.

Steps to reproduce

Add checkFunctionArgumentTypes: true to the parameters section of phpstan.neon.dist

Run phpstan:
$ ./vendor/bin/phpstan analyse -c core/phpstan.neon.dist --error-format=raw

Compare 11.x to this branch

Proposed resolution

Fix doc types to resolve the attached errors

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#23 3552759-nr-bot_ki5q0_3y.txt3.14 KBneeds-review-queue-bot
#17 3552759-nr-bot_u25lsytj.txt91 bytesneeds-review-queue-bot
#13 3552759-fixes.txt25.65 KBmstrelan
phpdoc-errors.txt26.9 KBmstrelan

Issue fork drupal-3552759

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

mstrelan created an issue. See original summary.

mstrelan opened merge request !13512

mstrelan’s picture

mstrelan commented
Status: Active » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Left some comments. But shouldn't the descriptions be updated too to match what's being changed?

mstrelan’s picture

mstrelan commented
Status: Needs work » Needs review

Left some comments. But shouldn't the descriptions be updated too to match what's being changed?

From my perspective that just opens up more for reviewers and committers to have to parse and agree on. You could also argue that if we're updating the doc types we should also be adding native typing too, but that's not in scope, so why are descriptions? Nevertheless I've taken a stab at this.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

Fair point. I’m going to mark as this does seem to be a net gain plus.

Thanks for making those changes!

quietone’s picture

quietone commented

How was this group of changes selected? They seem to be a variety of types.

mstrelan’s picture

mstrelan commented

They are selected at random from looking at a list of around 400 errors and picking ones that occurred several times or looked easy to fix. I have another batch ready to go but this feels like a good size. I don't think there is a way to group them that is particularly meaningful.

quietone’s picture

quietone commented
Assigned: Unassigned » quietone

@mstrelan, thanks. I scanned this earlier and didn't see any problems. It is now late and I will review again tomorrow, so assigning to myself.

quietone’s picture

quietone commented
Assigned: quietone » Unassigned
Status: Reviewed & tested by the community » Needs work

Just some removals to keep this in scope, more details in the MR.

mstrelan’s picture

mstrelan commented
Status: Needs work » Needs review

Reverted the two changes that include description updates. Not opening follow ups for these yet as they'll eventually be picked up in the existing meta.

dcam’s picture

dcam commented

I did manual checks on all the changes. Overall they look OK to me. I left comments for the couple of things that I noticed.

mstrelan’s picture

mstrelan commented
Issue summary: View changes
StatusFileSize
new 3552759-fixes.txt25.65 KB

Rebased and found a few that had been fixed in sibling issues. Reverted change to DataDefinitionInterface, we can discuss it in another issue. This PR now has 22 fixes, resolving 90 issues. Ready for another review.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Sorry appears to need a rebase.

mstrelan’s picture

mstrelan commented
Title: Fix more incorrect phpdoc type hints » Fix more incorrect phpdoc type hints (part 1)
Status: Needs work » Needs review

Rebased

dcam’s picture

dcam commented
Status: Needs review » Reviewed & tested by the community

I ran PHPStan before and after the changes. I verified that the changes reduce the violation count by 90. My recent feedback was addressed. Tests are passing. This looks good to me.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3552759-nr-bot_u25lsytj.txt91 bytes

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.

mstrelan’s picture

mstrelan commented
Status: Needs work » Needs review

Rebased and responded to feedback

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Leaning on previous reviews, but feedback appears to be addressed.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented

I have reviewed this as well, this looks great, and all questions are answered indeed. rtbc++

mstrelan’s picture

mstrelan commented

Rebased on main, there was one conflict in MigrationConfigurationTrait::getMigrations that was updated in #3557514: Fix "expects int, string given" issues detected by phpstan and I accepted theirs, meaning the param is typed as numeric instead of string. Leaving the status as it is.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3552759-nr-bot_ki5q0_3y.txt3.14 KB

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. 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.

dcam’s picture

dcam commented
Status: Needs work » Reviewed & tested by the community

Post-bot-rebellion rebase

mstrelan’s picture

mstrelan commented
Status: Reviewed & tested by the community » Needs review

Rebased. There was a conflict with AnnotatedClassDiscovery because #3567159: Fix "expects array<string>" issues detected by phpstan also touched this class, so I think at least that file needs review.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Needs review » Reviewed & tested by the community

That change looks good to me.

  • longwave committed 990c5fe0 on 11.3.x 
    docs: #3552759 Fix more incorrect phpdoc type hints (part 1)

By:...

  • longwave committed 32caacf9 on 11.x 
    docs: #3552759 Fix more incorrect phpdoc type hints (part 1)

By:...

  • longwave committed 126e7745 on main 
    docs: #3552759 Fix more incorrect phpdoc type hints (part 1)

By:...
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Version: main » 11.3.x-dev
Status: Reviewed & tested by the community » Fixed

Everything here looks good to me. There is one case where File could have been FileInterface but it's only in a test and not worth the effort.

Backported to 11.3.x as a docs-only fix. Doesn't apply to 10.6.x, not sure it's worth the effort there either.

Committed and pushed 126e77458b2 to main and 32caacf906b to 11.x and 990c5fe085c to 11.3.x. Thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

longwave closed merge request !13512

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.