Actions created in user_user_role_insert contain a '.' and cannot be updated via the UI

Perhaps the simplest solution is to simply allow the periods. Patch attached.

Adding updated error text to reflect that with this patch periods are allowed too.

Allowing periods makes sense to me. Should we add a test here to avoid future regressions?

Writing test

Test only and patch with code from #4 attached

Will this also need an upgrade path if the action was already saved with the .?

The user module seems to use a period to "namespace" its actions to keep people from creating an action with the same machine name. This is because it will delete any associated actions when a user role is deleted.

If you happened to be unfortunate enough to:

• Create an action called user_add_role_action.myrole
• Create a role called my_role (no action will be created)
• Delete the role called my_role

Your custom action 'user_add_role_action.myrole' would be deleted by the user module even though it was never created by that module! I therefore suggest we do not allow the use of periods in the machine name, but instead disable the machine name field when editing so it doesn't need to run through validation.

As per #12,
machine name doesn't need to run through validation, if at all this not created newly.

re-rolled #13 patch.

The element should be disabled, not just its validation. Right now, you allow anything to be entered into the machine name field.

Consider using the #disabled, #value and #element_validate key.

machine name will be disabled if it is not new.
'#disabled' => !$this->entity->isNew(),
and also will not be validated if machine name is not new.
'#element_validate' => !$this->entity->isNew() ?: [],

From the FAPI 8 reference:

Values: An array of function names to be called to validate this element (and/or its children).

So '#element_validate' => !$this->entity->isNew() ?: [], won't work. It assigns TRUE to the property when the entity isn't new and an empty array when it is. You should leave the property alone when the entity is new and specify an empty array when it isn't.

leaving the property alone when the entity is new and specify an empty array when it isn't.

Looks good, but this is the kind of "weird" code that gets tossed out in another issue unless you document why it was added in the first place. Attached is a patch with documentation added.

Hmm, I think we should always validate the machinename, even if its disabled. So #12 is prolly not an option, as disabled form items are also submitted (and therefore can be altered by javascript etc.)

Feel free to correct me if I'm wrong ;)

From FormBuilder::handleInputElement()

   // Setting #disabled to TRUE results in user input being ignored regardless
    // of how the element is themed or whether JavaScript is used to change the
    // control's attributes. However, it's good UI to let the user know that
    // input is not wanted for the control. HTML supports two attributes for:
    // this: http://www.w3.org/TR/html401/interact/forms.html#h-17.12. If a form
    // wants to start a control off with one of these attributes for UI
    // purposes, only, but still allow input to be processed if it's submitted,
    // it can set the desired attribute in #attributes directly rather than
    // using #disabled. However, developers should think carefully about the
    // accessibility implications of doing so: if the form expects input to be
    // enterable under some condition triggered by JavaScript, how would someone
    // who has JavaScript disabled trigger that condition? Instead, developers
    // should consider whether a multi-step form would be more appropriate
    // (#disabled can be changed from step to step). If one still decides to use
    // JavaScript to affect when a control is enabled, then it is best for
    // accessibility for the control to be enabled in the HTML, and disabled by
    // JavaScript on document ready.
    if (!empty($element['#disabled'])) {
      if (!empty($element['#allow_focus'])) {
        $element['#attributes']['readonly'] = 'readonly';
      }
      else {
        $element['#attributes']['disabled'] = 'disabled';
      }
    }

    // With JavaScript or other easy hacking, input can be submitted even for
    // elements with #access=FALSE or #disabled=TRUE. For security, these must
    // not be processed. Forms that set #disabled=TRUE on an element do not
    // expect input for the element, and even forms submitted with
    // self::submitForm() must not be able to get around this. Forms that set
    // #access=FALSE on an element usually allow access for some users, so forms
    // submitted with self::submitForm() may bypass access restriction and be
    // treated as high-privilege users instead.
    $process_input = empty($element['#disabled']) && (($form_state->isProgrammed() && $form_state->isBypassingProgrammedAccessChecks()) || ($form_state->isProcessingInput() && (!isset($element['#access']) || $element['#access'])));

Playing the devil's advocate here, but if core created these actions and core will delete them when the user roles are deleted: What good will editing them do? They may end up being deleted automatically because it would make sense to do so if they weren't modified. However, if you did modify them, an automatic deletion may really hurt your site.

Aside from the patch fixing the issue at hand, there's other approaches to this:

1. Do not edit the actions created by core. This seems unacceptable because end users can't guess which ones can and can't be edited.
2. Mark some actions as "fixed", i.e. not editable. This fixes the original report and the issue raised in approach 1. If you'd want an action that adds or removes mutliple roles, you could always create your own action.

That being said, implementing a system to mark action entities as fixed would require a lot more code than what the current patch contains.

Oh, thanks for that!

In that case, I can live with the change suggested. Its not the most pretty (core generating machinenames with dots, but not allowing them to be created), but neither is allowing dots and having certain actions (in a rare case) automagicly deleted.

Another option might be removing the dots in the generated "add x role to selected user" machinename, and delete those specific generated actions on role_deletion by using a custom event/hook and query. That way we keep machinenaming consistency and do not have a special case for Actions.

#2605042: Cannot edit actions created by user_user_role_insert()