Problem/Motivation

The permissions are not always in the correct order because the PermissionHandler sorts on title which can contain markup if you've used t() with placeholders.

Proposed resolution

strip tags from the title before sorting.

Before:
Before sort

After
After sort

CommentFileSizeAuthor
#27 2804947-27.patch5.44 KBimmaculatexavier
#26 2804947-169.patch5.44 KBimmaculatexavier
#23 2804947-23.patch4.57 KBmarcvangend
#16 interdiff-14_16.txt3.79 KBanmolgoyal74
#16 2804947-16.patch4.82 KBanmolgoyal74
#14 2804947-14.patch3.32 KBanmolgoyal74
#7 2804947-7-complete.patch3.22 KBLoMo
#7 2804947-7-test-only.patch2.29 KBLoMo
#3 after-sort-2.png72.65 KBbenjy
#3 2804947-3.patch960 bytesbenjy
#2 2793445-2.patch2.56 KBbenjy
after-sort.png93.37 KBbenjy
before-sort.png142.79 KBbenjy

Issue fork drupal-2804947

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
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

benjy created an issue. See original summary.

benjy’s picture

benjy CreditAttribution: benjy at PreviousNext commented
Issue summary: View changes
Status: Active » Needs review
FileSize
2793445-2.patch2.56 KB

Heres a patch this improves on things but it still doesn't seem quite right as Administer Entity Print is at the bottom of my permission list rather than at the top.

benjy’s picture

benjy CreditAttribution: benjy at PreviousNext commented
Issue summary: View changes
FileSize
2804947-3.patch960 bytes
after-sort-2.png72.65 KB

Fixed this for my use case, the key part was the strip_tags() as I was using %placeholders in my permission titles and the sort was based on the markup :)

The last submitted patch, 2: 2793445-2.patch, failed testing.

cilefen’s picture

cilefen CreditAttribution: cilefen commented
Issue tags: +Needs tests

It should be easy to create a test for this.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

LoMo’s picture

LoMo CreditAttribution: LoMo as a volunteer commented
Issue tags: -Needs tests
FileSize
2804947-7-test-only.patch2.29 KB
2804947-7-complete.patch3.22 KB

I would be marking this RTBC, since manual testing shows that the patch works, but since I'm also adding to the patch with the requested tests, I guess the status needs to stay on "Needs review". But I think this is now committable. We should see the "test-only" patch fail and the "complete" patch should pass.

The last submitted patch, 7: 2804947-7-test-only.patch, failed testing.

LoMo’s picture

LoMo CreditAttribution: LoMo as a volunteer commented

Re #8: Good. That is what we want to see. The new test, on its own, fails, but passes with the bugfix patch. :-)

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

joachim’s picture

joachim CreditAttribution: joachim as a volunteer commented
Status: Needs review » Needs work

Couple of nitpicks, but apart from that looks good.

  1. +++ b/core/modules/user/src/PermissionHandler.php
@@ -205,12 +205,13 @@ protected function sortPermissions(array $all_permissions = []) {
+        return strnatcasecmp(strip_tags($permission_a['title']), strip_tags($permission_b['title']));

    A comment here to explain why strip_tags() is used would be helpful. 

  2. +++ b/core/modules/user/src/Tests/UserPermissionsTest.php
@@ -90,6 +99,31 @@ public function testUserPermissionChanges() {
+   * @see https://www.drupal.org/node/2804947

    I don't think we need to reference this issue -- the comment suffices. (And there is always git blame!)

    Also, first paragraph of the docblock needs to be a single line.

anmolgoyal74’s picture

anmolgoyal74 CreditAttribution: anmolgoyal74 at OpenSense Labs for DrupalFit commented
Status: Needs work » Needs review
FileSize
2804947-14.patch3.32 KB

The patch includes the changes mentioned in #13.

Status: Needs review » Needs work

The last submitted patch, 14: 2804947-14.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

anmolgoyal74’s picture

anmolgoyal74 CreditAttribution: anmolgoyal74 at OpenSense Labs for DrupalFit commented
Status: Needs work » Needs review
FileSize
2804947-16.patch4.82 KB
interdiff-14_16.txt3.79 KB

Updated the patch as per the coding standard.

Status: Needs review » Needs work

The last submitted patch, 16: 2804947-16.patch, failed testing. View results

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.2.x-dev

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.2.x-dev » 9.3.x-dev
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Issue tags: +Bug Smash Initiative, +Needs reroll
marcvangend’s picture

marcvangend
he/him
Primary language Dutch
Location Amsterdam
CreditAttribution: marcvangend as a volunteer and at LimoenGroen commented
FileSize
2804947-23.patch4.57 KB

This is a re-roll of #16, not addressing the test failure yet.

marcvangend’s picture

marcvangend
he/him
Primary language Dutch
Location Amsterdam
CreditAttribution: marcvangend as a volunteer and at LimoenGroen commented
Version: 9.3.x-dev » 9.4.x-dev
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 23: 2804947-23.patch, failed testing. View results

immaculatexavier’s picture

immaculatexavier CreditAttribution: immaculatexavier as a volunteer and at Srijan | A Material+ Company for Drupal India Association commented
Status: Needs work » Needs review
FileSize
2804947-169.patch5.44 KB

Rerolled patch against 9.4.x

immaculatexavier’s picture

immaculatexavier CreditAttribution: immaculatexavier as a volunteer and at Srijan | A Material+ Company for Drupal India Association commented
FileSize
2804947-27.patch5.44 KB

Rerolled patch against 9.4.x

Status: Needs review » Needs work

The last submitted patch, 27: 2804947-27.patch, failed testing. View results

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

ankithashetty’s picture

ankithashetty
Location Karnataka, India
CreditAttribution: ankithashetty as a volunteer and at Specbee commented
Issue tags: -Needs reroll

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.