Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

Issue tags: +FormInterface

Tagging.

ayelet_Cr’s picture

Assigned: Unassigned » ayelet_Cr
ayelet_Cr’s picture

Status: Active » Needs review
FileSize
9.5 KB

Status: Needs review » Needs work

The last submitted patch, 1938888-convert_user_admin_permissions.patch, failed testing.

ayelet_Cr’s picture

Status: Needs work » Needs review
FileSize
9.83 KB

Status: Needs review » Needs work

The last submitted patch, 1938888-convert_user_admin_permissions.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
Crell’s picture

Status: Needs review » Needs work
Issue tags: +WSCCI, +FormInterface, +SprintWeekend2013, +WSCCI-conversion

The last submitted patch, 1938888-convert_user_admin_permissions.patch, failed testing.

ayelet_Cr’s picture

ayelet_Cr’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1938888-convert_user_admin_permissions.patch, failed testing.

Crell’s picture

Status: Needs work » Postponed
tim.plunkett’s picture

FileSize
10.36 KB

Rerolled, revisiting the blocker.

tim.plunkett’s picture

Status: Postponed » Needs review
FileSize
11.35 KB
10.61 KB

Okay I'm just merging that issue into this. We have a wonderful example sitting right here.

Status: Needs review » Needs work
Issue tags: -WSCCI, -FormInterface, -SprintWeekend2013, -WSCCI-conversion

The last submitted patch, user-1938888-15-PASS.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review

#15: user-1938888-15-PASS.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +WSCCI, +FormInterface, +SprintWeekend2013, +WSCCI-conversion

The last submitted patch, user-1938888-15-PASS.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
10.99 KB

Rerolled.

Status: Needs review » Needs work

The last submitted patch, user-1938888-19.patch, failed testing.

tim.plunkett’s picture

Assigned: ayelet_Cr » tim.plunkett

Sorry @ayelet_Cr.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
724 bytes
10.99 KB

Bad copypaste from the reroll after user roles were converted.

Status: Needs review » Needs work

The last submitted patch, user-1938888-22.patch, failed testing.

dawehner’s picture

+++ b/core/modules/user/lib/Drupal/user/Form/UserPermissionsForm.phpundefined
@@ -0,0 +1,158 @@
+class UserPermissionsForm implements FormInterface, ControllerInterface {
...
+   * Constructs a new UserAdminPermissionForm.

The classname and the documentation seem to disagree. I would vote for the classname.

+++ b/core/modules/user/lib/Drupal/user/Form/UserPermissionsForm.phpundefined
@@ -0,0 +1,158 @@
+    $module_info = system_get_info('module');

If you just want to get the module info maybe accessing the info in the module handler would be better.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
1.68 KB
11 KB
640 bytes
10.99 KB

Since we need the module's human readable names, we can't use the module handler.
We can get closer by doing this (see patch 2).
If you think that's worse, I've included patch 1, which is just the UserPermissionsForm doc fix.

Status: Needs review » Needs work

The last submitted patch, user-1938888-25-2.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: +Stalking Crell
FileSize
12.79 KB
1.79 KB

Turns out drupal_valid_path() doesn't handle routes correctly, at all.

Status: Needs review » Needs work

The last submitted patch, user-1938888-27.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
574 bytes
12.8 KB

Ah, needed to assign it to ['access']

dawehner’s picture

+++ b/core/modules/user/lib/Drupal/user/Form/UserPermissionsForm.phpundefined
@@ -0,0 +1,158 @@
+  public function buildForm(array $form, array &$form_state, $role_name = NULL) {

Should we just use $role_id for now, as there is not even a name property on roles these days.

Crell’s picture

+++ b/core/includes/menu.inc
@@ -2788,11 +2788,11 @@ function menu_router_build($save = FALSE) {
 function _menu_router_translate_route($route_name) {
-  $route = Drupal::service('router.route_provider')->getRouteByName($route_name);
-  $path = trim($route->getPattern(), '/');
-
-  // Translate placeholders, e.g. {foo} -> %.
-  return preg_replace('/{' . DRUPAL_PHP_FUNCTION_PATTERN . '}/', '%', $path);
+  $outline = Drupal::service('router.route_provider')
+    ->getRouteByName($route_name)
+    ->compile()
+    ->getPatternOutline();
+  return trim($outline, '/');
 }

I'm surprised this passes; the pattern outline is not quite the same as the pattern. It also drops optional ending placeholders, I think. (Or it did. May have changed?) I don't know if this will have any knock-on effects.

+++ b/core/modules/user/lib/Drupal/user/Form/UserPermissionsForm.php
@@ -0,0 +1,158 @@
+    $role_names = user_role_names();

This doesn't have a service yet? :-(

tim.plunkett’s picture

FileSize
5.87 KB
13.9 KB

Let's just do this. It's way cleaner looking anyway.

Note, we can't convert the local task until its parent is a route.

dawehner’s picture

+++ b/core/modules/user/lib/Drupal/user/Form/UserPermissionsSpecificForm.phpundefined
@@ -0,0 +1,37 @@
+    return $this->roleStorage->loadMultiple(array($this->roleId));

This could just use a single load()

tim.plunkett’s picture

FileSize
13.88 KB

Ahh I refactored that a bunch of times, originally the other form was just passing NULL and other weird stuff, and you're totally right :)

tim.plunkett’s picture

FileSize
644 bytes
13.91 KB

Nope, we need to return an array. I almost prefer #32 more...
Interdiff against #32

tim.plunkett’s picture

Status: Needs review » Needs work
Issue tags: +WSCCI, +Stalking Crell, +FormInterface, +SprintWeekend2013, +WSCCI-conversion

The last submitted patch, user-1938888-35.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
500 bytes
13.91 KB

/facepalm

dawehner’s picture

+++ b/core/modules/user/lib/Drupal/user/Form/UserPermissionsSpecificForm.phpundefined
@@ -0,0 +1,37 @@
+class UserPermissionsSpecificForm extends UserPermissionsForm {

This class deals with roles, so maybe a better classname like UserPermissionsRoleSpecificForm

+++ b/core/modules/user/lib/Drupal/user/Form/UserPermissionsSpecificForm.phpundefined
@@ -0,0 +1,37 @@
+  /**
+   * {@inheritdoc}
...
+  public function buildForm(array $form, array &$form_state, $role_id = NULL) {

Needs docs for the additional parameter

tim.plunkett’s picture

FileSize
1.76 KB
14.01 KB

Thanks @dawehner!

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Thank you

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Needs a reroll...

git ac https://drupal.org/files/user-1938888-40.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 14343  100 14343    0     0  10695      0  0:00:01  0:00:01 --:--:-- 12483
error: patch failed: core/modules/user/user.admin.inc:97
error: core/modules/user/user.admin.inc: patch does not apply
amateescu’s picture

Status: Needs work » Needs review
FileSize
15.27 KB

Rerolled.

Status: Needs review » Needs work

The last submitted patch, user-1938888-43.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll, -Stalking Crell
FileSize
730 bytes
15.27 KB
amateescu’s picture

Status: Needs review » Reviewed & tested by the community

Ups, sorry for missing that.

tim.plunkett’s picture

tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned
Issue tags: +FormInterface, +WSCCI-conversion
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Found #2049681: User permissions table header is no longer sticky whilst doing manual testing.

Committed d3d54f1 and pushed to 8.x. Thanks!

I'm ignoring the injection of string translation until #2018411: Figure out a nice DX when working with injected translation lands and will continue this policy until that's resolved. We are going to have to second passes of all conversions due to drupal_set_message() and watchdog() anyway.

tim.plunkett’s picture

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