Support from Acquia helps fund testing for Drupal Acquia logo

Comments

InternetDevels’s picture

Status: Active » Needs review
FileSize
27.54 KB

Here is the patch. It still has some unresolved issues, but lets see what testbot says.

Status: Needs review » Needs work

The last submitted patch, drupal-user_replace_user_access-2062039.patch, failed testing.

InternetDevels’s picture

Status: Needs work » Needs review
FileSize
29.71 KB

corrected

Status: Needs review » Needs work
Issue tags: -CodeSprintCIS

The last submitted patch, drupal-user_replace_user_access-2062039-3.patch, failed testing.

Xano’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
22.78 KB

Re-roll.

andypost’s picture

InternetDevels’s picture

Status: Needs work » Needs review
FileSize
10.04 KB
18.16 KB

Status: Needs review » Needs work

The last submitted patch, 7: drupal-user_replace_user_access-20620397.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review

7: drupal-user_replace_user_access-20620397.patch queued for re-testing.

EDIT HEAD was broken

xjm’s picture

Priority: Minor » Normal
andypost’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs reroll
+++ b/core/modules/user/lib/Drupal/user/Tests/UserPermissionsTest.php
@@ -34,7 +34,7 @@ function setUp() {
-   * Change user permissions and check user_access().
+   * Change user permissions and check permissions.

it was ugly before, no idea how to improve

herom’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/user/lib/Drupal/user/RegisterFormController.php
    @@ -18,11 +18,9 @@ class RegisterFormController extends AccountFormController {
    -    global $user;
    +    $user = \Drupal::currentUser();
    

    shouldn't we use $this->currentUser() ?

  2. +++ b/core/modules/user/lib/Drupal/user/Tests/UserPermissionsTest.php
    @@ -34,7 +34,7 @@ function setUp() {
    -   * Change user permissions and check user_access().
    +   * Change user permissions and check permissions.
    

    "Test changing user permissions through the (UI|permissions page).", maybe?

andypost’s picture

both nitpicks makes sense

h3rj4n’s picture

Status: Needs work » Needs review
Issue tags: -CodeSprintCIS
FileSize
1.33 KB
18.06 KB

As suggested in comment #12 changed the two things.

herom’s picture

Status: Needs review » Needs work
+++ b/core/modules/user/lib/Drupal/user/AccountFormController.php
@@ -252,6 +252,7 @@ public function form(array $form, array &$form_state) {
+      '#access' => $show_admin_language && $account->hasPermission('access administration pages'),

this line isn't necessary after last reroll. please remove.

Xano’s picture

Status: Needs work » Needs review
FileSize
970 bytes
17.84 KB
herom’s picture

Status: Needs review » Reviewed & tested by the community
webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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