Problem/Motivation

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

There are 16 violations where a parameter of a function expects an int but receives a string. Most of the time this is a documentation issue, where the function actually should take a string, or can take a numeric string, but a handful of times it should take an int that we need to cast.

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 | grep "expects int, string given"

Proposed resolution

Fix the param docs, or cast where necessary.

core/modules/comment/src/CommentManager.php:199:Parameter #2 $error_level of function trigger_error expects int, string given.
core/modules/file/src/FileAccessControlHandler.php:97:Parameter #3 $age of function file_get_file_references expects int, string given.
core/modules/file/src/Hook/FileDownloadHook.php:48:Parameter #3 $age of function file_get_file_references expects int, string given.
core/modules/help/tests/src/Functional/HelpTopicsSyntaxTest.php:341:Parameter #2 $value of static method Drupal\help_topics_twig_tester\HelpTestTwigNodeVisitor::setStateValue() expects int, string given.
core/modules/link/tests/src/Functional/LinkFieldTest.php:388:Parameter #1 $id of method Drupal\Tests\link\Functional\LinkFieldTest::renderTestEntity() expects int, string given.
core/modules/link/tests/src/Functional/LinkFieldTest.php:401:Parameter #1 $id of method Drupal\Tests\link\Functional\LinkFieldTest::renderTestEntity() expects int, string given.
core/modules/migrate_drupal_ui/src/Form/CredentialForm.php:444:Parameter #2 $drupal_version of method Drupal\migrate_drupal_ui\Form\MigrateUpgradeFormBase::getMigrations() expects int, string given.
core/modules/system/src/Controller/AssetControllerBase.php:181:Parameter #2 $group_delta of method Drupal\system\Controller\AssetControllerBase::getGroup() expects int, string given.
core/modules/views/src/Plugin/views/field/NumericField.php:184:Parameter #1 $count of static method Drupal\Core\StringTranslation\PluralTranslatableMarkup::createFromTranslatedString() expects int, string given.
core/modules/workspaces/src/WorkspaceMerger.php:66:Parameter #1 $seconds of function set_time_limit expects int, string given.
core/modules/workspaces/src/WorkspacePublisher.php:102:Parameter #1 $seconds of function set_time_limit expects int, string given.
core/profiles/demo_umami/modules/demo_umami_content/src/InstallHelper.php:484:Parameter #2 $term_csv_id of method Drupal\demo_umami_content\InstallHelper::getTermId() expects int, string given.
core/profiles/demo_umami/modules/demo_umami_content/src/InstallHelper.php:526:Parameter #2 $term_csv_id of method Drupal\demo_umami_content\InstallHelper::getTermId() expects int, string given.
core/profiles/demo_umami/modules/demo_umami_content/src/InstallHelper.php:584:Parameter #2 $term_csv_id of method Drupal\demo_umami_content\InstallHelper::getTermId() expects int, string given.
core/scripts/run-tests.sh:1645:Parameter #2 $color_code of function simpletest_script_print expects int, string given.
core/scripts/run-tests.sh:1646:Parameter #2 $color_code of function simpletest_script_print expects int, string given.

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#11 3557514-nr-bot_ake65elf.txt91 bytesneeds-review-queue-bot

Issue fork drupal-3557514

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 !13813

mstrelan’s picture

mstrelan commented
Status: Active » Needs review

Left some commentary on the MR

dcam’s picture

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

I ran PHPStan with the rule turned on. I only got 16 errors instead of the 19 mentioned in the IS. I don't know if other recent updates fixed a few of them. The MR corrected all of the issues PHPStan found.

Afterward I read through all of the changes. They all seem reasonable. I would leave the int casts in this MR rather than splitting them off, especially the ones from Workspaces. I left a couple of replies to the comments, agreeing with the assessments.

This one's looking good to me. I'll RTBC it.

mstrelan’s picture

mstrelan commented
Issue summary: View changes
Status: Reviewed & tested by the community » Needs work

Thanks for the review.

The following issues were fixed in #3554676: Fix more incorrect phpdoc type hints (part 3):

core/modules/menu_ui/tests/src/Functional/MenuUiTest.php:543:Parameter #2 $parent of method Drupal\Tests\menu_ui\Functional\MenuUiTest::moveMenuLink() expects int, string given.
core/modules/migrate/src/Plugin/MigrationPluginManager.php:180:Parameter #2 $id of method Drupal\migrate\Plugin\MigrationPluginManager::addDependency() expects int, string given.
core/modules/migrate/src/Plugin/MigrationPluginManager.php:182:Parameter #2 $id of method Drupal\migrate\Plugin\MigrationPluginManager::addDependency() expects int, string given.
core/modules/migrate/src/Plugin/MigrationPluginManager.php:187:Parameter #2 $id of method Drupal\migrate\Plugin\MigrationPluginManager::addDependency() expects int, string given.

There is also a new error introduced by #3543035: Deprecate CommentManagerInterface::getCountNewComments:

core/modules/comment/src/CommentManager.php:199:Parameter #2 $error_level of function trigger_error expects int, string given.

I've updated the issue summary and will rebase and fix the new issue.

mstrelan’s picture

mstrelan commented
Status: Needs work » Needs review
dcam’s picture

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

The new change makes sense - that's a constant after all. After downloading the latest changes PHPStan reported no errors.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

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

Should we add this permanently?

mstrelan’s picture

mstrelan commented

@longwave yes that's what the meta is for, there are still a few hundred violations I'm trying to solve first to keep the baseline down.

It's on by default in level 5 or 6 but we can benefit from using it manually.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented

This it looks very good and it will be helpful to get this in. I am not 100% sure about the fact that we've added type casts as well as documentation changes here, but since there are only 2, to me it seems not to be an issue.

RTBC++

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3557514-nr-bot_ake65elf.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 » Reviewed & tested by the community

Rebased cleanly:

$ git rebase 11.x
warning: skipped previously applied commit 8188832cf7d
hint: use --reapply-cherry-picks to include skipped commits
hint: Disable this message with "git config set advice.skippedCherryPicks false"
Successfully rebased and updated refs/heads/3557514-int-string.

Note the skipped commit was fixed in #3563876: TypeError: trigger_error(): Argument #2 ($error_level) must be of type int, string given in trigger_error().

catch closed merge request !13813

  • catch committed 79e7d04e on 11.x 
    fix: #3557514 Fix "expects int, string given" issues detected by phpstan...

  • catch committed b9bd2cf5 on main 
    fix: #3557514 Fix "expects int, string given" issues detected by phpstan...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

I think the casting is OK here. Committed/pushed to main and 11.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.

Status: Fixed » Closed (fixed)

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