Problem/Motivation

If I create a role and have the Actions module enabled I can edit the role created by user_user_role_insert() - but I can not save. This is because the ID has a dot in it.

Proposed resolution

There are currently three proposals:

  1. Allow actions to have dots in their config names (as is true of entity_view_mode, entity_form_mode, block, and config_test)
  2. Change user_user_role_insert to not create actions with dots
  3. Add a UI lock on actions, and lock those created via user_user_role_insert (similar to date_format).

#3 might be appropriate since these actions are created and deleted by the User module. In fact the user_user_role_delete is completely unnecessary because of configuration dependencies. So therefore it could be possible to remove that and just create an action with an ID that can be edited.

However this is related to a wider issue of should configuration entities be able to control the meaning of dots in their ID see #2100203: Make config entities use dots in machine names consistently

Remaining tasks

Agree approach

User interface changes

tbd

API changes

tbd

Data model changes

tbd

Why is this a possible RC target

The UI makes it look like such actions are editable but they are not. We have a form where the submit is impossible - regardless of what the user does.

CommentFileSizeAuthor
Screen Shot 2015-10-30 at 16.10.37.png134.77 KBalexpott
#3 2605042-3.patch1.74 KBalexpott
#5 2605042-action-5.patch2.35 KBtim.plunkett
#5 interdiff.txt621 bytestim.plunkett
#7 2605042-action-7.patch5.91 KBtim.plunkett
#7 interdiff.txt5.48 KBtim.plunkett
#11 2605042-action-11.patch12.82 KBtim.plunkett
#11 interdiff.txt6.92 KBtim.plunkett
#27 2605042-action-27.patch12.82 KBmohit_aghera
#27 interdiff-2605042-11-27.txt611 bytesmohit_aghera
#31 2605042-31.patch12.82 KBjofitz
#33 interdiff-31-33.txt2.05 KBjofitz
#33 2605042-33.patch13.9 KBjofitz
#43 2605042-nr-bot.txt132 bytesneeds-review-queue-bot
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Related issues: +#2597608: User role action labels get double-escaped and out of sync with the corresponding user role label

This was discovered whilst investigating #2597608: User role action labels get double-escaped and out of sync with the corresponding user role label

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Issue summary: View changes
Status: Active » Needs review
Issue tags: +rc target triage
FileSize
2605042-3.patch1.74 KB

Here's a failing test.

Status: Needs review » Needs work

The last submitted patch, 3: 2605042-3.patch, failed testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett as a volunteer commented
Issue summary: View changes
Status: Needs work » Needs review
FileSize
2605042-action-5.patch2.35 KB
interdiff.txt621 bytes

Adjusted the IS to clarify the options. This is a patch for approach #1. Patch for #3 will follow, I just wanted to prove we have an easy out.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

The problem with proposal 1 is that then someone could feasibly create a clash with the action created by user_user_role_insert()

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett as a volunteer commented
FileSize
2605042-action-7.patch5.91 KB
interdiff.txt5.48 KB

#6, I don't follow. The machine name FAPI #type still enforces uniqueness, even with dots.

Here's a patch for approach #3.
Interdiff is against #3, not #5

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

re #6 Someone could create the action for another role with the same ID as a role they will create in the future - madness yes - but also possible.

Status: Needs review » Needs work

The last submitted patch, 7: 2605042-action-7.patch, failed testing.

The last submitted patch, 7: 2605042-action-7.patch, failed testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett as a volunteer commented
Status: Needs work » Needs review
FileSize
2605042-action-11.patch12.82 KB
interdiff.txt6.92 KB

