Problem/Motivation

It's being discussed in the parent to replace calls to uniqid() with something else. If that's done, it would later be rather convenient to prevent it being readded by mistake.

In #2972100-17: Remove usage of uniqid it was introduced https://github.com/ekino/phpstan-banned-code as a potential PHPStan based solution.

Proposed resolution

Experiment and if it's worth adopt the PHPStan extension.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#11 3390368-nr-bot.txt90 bytesneeds-review-queue-bot

Issue fork drupal-3390368

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

mondrake created an issue. See original summary.

mondrake opened merge request !4898

mondrake’s picture

mondrake
Location 🇮🇹
commented
Title: Adopt ekino/phpstan-banned-code and use it to prevent usage of uniqid() » [upstream] Adopt ekino/phpstan-banned-code and use it to prevent usage of uniqid()
Status: Active » Postponed

This detects more than we need :(

I think this is not usable until https://github.com/ekino/phpstan-banned-code/issues/60 is solved upstream... using exit and echo is legit in some of Drupal code and the error thrown can't be disabled.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Title: [upstream] Adopt ekino/phpstan-banned-code and use it to prevent usage of uniqid() » Adopt ekino/phpstan-banned-code and use it to prevent usage of uniqid()
Status: Postponed » Needs review

Actually with just 1 ignoreErrors entry added to phpstan.neon.dist we can make it do exactly what we need for now.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs framework manager review

Seemed to have a CC failure. Tagging for framework manager to make the call too.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Needs review

Actually, the failure is what I would like to be reviewed and commented…

See #2972100-23: Remove usage of uniqid

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented

If we want to use this, we should probably do a dependency evaluation as well?
I think this is a good solution to ensure that we no longer use some of those unwanted functions, but I'm not sure that this phpstan plugin is the right solution, it looks like it contains some rules we don't want to (or can't) use in core itself.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Title: Adopt ekino/phpstan-banned-code and use it to prevent usage of uniqid() » Adopt ekino/phpstan-banned-code and use it to prevent usage of uniqid(), md5(), sha1()

I'm not sure that this phpstan plugin is the right solution, it looks like it contains some rules we don't want to (or can't) use in core itself.

that's why I'd suggest to ignore those that we do not need.

Actually, mglaman/phpstan-drupal is implemented exactly with this model - it has some opinionated rules that we are just ignoring,

    # Ignore common errors for now.
    - "#Drupal calls should be avoided in classes, use dependency injection instead#"
    - "#^Plugin definitions cannot be altered.#"
    - "#^Missing cache backend declaration for performance.#"
mondrake’s picture

mondrake
Location 🇮🇹
commented
Title: Adopt ekino/phpstan-banned-code and use it to prevent usage of uniqid(), md5(), sha1() » Adopt ekino/phpstan-banned-code and use it to prevent further usage of uniqid(), md5(), sha1()
smustgrave’s picture

smustgrave commented

I for one kinda like the idea of being able to prevent certain functions from coming back. But not sure the process for adding a package takes. Would think if we pursue that all functions should be allowed and we 1 by 1 remove any we want.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3390368-nr-bot.txt90 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 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.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Closed (won't fix)

Let's be realistic.

mondrake closed merge request !4898