Problem/Motivation

The UserSession class has a @todo to make all properties protected. The only remaining one in 11.x is the name property.

This issue makes the name property protected and deprecates calling the function directly. This should cause no BC breaks until the deprecation layer is removed in 12.x

Steps to reproduce

Proposed resolution

Make the name property protected
Add a BC layer with __get & __isset

Remaining tasks

Open MR
Review
Merge

API changes

UserSession::name is now protected.

Release notes snippet

CommentFileSizeAuthor
#19 3513856-nr-bot_oc9670as.txt91 bytesneeds-review-queue-bot

Issue fork drupal-3513856

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

danielveza created an issue. See original summary.

danielveza opened merge request !11525

danielveza’s picture

danielveza
he/him
Location Brisbane, AU
commented
Status: Active » Needs review

MR is up for this and green. Moving to review.

I imagine this will need a CR?

How would we want to handle the removal of this code in D12? Do we wait for this to be committed and open a follow up?

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented

Reviewed this and checked the property can still be accessed and there is a test to trigger the deprecation.

RTBC once we have a CR.

> I imagine this will need a CR?

Yep.

> How would we want to handle the removal of this code in D12? Do we wait for this to be committed and open a follow up?

I think this would get handled when we make deprecation removal issues when 12.x opens up.

danielveza’s picture

danielveza
he/him
Location Brisbane, AU
commented

CR created: https://www.drupal.org/node/3513877

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Status: Needs review » Reviewed & tested by the community

All good now.

danielveza’s picture

danielveza
he/him
Location Brisbane, AU
commented

Feedback addressed and tests are green again

mstrelan’s picture

mstrelan commented

Thanks for addressing the isset feedback. I've left one more minor nit but happy to leave at RTBC.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work
danielveza’s picture

danielveza
he/him
Location Brisbane, AU
commented
Status: Needs work » Needs review

Addressed feedback, tests are green

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs Review Queue Initiative

Sorry but think this missed 11.2, can we update the deprecations for 11.3 please.

What's a recommended way to test this one for when it comes back around.

If you are another contributor eager to jump in, please allow the previous poster(s) at least 48 hours to respond to feedback first, so they have the opportunity to finish what they started!

danielveza’s picture

danielveza
he/him
Location Brisbane, AU
commented

Updated the deprecations to 11.3.

In terms of testing this, there isn't a way to do it via the UI but you can create a PHP script that pretty much follows the test coverage in testNamePropertyDeprecation and run that with drush php:script

Something like

$user = new UserSession([
  'name' => 'test',
]);
// This should throw a deprecation
echo $user->name;
echo $user->getAccountName();
$user->foo = 'test';
echo $user->foo;
// This should throw an exception
echo $user->mail;
mstrelan’s picture

mstrelan commented

I was thinking the removal might need to be deferred to 13.x as per #3518671: [policy, no patch] Defer disruptive 11.3 deprecations for removal until 13.0 but since getAccountName already exists contrib can still support 11.x and 12.x simultaneously by updating to use the getter.

danielveza’s picture

danielveza
he/him
Location Brisbane, AU
commented
Status: Needs work » Needs review

Deprecations updated, tests are green

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Think I agree with @acrambley shouldn't set also be deprecated

riyas_nr made their first commit to this issue’s fork.

riyas_nr’s picture

riyas_nr commented
Status: Needs work » Needs review

Deprecated the set method and added corresponding test coverage. Moving to NR.

smustgrave’s picture

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

Feedback appears to be addressed

needs-review-queue-bot’s picture

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

smustgrave’s picture

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

Conflict in core/tests/Drupal/Tests/Core/Session/UserSessionTest.php

smustgrave’s picture

smustgrave commented

JK there were some new phpstan failures https://git.drupalcode.org/issue/drupal-3513856/-/jobs/6364907

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed 03b1887 and pushed to 11.x. Thanks!

  • alexpott committed 03b18875 on 11.x 
    Issue #3513856 by danielveza, smustgrave, riyas_nr, kim.pepper, mstrelan...

Status: Fixed » Closed (fixed)

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

quietone closed merge request !11525