Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
User::addRole() does not return a value, meaning the function be changed.
Steps to reproduce
Execute the following code, where $id
is an existing user ID, and $role_id
is the machine name of an existing role.
\Drupal::entityTypeManager()->getStorage('user')
->load($id)
->addRole($role_id)
->save();
An error is thrown that save() is called on NULL.
Proposed resolution
User::addRole() should return $this
.
Remaining tasks
Write tests.
User interface changes
None.
API changes
None needed - this is for forward compatibility.
Data model changes
None.
Release notes snippet
Methods User::addRole()
and User::removeRole()
previously did not return a value, and therefore could not be chained. Now both return $self
and can be used in chained calls.
Previous:
$user->removeRole('editor');
$user->addRole('contributor');
$user->save();
New:
$user
->removeRole('editor')
->addRole('contributor')
->save();
Comment | File | Size | Author |
---|---|---|---|
#27 | 3324729-26.patch | 2.55 KB | andypost |
| |||
#27 | interdiff.txt | 1.54 KB | andypost |
#24 | 3324729-24.patch | 3.88 KB | elber |
|
Comments
Comment #2
Jaypan CreditAttribution: Jaypan commentedComment #3
cilefen CreditAttribution: cilefen as a volunteer commentedComment #4
andypostpatch was broken because using "web" prefix
Here's a test fix for new expectation
Comment #5
Jaypan CreditAttribution: Jaypan commentedComment #6
andypostYou're missed to add a test from my patch
Comment #8
Jaypan CreditAttribution: Jaypan commentedComment #9
andypostyour patch against "web" again( not applicable to core
Comment #10
Jaypan CreditAttribution: Jaypan commentedComment #11
andypostIt may need change record
Comment #12
Jaypan CreditAttribution: Jaypan commentedComment #13
longwaveCan we make
removeRole()
chainable in the same way?Comment #14
Jaypan CreditAttribution: Jaypan commentedI'm sure someone could.
Comment #15
longwaveI think it would be a good idea to change both at the same time, given that addRole and removeRole complement each other. I found this issue because I tried to chain
$user->removeRole('foo')->save()
today and it didn't work.We should also write a short change record to inform developers of this API addition.
Comment #16
Jaypan CreditAttribution: Jaypan commentedI'm going to exit stage left due to bikeshedding, I've already added the existing patch to my current application. Hopefully someone else will come pick this up.
Comment #17
Chandreshgiri Gauswami CreditAttribution: Chandreshgiri Gauswami as a volunteer and at TO THE NEW commentedI will work on it.
Comment #18
Chandreshgiri Gauswami CreditAttribution: Chandreshgiri Gauswami as a volunteer and at TO THE NEW commentedProviding extended patch with interdiff file.
Comment #19
Chandreshgiri Gauswami CreditAttribution: Chandreshgiri Gauswami as a volunteer and at TO THE NEW commentedComment #20
andypostRelease notes snippet needs tuning
Comment #21
andypostAdded CR https://www.drupal.org/node/3326231 and fixed release snippet
Comment #22
quietone CreditAttribution: quietone at PreviousNext commentedJust a wee thing
Pretty sure these can be removed. See #3129184: Remove description from @return $this.
Comment #23
elberComment #24
elberComment #25
andypostScope creep drupal.org/core/scope
this changes are unrelated, please remove it
Comment #26
andypost@quietone Thank you! good old issue) Re #22 Checked core with
git grep -A2 -F ' @return $this'
and the most of places has no value, so it needs to fix coding standard before related issue as we just missing snifferComment #27
andypostComment #28
longwaveThanks, this looks good to me.
As a followup we could start using this across core, e.g. standard.install:
Comment #30
xjmThere is a slight disruption in adding the documentation to the interface, because it means child implementations that return nothing no longer implement the interface as documented and might fail coding standards checks. There's also hypothetically the possibility of disruption if a child implementation is already returning something other than
$this
(like a boolean flag, or whatever). Both those things are fine as disruptions in a minor release with a CR. I don't think it's disruptive enough that we need to add it to the release notes.Tagging "Needs followup" for the issue to implement this where feasible.
Thanks for the test coverage and for keeping the out-of-scope changes out of scope. Committed to 10.1.x and published the CR.
Comment #31
quietone CreditAttribution: quietone at PreviousNext commentedFollow up made, removing tag. #3331229: Use chaining for User::addRole() and ::removeRole()