Problem/Motivation

At the moment it is possible to create config entity types than have config prefixes that do not start with the name of the module that provides the entity. This is bad news for default configuration installation and configuration dependency management. Neither or which will work as expected.

Proposed resolution

  • Ensure that the value returned by EntityType::getConfigPrefix() starts with the provider
  • If the config entity does not have a config_prefix annotation concatenate the provider and the entity type ID
  • If a config_prefix is supplied this is concatenated with the provider.

Remaining tasks

User interface changes

None

API changes

None

Files: 
CommentFileSizeAuthor
#7 2199483-7.patch17.95 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 64,711 pass(es). View

Comments

alexpott’s picture

Status: Active » Needs review
FileSize
17.78 KB
PASSED: [[SimpleTest]]: [MySQL] 64,485 pass(es). View
Berdir’s picture

Makes sense, two questions...

a) You kept all explicit config prefixes where the current one doesn't match what it would generate by default. Is that just to make the patch easier or is there a more specific reason? Because some of them are pretty weird, especially now with the removed module prefix ("config_prefix = entity" ?!)

b) In light of #2031717: Make entity module not required, can we also think about entities provided by components here? Right now, plugins provided by a component have "Core"/"Component" as the provider, simply because this was never thought about in AnnotatedClassDiscovery::getProviderFromNamespace() and the regex there produces that? Should we have an issue to discuss that and define it if should be core, core.$component (or something else) and another issue to special case that in the config prefix/dependency system (always exists)

alexpott’s picture

a) user.user_role.ID is repetitive. So to handle the repetition you can use the config_prefix annotation so it becomes user.role.ID. The specific example you're using is language. Where the entity type ID is language_entity - language.language_entity is pretty horrible but actually I do not like the entity type ID. I think it should just be language and the config files be language.language.ID - but that seems out of scope to me.

b) I'd be fine with core.entity_type_id.ID - i don't think we need to worry about the component since two core components can not supply entity types with clashing IDs.

Berdir’s picture

1) Yeah, language is weird, also the interaction between entities and language objects, saving them and so on. Agree that it's not for this issue to improve that. Avoiding the repetition by explicitly specifying it is fine with me, I wasn't sure if it's just temporary to keep the patch small or if it's still OK to define a config_prefix that is != entity type ID in the long term, sounds like it is.

2) I guess I agree. I think it is a bit weird because "Core"/"Component" isn't really a thing in that context, just a collection of components (there really shouldn't be any classes without a sub-namespace directly in Core IMHO). This is more obvious for "Component" but not relevant for us anyway as there can't be any component-provided entity types ;)

xjm’s picture

sun’s picture

Status: Needs review » Reviewed & tested by the community

Yay! This is exactly how I imagined all of our config/plugin/menu_link/route/whatnot IDs to work since 1-2 years.

It's pointless to have to manually include the extension/provider in declarations, because an extension/provider simply MUST NOT be allowed to create IDs outside of its own namespace.

That unilaterally applies to all declarations we have and the approach of this patch should be the leading example for everything else, IMHO.

My extension system revamp plans already foresee to make "core" an actual "extension"/provider itself, so that should not present a blocker.
(FWIW, "component" is irrelevant, since those are just random PHP libraries.)

damiankloip’s picture

FileSize
17.95 KB
PASSED: [[SimpleTest]]: [MySQL] 64,711 pass(es). View

Needs a reroll.

catch’s picture

Status: Reviewed & tested by the community » Fixed
Berdir’s picture

Title: Provide a default config_prefix based on entity type ID and provider » Change record: Provide a default config_prefix based on entity type ID and provider
Category: Bug report » Task
Priority: Critical » Major
Status: Fixed » Active
Issue tags: +Needs change record, +Missing change record

I think this at least needs to be updated/mentioned in existing change records and possibly a new one...

xjm’s picture

Closed #1888218: Default configuration entities provided by a module should include the module name in the machine name as a duplicate of this issue.

I have several questions about the patch that went in here:

  1. @alexpott said this was a hard blocker for #2080823: Create API to discover config entities' soft dependencies and use this to present a confirm form on module uninstall. However, there were no failing tests on that issue without this patch, nor any tests added nor changed in this issue. Where is the test coverage for whatever breaks horribly when this patch isn't in? My test in #1888218: Default configuration entities provided by a module should include the module name in the machine name was a different specific bug caused by this general problem that was since fixed in a different issue, so it won't work for test coverage.
  2. The names are extremely confusing. Either config_prefix is provider_module.config_entity_type_label_thing, or it's just config_entity_type_label_thing. In this patch, it's used to mean both.
  3. The patch changes expectations about the format of the config object name -- that it should be of the format provider_module.config_entity_type_label_thing instead of something.something.whatever.something -- but the new expectation is not codified anywhere; it just happens magically inside a getter. At the minimum, if it's so important, shouldn't it have a unit test for possible inputs?
  4. Shouldn't this have a change record?
