Configured actions created by User module cannot be edited

+1 for the second proposed fix, there wouldn't be a need for a BC layer with that one so would be simpler to implement.

Another possible fix would be not to validate the machine name when it's set to be disabled. That would be in the machine name Element class. There's no point warning the user that their entered machine name is not valid if they cannot edit that machine name!

I wonder if it makes sense to make that change on the machine_name Element itself and so fix this problem in any other places it might be lurking, because I'd be surprised if Actions was the only place this problem existed.

Hi,

I have installed the same drupal version and followed the same steps. But in my case, the machine name is not editable. Did I miss anything?

I have installed the same drupal version and followed the same steps. But in my case, the machine name is not editable. Did I miss anything?

Notice in the screenshot in the issue summary it is highlighting that the machine name has an error, because when the user module creates the actions it adds a "." (period) before the role name. This is a perfectly valid machine name, but the problem is that the machine_name form element does not have a "." (period) in the allowed characters so it throws an error when you try to save the form. Yet it is not possible for the user to change the machine name form field (for good reason), so essentially it becomes impossible for the user to make any changes to the effected actions.

I think that's main issues about editing, UX and safety(access) for action ui is what could wait for rename

The bug in dot used( ref https://www.drupal.org/node/2297311

Okay, so essentially, the currently discussed resolution is to do something to the effect of:

--- a/core/lib/Drupal/Core/Render/Element/MachineName.php
+++ b/core/lib/Drupal/Core/Render/Element/MachineName.php
@@ -235,6 +235,11 @@ public static function processMachineName(&$element, FormStateInterface $form_st
    * - Cannot be changed after creation (via #disabled).
    */
   public static function validateMachineName(&$element, FormStateInterface $form_state, &$complete_form) {
+    if (isset($element['#disable']) && $element['#disabled'] === TRUE) {
+      // Don't validate the machine name if the element is disabled, see https://www.drupal.org/node/3150316
+      return;
+    }
+
     // Verify that the machine name not only consists of replacement tokens.
     if (preg_match('@^' . $element['#machine_name']['replace'] . '+$@', $element['#value'])) {
       $form_state->setError($element, t('The machine-readable name must contain unique characters.'));

My concern is, does this approach introduce any unintended consequences, my gut says probably not, is there any security/integrity concerns here??

What if I am putting a check before disabling it? Something like this:

    if (!$this->entity->isNew()) {
      $disabled = TRUE;
      $getOriginalId = $this->entity->getOriginalId();
      if (preg_match('@[^a-z0-9_]+@', $getOriginalId)) {
        $disabled = FALSE;
      }
    }
    
    $form['id'] = [
      '#type' => 'machine_name',
      '#default_value' => $this->entity->id(),
      '#disabled' => $disabled,
      '#maxlength' => 64,
      '#description' => $this->t('A unique name for this action. It must only contain lowercase letters, numbers and underscores.'),
      '#machine_name' => [
        'exists' => [$this, 'exists'],
      ],
    ];

Again if it is not breaking any security/integrity here. Then I can create a patch for the same. I have checked it, It is working for me.

I have attached the patch for testing purposes.

Let me elaborate - as #9 said ID using "." (dot)

because when the user module creates the actions it adds a "." (period) before the role name

This leads to system.action.user_add_role_action.administrator.yml config key (which contains dot as #11 CR said)

Here's a starting fix - and it needs to fix existing config as well (that's why I added upgrade path)

I mean that machine name changes are not allowed in UI (mostly to prevent other config breakage - for example action used in views, so looping through all views looking for every plugin is overkill)

Another thing is generated machine names which should not use dot in ID because it breaks
- machine name element
- probably config listings

Re #18, this wouldn't address existing sites though and only puts a bandage over the actual issue here. And it seems like there are way too many unknowns and possible cases where the machine_name may be used (it is the ID of the config entity after all). Basically we could change this pattern for new sites, but it would be impossible to provide a reliable upgrade path for existing sites.

Fundamentally, I think #13 fixes the root of the problem, because if the MachineName Element is validated when it is not possible for the user to provider any input and the existing machine_name is not valid in the UI but is technically valid in code, then you end up in a situation where the user cannot edit the config entity in question. I say config entity because the actual issue here theoretically effects all config entities and is not restricted to just the actions created by the user module.

Regarding the CR in #11, I don't think that actually applies here, because it's specifically referring to using dots in YAML/array keys, not in the values of those keys, nor in the names of config files (which as we know pretty much always have dots in them). The machine_name value is only ever used as part of the config name and as a value in the config, never as an actual config key, which is what the CR is referring to.

To explain better just use drush

$ drush ev 'var_dump(get_class(\Drupal::config("system.action.user_add_role_action")));'
string(34) "Drupal\Core\Config\ImmutableConfig"

Nice point #3120869-5: Allow dot "." inside machine name

Another issue here is config schema which using dots as separators

I'm concerned that we may be going a little too far down one rabbit hole here, I'm concerned that this issue is just going to become a debate about whether Config Entity ID should have periods in them or not.

The problem that this issue needs to address is that the role actions created by the user module can't be edited because they have a period in them and that's not an allowed value for the MachineName Element. Now in theory this problem can extend to other Config Entities, because it is possible for a Config Entity ID to have a period in it, in fact any character for that matter, even spaces. What this means is that it is important to remember that the MachineName Element and the Config Entity ID are two completely separate things here, it is just by circumstance that the MachineName Element happens to be used by the majority of Config Entities in Core and Contrib for inputting the Config Entity ID. It is absolutely possible to use a regular text field as the Config Entity ID field, and by doing so the user would not experience this problem.

The important thing to remember: periods are perfectly legal characters for a Config Entity ID, they are not for the MachineName Element.

What this all means is that a solution here can't focus on what characters may or may not be used as the Config Entity ID, and so by extension what may or may not appear in the MachineName element, we have to assume that in theory it is possible for any possible combination of characters to appear in the MachineName Element, if (like in this case) that Config Entity was created through the API and not through the UI.

This means that, the only viable solution I can see is simply not to validate the MachineName Element if it is disable. I addressed other concerns around using other approaches in #21, specifically that if we think of this as a problem we can solve at the Config level, there is no viable backwords compatible solution, so we have to address this at the render level.

I think we should not allow edit or add machine names that has dots, it makes config schema validation hard

That's why I think it better to convert all "role bound actions" to locked (non-editable) and without dots in machine names
But it's fine in follow-up

That is not quite right.

The **default** behavior for the MachineName element is not to allow . as a legal machine name character.

However, any code that uses a MachineName element in a form is free to set the regular expression to a different value in order to allow . as a legal character. That would actually probably be the simplest solution to this -- a one-line addition to the editing form in the Actions module. This is already in the issue summary.

Ah yes you are right. Technically yes doing that would be the simplest solution, I'm just thinking that if instead we just don't validate the machine name when the element is disable, it fixes the problem for any and all similar edge cases. While, since the value can't be edited anyway because the element is disabled, seems like a solid long-term solution here.

I think we should not allow edit or add machine names that has dots, it makes config schema validation hard

That's why I think it better to convert all "role bound actions" to locked (non-editable) and without dots in machine names
But it's fine in follow-up

I'm not sure I understanding why it makes config schema validation any harder? Almost every schema I know has at least one or even more dots "." in the schema name, pretty much every Config Entity in Core does.

If we took that approach it would only solve the problem though for new sites, existing sites would still experience the issue, since as we established earlier in the issue it would be practically impossible to provide a reliable upgrade path for existing sites, since we have no way of guaranteeing exactly where the Config Entity IDs for existing actions are referenced.

I've gone through this issue and it seems the consensus to move this ticket forward it to allow the dots in the machine name as that feels safest for BC.

I've done this at the the base form of ActionFormBase as reocommended, but I wonder if it would be better inside a form_alter in the user module so it only applies there?

I've also made it so dots are no longer created with new role based actions so this shouldn't be in issue moving forward. The new pattern is user_[add,remove]_role_$role_action. This keeps consistency as all action except these ones end with _action

As actions getting new names we need to rename old actions

Went through and renamed the rest of the instances of the old actions format. Tests passing locally. Lets see here.

The patch looks good, but do we need an update path now to rename any pre-existing items with invalid IDs?

I made the patch so it wouldn't break existing sites while still leaving the config as is.

If we want to make an upgrade path I can take that stuff out and add a hook_update to update the action config on existsing sites for all roles

Sorry what I meant to say was existing sites are still in the same boat as the original bug report, they can't edit those actions

Hmmm looking at the MR, it now does both solutions? It allows a dot but also takes existing dots out of the machine names? Seems excessive.

If we just allow the dot here, we can just leave the rest as is right? Seems like the path of least resistance. The dot is obviously not causing problems or none of the existing plugins would be working, right?

Yeah I made it so it existing sites would work and actions already created would now be editable. But also made it so that new actions created would have a consistent naming pattern with other actions. I thought that seemed like the best solution but happy to take other opionions. Don't feel super stongly about it. I just prefer the consistency.

I'm not totally comfortable with allowing dots (.) in the machine name for actions, the reason being is that from a UI/UX perspective it diverges from the established convention of what the acceptable character set it. It's important that Core has a level of consistency across the UI, mainly because consistency is important from a UX perspective. When if a user becomes accustom to being able to use dots in the action ID, they could reasonably ask why they can't do this in other parts of Core. So if we're going to start allowing dots, we should do it consistently across core.

I'm also not convinced that this is the most robust solution, by allowing dots we're only putting a plaster over the root problem, which is that config can (in theory) provide any character in the machine name, whereas that's not possible in the UI. The machine name could contain a different illegal character and in theory the problem would still be triggered. Thanks @larowlan for raising that related issue.

Therefor, I recommend going back to the proposed solution of earlier in this issue, which is to not validate the machine name if it's not editable (see comment #13). That would be better UX because it ensures that the user never gets an error that they cannot do anything to address. That makes it a more robust solution as it actually addresses the root problem of the user getting an error they cannot do anything about. That would also be a simpler implantation as it works for new and existing sites, so no upgrade path required, maybe some test coverage though.

I notice this is already tagged for a usability review, what I've covered likely covers most of what would be brought up at a UX meeting, but I will raise this issue at the next meeting so others in the meeting can provide input as well.

Sorry what I meant to say was existing sites are still in the same boat as the original bug report, they can't edit those actions

My MR fixed that so it worked on new and existing :).

Setting to needs work until a decision is made on this. IMO since this is a valid bug in core, we should decide on what the fix is for that then open follow ups for everything else.

After realizing that view modes have a dot in their machine name, I think we should allow periods in the machine names of Actions as well.

Once a decision is made, we could also update the Wiki: https://www.drupal.org/docs/develop/user-interface-standards/machine-name

Edited May 14, 2022, to edit my stance.

Note: if wanting to update the label of an Action (e.g. for the Views Bulk Operations on the People page (/admin/people)), you can:

1. export the action (/admin/config/development/configuration/single/export), then
2. reimport it (/admin/config/development/configuration/single/import) with the updated label.

We reviewed this issue at #3279239: Drupal Usability Meeting 2022-05-13.

We discussed the various options available, and two key recommendations came out of the discussion.

First, we agreed that having a consistent approach to how machine names are handles across Core is important, so we did not think that changing the allowed characters was appropriate as it would mean the Actions module diverging from other parts of Core.

To this effect we agreed that it would be a sensible approach to change the actions generated by the user module so that they do not have a . in the machine name. That would ensure the User module is then observing the rules of allowed characters for machine names. It also means that the machines names generated by the User module actions are then consistent with what the user is able to create in the UI. We also noted that #2920678: Add config validation for the allowed characters of machine names would further aid in bringing consistency to machine names.

The second key recommendation that came out of the meeting is that we felt it was a good idea to investigate disabling validation on the machine name element if the element could not be edited by the user, thereby ensuring that the user would never see an error message that they are unable to take action on. We agreed though that this should be further investigated and progressed in a follow-up issue as the scope of such a change to the Machine Name element could have impact across Core and Contrib.

Both of these recommendations taken together would ensure that the actions generated have valid machine names and that the user does not get stuck in the situation described in this issue.

As a side note, taking these recommendations together may also simplify or completely negate the need for any upgrade path. In other words, if we split the problem across two issues, with this issue focusing just on the generation of actions, while a follow-up focuses on the fundamental problem of the machine name elements error validation. However, that is more of an implementation decisions and not a UX review decision, so leaving the issue status as is.

@DanielVeza please rebase and update target branch for MR to 10.1.x or create new MR/patch

Added reroll for 10.1.x.

Added reroll for 10.1.x.

Added reroll again.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Verified patch #63 on Drupal version 10.1.x. Patch applied cleanly but it doesn't fix the original issue mentioned in the ticket. The description of the machine name field is changed but I'm still getting the error when I try to save the configuration page. Attached screenshots for reference. When I verified the code, I can see some of the code in the MR!2086 of #42 is missing in re-rolled patch. I applied MR!2086 changes in Drupal 10 version and it applied cleanly. I was able to save the page without any error.

This is not yet ready for review.

Still needs an upgrade path, tests, followup, etc.

Addressed #64, Patch created, Please have a look.

But did not address the tags.

Patch created, Fixed the fail tests. Keeps as is in "needs work" to address the #69.

It could become outdated after #3427413: Stop using action in user module in role entity hooks