Problem/Motivation

Follow-up from #3410098: Deprecate passing non-strings to UserSession::hasPermission() and User::hasPermission(), created as per https://www.drupal.org/about/core/policies/core-change-policies/drupal-d...

Proposed resolution

Add type-hint to the methods according to #step-2-implement-the-new-signature

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3415189

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

kristiaanvandeneynde created an issue. See original summary.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented
Issue tags: +Major version only
quietone’s picture

quietone commented
Issue tags: +Needs issue summary update

I reviewed this issue because I was doing triage on the originating issue.

The issue summary here should be more helpful. It should explain what work is to be completed here. As it is, a contributor has to read the parent issue and the change there first. And hopefully, they will find comment #19 over there that links to the deprecation policy that explains part of the process.

Also, the standard template should be restore so that all sections are available. Those sections are helpful in many ways. It is the first thing someone a contributor see when deciding to work on an issue so it should be inviting and explain the work clearly. It is also how we track of what work has been completed and what still needs to be done. And, speaking for myself, it is too often that I have to restore the template so I can complete a review. I'd rather not have to do that.

Typically, I summarize the remaining work in the remaining tasks section but I will do so here.

  1. Restore the standard issue summary template
  2. Complete the template, explaining clearly what work is to be done here. Add link to the correct section of the Deprecation Policy.
  3. Consider tagging as Novice.
quietone’s picture

quietone commented
Status: Active » Needs work
andypost’s picture

andypost
he/him
Primary language Russian
commented
Issue summary: View changes
Status: Needs work » Active
Parent issue: #3410098: Deprecate passing non-strings to UserSession::hasPermission() and User::hasPermission() » #3050720: [Meta] Implement strict typing in existing code
Related issues: +#3410098: Deprecate passing non-strings to UserSession::hasPermission() and User::hasPermission()

As blocker merged, tuned IS a bit

andypost’s picture

andypost
he/him
Primary language Russian
commented
Issue summary: View changes
Issue tags: -Needs issue summary update

andypost opened merge request !6851

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Active » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Seems to have test failures.

andypost’s picture

andypost
he/him
Primary language Russian
commented

Looks like return-type needs more work in unit tests

quietone’s picture

quietone commented
Title: [11.x] Tighten typehint in UserSession::hasPermission() and User::hasPermission() » Tighten typehint in UserSession::hasPermission() and User::hasPermission()
mondrake’s picture

mondrake
Location 🇮🇹
commented

Added a comment inline in the MR.

damienmckenna’s picture

damienmckenna
Location TN, USA
commented

Core maintainers, do you agree with mondrake's comment about the return type?

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented

10.3 has shipped already and it feels a bit odd to patch in a return type hint right before the release of 11. I would rather we drop the return type hint altogether and stick to the bare essentials: typehint the permission as a string and enforce this in 11.x

For that matter, we need to be quick about this too as 11.0 is to be released this week.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented
Status: Needs work » Closed (outdated)

Seems like this was fixed in #3432868: Remove deprecated code from user module