Deprecate user_roles() and user_role_names()

There's no need to postpone this.

Uploaded a patch for the issue mentioned, please review.
Thank You.

needs to replace user_role_names() too and deprecate both functions https://www.drupal.org/about/core/policies/core-change-policies/drupal-d... → and add test

Updated IS

Created MR - replaced all usage and deprecated

Needs reviews and polishing

PHPStan reports false positive - it has no idea that roles has string IDs

Running PHPStan on *all* files.
 ------ --------------------------------------------------- 
  Line   core/modules/user/src/Form/RoleSettingsForm.php    
 ------ --------------------------------------------------- 
  62     Cannot unset offset 'anonymous' on array<int,      
         Drupal\user\Entity\Role>.                          
  63     Cannot unset offset 'authenticated' on array<int,  
         Drupal\user\Entity\Role>.                          
 ------ --------------------------------------------------- 

 ------ ----------------------------------------------------- 
  Line   core/modules/user/src/Plugin/views/filter/Roles.php  
 ------ ----------------------------------------------------- 
  57     Cannot unset offset 'anonymous' on array<int,        
         Drupal\user\Entity\Role>.                            
  58     Cannot unset offset 'authenticated' on array<int,    
         Drupal\user\Entity\Role>.                            
 ------ ----------------------------------------------------- 

 [ERROR] Found 4 errors   

-    $roles = user_role_names(TRUE);
-    $roles = $this->roleStorage->loadMultiple();
+    $roles = Role::loadMultiple();
...
     $admin_roles = $this->roleStorage->getQuery()

Replaced roleStorage with static loadMultiple() solved fbut it needs follow-up

Filed issue to fix typing https://github.com/mglaman/phpstan-drupal/issues/522

Re #9 add to the baseline rather than make incorrect changes for PHPStan. PHPStan should not decide correctness when it is wrong.

Per #11

Updated baseline and reverted to injected storage

Meantime update of phpstan's baseline removed one false-positive

diff --git a/core/phpstan-baseline.neon b/core/phpstan-baseline.neon
index 4b0b775af4..ae976aebef 100644
--- a/core/phpstan-baseline.neon
+++ b/core/phpstan-baseline.neon
@@ -880,11 +880,6 @@ parameters:
                        count: 3
                        path: modules/block_content/tests/src/Kernel/Migrate/d6/MigrateCustomBlockContentTranslationTest.php
 
-               -
-                       message: "#^Variable \\$book_links in empty\\(\\) is never defined\\.$#"
-                       count: 1
-                       path: modules/book/book.module
-
                -
                        message: "#^Variable \\$callable in empty\\(\\) always exists and is not falsy\\.$#"
                        count: 1

I did not commit guessing other issue

If we can update the IS and CR similar to 📌 Deprecate user_role_permissions() Fixed please

Unrelated failure, updated IS and CR

There's 3 threads are open in MR and it needs addressing, maybe in follow-up

Basically some places using to call HTML::escape() before using list of names for checkboxes but never for radios form elements. Looking at code I think we can remove escaping as element doing that at render time.

$roles = array_map(['\Drupal\Component\Utility\Html', 'escape'], $roles);

I have uploaded the patch.
Address #17 comment

@Anchal_gupta this issue is being worked on in the MR not patches. Hiding patch 18 and updating credit.

Actually NW per #17

There's 3 threads are open in MR and it needs addressing, maybe in follow-up

Removed escaping with extra commit and rebased

Also filed follow-up to remove 2 extra escaping 📌 Stop calling Html::escape() for checkbox options Needs work

Applied MR and did searches for user_role_names() and user_role() and all instances, minus trigger_error section, have been addressed.

IS was recently updated so think that's fine.

For the comment I just made on the MR.

Just trying to move this forward, updated the code and fixed #23, moving back to NR

quick look into the build I don't immediately see the why but seems there were some failure in the last commit. Needs investigating.

rebased, squashed 2 commits from murilohp and fixed it by filtering roles by permission first and then returning labels

Hi sorry @andypost I didn't see your comment before, I just did the rebase, I reverted the commit "initial-default" because it pushed non-related changes.

@elber you can just use git rebase -i HEAD~2 and drop useless commits as they will confuse reviewers

Removed this commits and rebased again (the failed test CKEditor5AllowedTagsTest is known as failing randomly)

Point #23 appears to have been addressed

Posted another round of review on the MR

Thank you! fixed both points and reverted removal of Html::escape() for 📌 Stop calling Html::escape() for checkbox options Needs work

So there is a build failure, I believe because 📌 Stop calling Html::escape() for checkbox options Needs work needs to land first. So temporarily postponing this until that lands.

No, it was typo - closing ) in wrong place, I'm sure the 📌 Stop calling Html::escape() for checkbox options Needs work should go second at beta stage

Gotcha. In that case lets move it forward.

Left a couple of comments on the MR.

Unfortunately the deprecation message needs a bit of work as Use to inline implementation instead. doesn't make much sense. I think it should be something like:

@trigger_error(__FUNCTION__ . '() is deprecated in drupal:10.1.0 and is removed from drupal:11.0.0. Use \Drupal\user\Entity\Role::loadMultiple() and, if necessary, an inline implementation instead. See https://www.drupal.org/node/3349759', E_USER_DEPRECATED);

Note this needs fixing for both user_role_names() and user_roles().

@catch there's a follow-up for the escaping inconsistency - see 📌 Stop calling Html::escape() for checkbox options Needs work

@alexpott thanks, change message to suggested, and rebased

@catch user_role_names() doing this array_map() but having wrapper function for single function call sounds bad idea. Moreover, places where we need labels from array of entities are not a lot but could be a global helper (follow-up?)

Lets see if we can get this in for 10.1

Seems #39 has been addressed.

And #40

places where we need labels from array of entities are not a lot but could be a global helper (follow-up?)

Think that should be a follow up as seems out of scope for this particular issue.

I can open that up if we all agree.

Needs a re-roll for phpstan-baseline.neon (and also the deprecation version although that's also easy to fix on commit).

Rebased and updated deprecation message

Reroll looks good!

OK let's go ahead without the helper method and rely on array_map(). Guessing we have quite a lot of usages in core compared to contrib. Also thanks for pointing me to the existing checkboxes issue.

Committed/pushed to 11.x, thanks!

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

andypost → closed merge request !3701