Wow, \Drupal\KernelTests\Config\DefaultConfigTest is really cool.
Adding false to all exported action files.
Does that need an update hook? The property defaults to FALSE, should work...

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett as a volunteer commented 
+++ b/core/modules/system/src/Entity/ActionAccess.php
@@ -0,0 +1,31 @@
+    if ($operation === 'update' && $entity->isLocked()) {

I only checked for update, should I have included delete?

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett as a volunteer commented
Component: user.module » action.module
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

re #12 I think so - I don't think a user should be able to delete such actions through the UI either - that said there is no harm in allowing them to do this.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett as a volunteer commented

See also #1831928: Support a 'locked' property on ConfigEntities

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Issue tags: +Needs issue summary update
Related issues:
kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: kristiaanvandeneynde at Deeson commented
Related issues: +#2553765: Actions created in user_user_role_insert contain a '.' and cannot be updated via the UI

Seems like a duplicate of #2553765: Actions created in user_user_role_insert contain a '.' and cannot be updated via the UI, the other issue being older than this one. Which one should be closed?

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: kristiaanvandeneynde at Deeson commented

Although the patch in #11 does look really good.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

@kristiaanvandeneynde I think we should choose the approach in this issue rather than the other issue so I suggest we close the other issue and give people who work it on commit credit on this issue.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: kristiaanvandeneynde at Deeson commented

Yeah, funnily enough I came up with the same idea as summary item #3. So seeing as this issue already has code for that approach, it makes sense to close the other issue. I'll go ahead and close the other one.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

We need to add heykarthikwithu, StryKaizer, kristiaanvandeneynde, damiankloip, Jeremy to the commit credit when and if this gets in.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Issue tags: -rc target triage
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Issue tags: +Triaged core major

Discussed with @xjm and @catch - we agreed this was a major bug because it looks like you can make a change in the UI but in reality you just can't - and the error makes it look like the machine ID is wrong and you should change it.

lokapujya’s picture

lokapujya
Primary language English
Location Boston
CreditAttribution: lokapujya commented
Status: Needs review » Needs work 
  1. +++ b/core/modules/user/src/Tests/UserRoleActionsTest.php
@@ -0,0 +1,57 @@
+    // Add a role

    Finish comment with a period.

  2. Is any update hooks needed?

and the error makes it look like the machine ID is wrong and you should change it.

An editor can get into this same state if they fail to use a unique label for the action. At this point, you cannot change the machine name, and need to hit the back button. Not the coolest UX, but that is a separate issue.

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.

mohit_aghera’s picture

mohit_aghera CreditAttribution: mohit_aghera as a volunteer and at Axelerant for Red Hat commented
Status: Needs work » Needs review
FileSize
2605042-action-27.patch12.82 KB
interdiff-2605042-11-27.txt611 bytes

Updating changes mentioned by @lokapujya
Applying patch on 8.3.x branch.

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.

Chi’s picture

Chi CreditAttribution: Chi commented
Status: Needs review » Needs work
Issue tags: +Needs reroll

The patch is outdated.

jofitz’s picture

jofitz CreditAttribution: jofitz at ComputerMinds commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
2605042-31.patch12.82 KB

Re-rolled.

Status: Needs review » Needs work

The last submitted patch, 31: 2605042-31.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

jofitz’s picture

jofitz CreditAttribution: jofitz at ComputerMinds commented
Status: Needs work » Needs review
FileSize
interdiff-31-33.txt2.05 KB
2605042-33.patch13.9 KB

Addressed test failures.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Issue tags: +Needs change record
Related issues: +#2289551: Clarify what 'locked' means for a config entity and whether it's okay for code to rely on a locked config entity existing

New methods needs CR, also "locked" discussed in #2289551: Clarify what 'locked' means for a config entity and whether it's okay for code to rely on a locked config entity existing

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.

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

Drupal 9.4.9 was released on December 7, 2022 and is the final full bugfix release for the Drupal 9.4.x series. Drupal 9.4.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.5.x-dev branch from now on, and new development or disruptive changes should be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

needs-review-queue-bot’s picture

needs-review-queue-bot CreditAttribution: needs-review-queue-bot as a volunteer commented
Status: Needs review » Needs work
FileSize
2605042-nr-bot.txt132 bytes

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or 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.

Version: 9.5.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Related issues: +#3150316: Configured actions created by User module cannot be edited, +#3427413: Stop using action in user module in role entity hooks

Looks closely related #3150316: Configured actions created by User module cannot be edited

Actions locking probably can be moved to contrib, but not clear yet with views+user #3427413: Stop using action in user module in role entity hooks

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Component: action.module » user.module