xjm’s picture

Yes, that.

alexpott’s picture

  1. It is a hard blocker for #2080823: Create API to discover config entities' soft dependencies and use this to present a confirm form on module uninstall because that relies the assumption that the provider is the first part of the prefix. There are no failing test since all config entities already obeyed the convention this codifies. And they have too since the ConfigInstaller and ConfigManager::uninstall also make this assumption. It is just baked in everywhere. The only entity that did not (ConfigQueryTest) was only used to create entities - there are no default config entities of this type.
  2. I would like to see the config_prefix annotation completely removed but to do that we need properly namespaced entities - see #1862600: Entity type names/IDs are not properly namespaced by owner (e.g., taxonomy.term vs. taxonomy_term). In this patch if an entity type has a config_prefix annotation it always means that that entity type wants to override the default prefix and specify the second part. getConfigPrefix returns the full prefix which is either provider + id or provider + config_prefix. I can see how there can be some confusion over getConfigPrefix() and config_prefix - I'm open to suggestions - config_prefix_suffix sucks :)
  3. As noted in the answer to point 1 the assumption of provider is baked into the core of the config system. I would say that this patch enforces existing expectations about the format of a configuration file name.
    • I've updated https://drupal.org/node/2073793
    • I'm concerned about the emergence of 8.x to 8.x change records - at the very least we need to mark them as such. I'm happy to write a change record for the issue I just don't want to confuse people who are upgrading modules from 7.x to 8.x - do we have a policy on this?
alexpott’s picture

xjm’s picture

#2100203: Make config entities use dots in machine names consistently also needs to be re-evaluated in light of this issue and is possibly another duplicate.

catch’s picture

I don't think this needs a change notice - it's a small API addition to a new API. If config_prefix is completely removed we might want one then, but patch here does not break any APIs really.

Berdir’s picture

It does break existing config entities because they need to update their EntityType annotation.

xjm’s picture

We need to at a minimum update the documentation for ConfigEntityType::$config_prefix and especially ConfigEntityType::getConfigPrefix(), as they currently say exactly the same thing even though they're now completely different things. That should have been included in this patch TBH. Edit: the method docs are actually on EntityType::getConfigPrefix(), oh dear. EntityType has a config-specific method?...

My question is, what's the usecase for the config prefix being different from the configuration entity type ID in the first place? That seems like what would tell us what the property should be called. When would I ever want to call my node_type configuration objects balloon_hat?

Or is that why you're referencing #1862600: Entity type names/IDs are not properly namespaced by owner (e.g., taxonomy.term vs. taxonomy_term), and so config_prefix here is now just a hack to let us create the slightly more aesthetic node.type.page.yml instead of node.node_type.page.yml? Is there no other usecase for it?

It does break existing config entities because they need to update their EntityType annotation.

Yes, in the sense that they'll suddenly be saving views.views.view.viewname.yml and that their views.view.viewname.yml views will no longer work until they fix the config_prefix. This is part of what I was concerned about -- we are changing an expectation that will fail silently when someone updates HEAD. Edit: Another reason it would have been preferable to rename the property or method, so it would actually fail informatively for un-updated code.

@alexpott, change records serve two purposes: documenting before-and-after for upgrading your code, and notifying core and contrib developers of API changes in real time. Coupling these ideas seems great until you have a 3+ year dev cycle with 15 pages of API changes, and I've raised the issue before of how to make them manageable, but I haven't been given time to work on it this quarter. We only use 8.x to 8.x change records for things that are important to notify core and contrib developers of, not every little change. There are "introduced in branch" and "introduced in version" fields for change records, and hypothetically this should cover it, but people don't actually use the "introduced in version" field that way or consistently for alphas.

I could see a value in changing it to be something explicit like "Incompatible with" where we could have < 8.x, < 8.0-alpha9, < 8.1, or whatever, so that people can easily find the change record. File a feature request perhaps?

We could plan to get rid of the change record for this issue if and when we remove config_prefix entirely, and create the change record so that it references both issues.

alexpott’s picture

Title: Change record: Provide a default config_prefix based on entity type ID and provider » Provide a default config_prefix based on entity type ID and provider
Priority: Major » Critical
Status: Active » Fixed
tim.plunkett’s picture

Yes, in the sense that they'll suddenly be saving views.views.view.viewname.yml and that their views.view.viewname.yml views will no longer work until they fix the config_prefix.

Who is "they" here? We changed the annotation in the View entity class. The only people that will be affected are those providing new config entity *types* outside core, not default config entities themselves. You'll note that not a single YAML file was changed here.

Status: Fixed » Closed (fixed)

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

xjm’s picture