Use chaining for User::addRole() and ::removeRole()

Patch for replacements on places where "addRole" is used. With some phpcs fixes also.
Thanks

Thanks for your interest in working on this issue.

Note that we should not mix out-of-scope coding standards fixes into patches because it makes them harder to review and creates merge conflicts. Reference:
https://www.drupal.org/docs/develop/issues/issue-procedures-and-etiquett...

1. +++ b/core/modules/ckeditor5/tests/src/FunctionalJavascript/MediaTest.php
   @@ -831,10 +830,10 @@ public function testTranslationAlt() {
   -    // cSpell:disable-next-line
   +    // cSpell:disable-next-line.
        $this->assertNotEmpty($assert_session->waitForElementVisible('xpath', '//img[contains(@alt, "texte alternatif par défaut")]'));
        // Test `aria-label` attribute appears on the preview wrapper.
   -    // cSpell:disable-next-line
   +    // cSpell:disable-next-line.
        $assert_session->elementExists('css', '[data-drupal-media-preview][aria-label="Tatou poilu hurlant"]');
        $this->click('.ck-widget.drupal-media');
        $this->assertVisibleBalloon('[aria-label="Drupal Media toolbar"]');
   @@ -843,11 +842,11 @@ public function testTranslationAlt() {
   
   @@ -843,11 +842,11 @@ public function testTranslationAlt() {
        $this->assertVisibleBalloon('.ck-media-alternative-text-form');
        // Assert that the default alt on the UI is the default alt text from the
        // media entity.
   -    // cSpell:disable-next-line
   +    // cSpell:disable-next-line.
        $assert_session->elementTextEquals('css', '.ck-media-alternative-text-form__default-alt-text-value', 'texte alternatif par défaut');
    
        // Fill in the alt field in the balloon form.
   -    // cSpell:disable-next-line
   +    // cSpell:disable-next-line.
   

   Please remove these changes; they are out of scope and also as far as I know will break the cspell integration, which does not indicate a period should be used: https://cspell.org/configuration/document-settings/

2. +++ b/core/modules/ckeditor5/tests/src/FunctionalJavascript/MediaTest.php
   @@ -1005,6 +1004,9 @@ public function testLinkability(bool $unrestricted) {
   +  /**
   +   *
   +   */
   
   +++ b/core/modules/comment/tests/src/Kernel/Views/CommentViewsKernelTestBase.php
   @@ -39,6 +39,9 @@ abstract class CommentViewsKernelTestBase extends ViewsKernelTestBase {
   +  /**
   +   *
   +   */
   
   +++ b/core/modules/toolbar/tests/src/Functional/ToolbarAdminMenuTest.php
   @@ -72,6 +72,9 @@ class ToolbarAdminMenuTest extends BrowserTestBase {
   +  /**
   +   *
   +   */
   
   +++ b/core/modules/user/tests/src/Functional/Views/HandlerFieldRoleTest.php
   @@ -25,6 +25,9 @@ class HandlerFieldRoleTest extends UserTestBase {
   +  /**
   +   *
   +   */
   
   +++ b/core/modules/user/tests/src/Kernel/Views/UserKernelTestBase.php
   @@ -38,6 +38,9 @@ abstract class UserKernelTestBase extends ViewsKernelTestBase {
   +  /**
   +   *
   +   */
   

   These are out-of-scope changes and should be removed.

The patch also does not apply (but it is more likely to apply and will be easier to reroll if the unneeded hunks are removed).

Thanks!

Done for both addRole and removeRole.

Needs work for the above test failures. Thanks!

Changed the code to chain usages of User::addRole() and ::removeRole(). A patch created for these changes.

Modified the code to chain usages of User::addRole() and ::removeRole().

Modified the code to chain usages of User::addRole() and ::removeRole(). Addressed the custom command fail as well.

Created a new patch because in the previous patch added by @nishta.pradhan, path of files was starting with /web/core instead /core patch was not applying because of this.

@nishtha.pradhan, why did you create a new patch instead of using the merge request? Also, when you update an existing issue with a patch, please supply an interdiff. Please, read the issue and understand its current status before you add changes.

Why, for that matter, is anyone creating patches? All the latest patches still do not have my feedback from #9 addressed. No one should supply a new patch that does not address that issue.

I am removing credit for all the unnecessary patches, and hiding them. If a MR is already used on an issue, please don't start suddenly creating patches without explanation; this adds noise and confusion. You can still get credit for this issue by implementing my feedback from #9.

I've addressed #9 and made a separate MR based on 10.1.x (previous one has to be rebased).

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

I feel bad that I made that commit accidentally and confused the review bot. How can we make the MR by @pminf the one the bot reacts to?

@rogerpfaff I reverted your commit from #25 and set the issue back to its previous state. Tests will pass and everything will be OK again :)

I made a rebase.

Before reviewing there appear to be some merge conflicts

error: patch failed: core/modules/block_content/tests/src/Kernel/BlockContentAccessHandlerTest.php:116
error: core/modules/block_content/tests/src/Kernel/BlockContentAccessHandlerTest.php: patch does not apply
error: patch failed: core/modules/user/tests/src/Kernel/UserEntityReferenceTest.php:67
error: core/modules/user/tests/src/Kernel/UserEntityReferenceTest.php: patch does not apply

Hi, I applied the patch after rebasing the branch, it seems look good, it is getting applied cleanly without any conflict, Attaching screenshot, Other than that patch look fine, there is no other User role method seen got unchained.

I'm going to claim this issue so that I can use it for the DrupalCon Portland core mentoring day.

-mike

Work/ed/ing on this issue at First Time Contribution DrupalCon Portland 2024.

We spent 1200 to 1500 May 8 2024 porting the merge request from Drupal 10 to Drupal 11.

• Zoyace Shrestha
• chadhester
• alexb7217
• ultimike

I reviewed the code and the updated code is all equivalent (but chained) with the exception of adding willReturn($this->account) statements in the RemoveRoleUserTest. Since the tests continue to pass, these don't appear to be causing issues.

Worked on the issue with ultimike at DrupalCon Portland 2024.

Worked on this with three mentees during DrupalCon Portland 2024 first time contributor workshop: @Zoyace Shrestha, @chadhester,
@alexb7217

-mike

Saving DrupalCon contributor credits from #45.

And hiding screenshot. (We only need screenshots when there is a user-facing change to evaluate.) Thanks!

Committed live! at DrupalCon Portland 2024 to 11.x, 11.0.x, 10.4.x, and 10.3.x. It did not cherry-pick cleanly to 10.2.x. Setting PTBP for a 10.2.x MR.

Thanks everyone!

10.2.x has had its final bugfix release, so marking fixed against 10.3.x (which means those credits from Portland will finally show on your profiles.) 😅 Yay!

w00t. Thank you.

Amending attribution.