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
Comment | File | Size | Author |
---|---|---|---|
#1 | 2259525.1.test-only.patch | 1.85 KB | alexpott |
#1 | 2259525.1.patch | 28.96 KB | alexpott |
Comments
Comment #1
alexpottSo this change kind of got unexpectedly big since the current system.action.* config schema was broken
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.
Comment #3
alexpottExpected fail due to test only patch
Comment #4
Gábor HojtsyAfter 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.
Comment #5
alexpottComment #6
vijaycs85Changes looks good to me.
Comment #7
Gábor HojtsyI 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 :)
Comment #8
Gábor Hojtsy1: 2259525.1.patch queued for re-testing.
Comment #9
webchickOk great. Nice catches with the underlying tests.
Committed and pushed to 8.x. Thanks!
Comment #10
Gábor HojtsyYay, thanks!
Comment #11
Gábor Hojtsy