Problem/Motivation

Since hasPermission() method uses isAdmin() method, there's no need to use both of this methods in a same check with OR condition.

Proposed resolution

Use hasPermission() only.

Remaining tasks

review/commit

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#23 Small-code-clean-for-RoleStorage-2563995-23.patch807 bytesrakhi soni
#2 drupal8-user-rolestorage-optimize-2563995-1.patch800 bytestemoor

Comments

Temoor created an issue. See original summary.

temoor’s picture

temoor commented
StatusFileSize
new drupal8-user-rolestorage-optimize-2563995-1.patch800 bytes
temoor’s picture

temoor commented
Status: Active » Needs review
andypost’s picture

andypost
he/him
Primary language Russian
commented
Issue tags: -user +Entity Field API, +Quick fix

Status: Needs review » Needs work

The last submitted patch, 2: drupal8-user-rolestorage-optimize-2563995-1.patch, failed testing.

Status: Needs work » Needs review

andypost queued 2: drupal8-user-rolestorage-optimize-2563995-1.patch for re-testing.

dawehner’s picture

dawehner
Primary language German
commented
Category: Support request » Task
Issue tags: +Needs beta evaluation

Its not a bad idea to get less function calls there.

m1r1k’s picture

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

Then let go! :)

alexpott’s picture

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

@dawehner well we actually might have extra function calls. Still need a beta evaluation and I don't really see how this change is necessary tbh. We should probably do this in 8.1

andypost’s picture

andypost
he/him
Primary language Russian
commented

"removing unused and untested functionality" makes this change prioritized but require unit-test that will show that method called twice
also it could have a performance impact because makes early exit in function that used a lot

daffie’s picture

daffie commented
Issue tags: +Needs tests, +Performance

See comment #10.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.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.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.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.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.

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.

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

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

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should 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.

rakhi soni’s picture

rakhi soni commented
StatusFileSize
new Small-code-clean-for-RoleStorage-2563995-23.patch807 bytes

Kindly review patch,,

rakhi soni’s picture

rakhi soni commented
Status: Needs work » Needs review
andypost’s picture

andypost
he/him
Primary language Russian
commented
Title: Small code clean for RoleStorage » Small code clean for RoleStorage::isPermissionInRoles()
Issue summary: View changes
Issue tags: -Needs beta evaluation, -Needs tests

I see no reason in tests as the function used mostly everywhere in core

andypost’s picture

andypost
he/him
Primary language Russian
commented
Title: Small code clean for RoleStorage::isPermissionInRoles() » Remove duplicated check for isAdmin() in RoleStorage::isPermissionInRoles()

Clearer title

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs review » Reviewed & tested by the community

I think it good to go as is

  • catch committed 660192c on 10.0.x 
    Issue #2563995 by Temoor, Rakhi Soni, andypost: Remove duplicated check...
  • catch committed 4ae63c5 on 10.1.x
    Issue #2563995 by Temoor, Rakhi Soni, andypost: Remove duplicated check...

  • catch committed d12f31e on 9.5.x 
    Issue #2563995 by Temoor, Rakhi Soni, andypost: Remove duplicated check...
catch’s picture

catch
he/him
Primary language English
commented
Version: 9.4.x-dev » 9.5.x-dev
Status: Reviewed & tested by the community » Fixed

Agreed this doesn't need explicit test coverage, it's functionally identical just with less function calls.

Committed/pushed to 10.1.x and cherry-picked to 10.0.x and 9.5.x, thanks!

Status: Fixed » Closed (fixed)

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