Problem/Motivation

When user_user_role_insert() is called it creates configuration objects for actions to add and remove the role. The configuration object id is system.action.user_add_role_action.ROLE_ID.
Currently this does not match the system.action configuration schema since it is defined as system.action.*.

Proposed resolution

There are two possible fixes.

Fix 1

Duplicate the system.action.* schema with a system.action.*.* schema. This has a number of downsides:

  • Where should the duplicated schema live? system.schema.yml or user.schema.yml?
  • What happens when two contrib modules define actions which use three part IDs?
  • Code duplication for no real gain

Fix 2 (current approach in patch)

Fix the wildcard look up to be greedy so when we fallback to look for system.action.*.* it will also match system.action.*. There has been discussion of whether the greedy matching will cause problems but in all cases the less greedy match is preferred. For example, when searching for matches for breakpoint.breakpoint.module.toolbar.narrow the following checks for schema will be made:

breakpoint.breakpoint.module.toolbar.*
breakpoint.breakpoint.module.*.*
breakpoint.breakpoint.module.*
breakpoint.breakpoint.*.*.* (This is the defined schema for this configuration)
breakpoint.breakpoint.*
breakpoint.*.*.*.*
breakpoint.*

... and finally

We also actually need to fix the system.action.* configuration schema because the configuration key is defined incorrectly as type: action.configuration.[plugin]. We need to look in the parent data for the plugin key,

Remaining tasks

  • Agree approach
  • Review and commit patch

User interface changes

None

API changes

None

Files: 
CommentFileSizeAuthor
#1 2259525.1.test-only.patch1.85 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,378 pass(es), 2 fail(s), and 0 exception(s). View
#1 2259525.1.patch28.96 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,515 pass(es). View

Comments

alexpott’s picture

Priority: Normal » Critical
Issue summary: View changes
Status: Active » Needs review
Issue tags: +beta blocker
FileSize
28.96 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,515 pass(es). View
1.85 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,378 pass(es), 2 fail(s), and 0 exception(s). View

So this change kind of got unexpectedly big since the current system.action.* config schema was broken

    configuration:
      type: action.configuration.[plugin]

Can not not ever get to the plugin key. It needs to be [%parent.plugin]

Also since I was adding a test to ConfigSchemaTest for the new greedy wildcard it became apparent that lobbing all the schema testing stuff into config_test is suboptimal - it means we're creating all the schema and configuration object in loads of tests that never use them.

Since the system.action.* schema is just plain wrong and we have no schema for system.action.user_add_role_action.* adn system.action.user_remove_role_action.* elvating to beta blocker.

Status: Needs review » Needs work

The last submitted patch, 1: 2259525.1.test-only.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review

Expected fail due to test only patch

Gábor Hojtsy’s picture

After a lengthy discussion with vijaycs85 and alexpott in IRC, I think this looks like a good direction. The only place where this could be a problem is abc.*.xyz but that kind of matching was not supported earlier either. So that means that abc.*.something can only have no schema defined if there was a schema defined for abc.*. So we are not making conflicting features available here just enabling the last star to slurp up all dots as well.

OTOH I *think* we had an issue where we discussed this earlier but decided against at the time, but cannot find it. I cannot recall why we would have done that, so unless we can come up with reasons, this looks good.

alexpott’s picture

Issue summary: View changes
vijaycs85’s picture

Status: Needs review » Reviewed & tested by the community

Changes looks good to me.

Gábor Hojtsy’s picture

I want to qualify that vijaycs85's comment was followed by extensive discussions in IRC. We confirmed this does not break existing schemas and is not a drastic change to how the wildcards apply :)

Gábor Hojtsy’s picture

1: 2259525.1.patch queued for re-testing.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Ok great. Nice catches with the underlying tests.

Committed and pushed to 8.x. Thanks!

Gábor Hojtsy’s picture

Yay, thanks!

Gábor Hojtsy’s picture

Status: Fixed » Closed (fixed)

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