Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Followup of #2199483: Provide a default config_prefix based on entity type ID and provider
- Moves getConfigPrefix of the EntityTypeInterface to a new ConfigEntityTypeInterface
- Improves documentation
- Adds a unit test for all methods in ConfigEntityType to ensure getConfigPrefix logic is tested.
Beta phase evaluation
Issue category | Task because nothing is broken and we can continue to implement everything on EntityType and EntityTypeInterface. However doing this increasely will bleed content logic into config and vica versa. |
---|---|
Issue priority | Critical because it blocks #2430219: Implement a key value store to optimise config entity lookups and #2432791: Skip Config::save schema validation of config data for trusted data which add methods to ConfigEntityType which should not on EntityInterface. |
Prioritized changes | The main goal of this issue is unblock a performance issue. Also I would argue that this should be permitted on the reducing fragility provision because it will make API addition to Config entity types much simpler to enforce during the 8.x cycle. |
Comment | File | Size | Author |
---|---|---|---|
#34 | 2204697-2.34.patch | 14.28 KB | alexpott |
#34 | 31-34-interdiff.txt | 416 bytes | alexpott |
#31 | 2204697-2.31.patch | 14.31 KB | alexpott |
#24 | 2204697-24-config-entity-type-interface.patch | 12.93 KB | tstoeckler |
Comments
Comment #1
alexpottRe-scoped issue
Comment #2
alexpottComment #3
alexpottBorked patch
Comment #4
sunIt looks like this would be easier with #2194783: Rename EntityTypeInterface::isSubclassOf() to ::entityClassImplements() ?
Shall we postpone this issue on that one?
Comment #5
xjmCan we fix the naming and docs while we're at it, or does that need to be a separate issue?
Comment #6
mauzeh CreditAttribution: mauzeh commentedIn #2226437: Create ContentEntityTypeInterface I created a
ContentEntityTypeInterface
and made it extend theEntityTypeInterface
.Shouldn't
ConfigEntityTypeInterface
also extendEntityTypeInterface
?Comment #7
BerdirGood point, yes it should.
Comment #8
mauzeh CreditAttribution: mauzeh commentedalright, working on it, will post a new patch. This one needs a reroll so will do that too.
Comment #9
mauzeh CreditAttribution: mauzeh commentedComment #11
mauzeh CreditAttribution: mauzeh commentedwoops, forgot a use statement... this one should pass.
Comment #13
tstoecklerAwesome, thanks @mauzeh. I just have some minor points.
We have a standard of documenting functions starting with a third person verb so the previous one was actually correct.
Awesome that you added this documentation!
This is now sort of duplicated above, but I think it couldn't hurt to leave this in.
I don't see any entity :-) ?! Also it should be "entity *type*" I think.
Minor, but due to this issue you can do
$this->entityType = $this->getMock('Drupal\Core\Config\Entity\ConfigEntityTypeInterface')
and therefore avoid the disable...() call.
Comment #14
tstoecklerOops, #13.1 was actually documenting a variable not a method, so the change is correct.
Fixes the rest. Also re-includes the actual interface and the test for ConfigEntityType that was accidentally left out of the previous patch.
Comment #16
tstoecklerAhh, now I know which use statement @mauzeh was talking about in #11.
We also already have ConfigEntityTest in 8.x now, so moving the test methods there. And fixing one method that needed updating after renaming the entity storage classes.
Comment #18
tstoecklerDon't see how that would be caused by this. Retesting
Comment #19
tstoeckler16: 2204697-16-config-entity-type-interface.patch queued for re-testing.
Comment #21
tstoecklertstoeckler-- for the time it took me to figure this out... Let's see.
Because of the missing use statement the array_filter callback in getEntityTypeIdByName() was never returning TRUE. Thus the check whether a config object is in fact a config entity in ConfigInstaller failed and fields were being saved as dumb config objects and thus the tables were never created. Fun! :-)
Comment #22
tstoecklerComment #24
tstoeckler...
Comment #25
BerdirWhy would we want to add tests for those here?
Comment #26
BerdirStill confused about that...
Comment #27
tstoecklerCan you explain your confusion? This just increases the test coverage of the ConfigEntityType class. Nothing else. It's not technically related to this issue but it seemed easy enough to do here so I thought why not?
Comment #28
xjm@tstoeckler, because it's not in scope and that makes the patch harder to review/commit. :) Might be good to spin off a separate issue "Increase test coverage for ConfigEntityType" with that part of the patch?
Comment #29
BerdirIt adds test coverage for methods that have nothing to do with config entities and there's an issue to remove them completely, so why add it here and why add it at all? :)
Comment #30
mgiffordComment #31
alexpottResurrecting because not doing this totally ties our had at doing API additions to only config entity types during the 8.x cycle and issues like #2430219: Implement a key value store to optimise config entity lookups need to add methods to config entity types that have no business being on the content entity types.
I removed the unnecessary tests added by #24 which stalled this patch in the first place.
Comment #32
alexpottThis issue is a normal task so we need to outline how it fits within the allowable Drupal 8 beta criteria. Can someone add Drupal 8 beta phase evaluation template to the issue summary.
If no one else does - will do myself later.
Comment #33
alexpottAdded beta eval. Bumping to major because blocks have a place to add the public methods needed for #2430219: Implement a key value store to optimise config entity lookups
Comment #34
alexpottFixing missing new line.
Comment #35
BerdirLooks good to me.
It is a small API change, but having this interface is very useful and the only way it breaks if someone is calling getConfigPrefix() unconditionally on all entity types. I assume that outside of core, there are very few reasons to call that method at all.
Comment #36
xjmThis needs a change record. Thanks!
Comment #37
alexpottCreated the CR https://www.drupal.org/node/2480283
Comment #38
xjmThanks @alexpott.
Comment #39
alexpotthttps://www.drupal.org/node/2181667 needs updating to remove the irrelevant mentions of getConfigPrefix() from the examples.
Comment #40
xjmI made the following updates:
https://www.drupal.org/node/2181667/revisions/view/6879507/8418563
Comment #41
xjmSo, per @alexpott, #2430219: Implement a key value store to optimise config entity lookups is now critical, and this issue is also now blocking #2432791: Skip Config::save schema validation of config data for trusted data, which is also critical. I asked @alexpott if it was a hard blocker, and he pointed out that it's increasingly fragile to have these various config-specific methods on EntityInterface but returning
FALSE
orNULL
or whatnot for content entity types. Therefore, elevating this to critical.instanceof ConfigEntityType
checks, but I see @Berdir is already looking at #2194783: Rename EntityTypeInterface::isSubclassOf() to ::entityClassImplements() which will address a related problem space.Hmm. So if the entity type is an instance of ConfigEntityTypeInterface, but getConfigPrefix() returns something that is false, or the name doesn't begin with the config prefix expected... then what happens?
This isn't introduced by this patch, but it seems like now this is a set of things that would mean the entity type were malformed if the first were true but not one of the others. I think. Can we get a followup to at least add an inline comment to document this, and maybe investigate whether this line is something that should be refactored?
So the first and third hunks of documentation are added by this patch, and the first is what we have in HEAD. That means we're explaining what a config prefix is composed of in why in different ways in three different places. We should consolidate that and clarify it.
Normally, since these added lines of documentation are the majority of non-moved lines in the patch, I'd ask to fix this before it went in. However, @alexpott said that this issue is now blocking two criticals, so I'm okay for that to be done in a followup instead.
I did fix a small bit of pirate speak on commit:
(Ensures it be created, arrr.)
Committed and pushed to 8.0.x. Thanks!
Comment #43
xjmComment #44
joachim CreditAttribution: joachim commentedThis has broken #2409677: add a method to EntityTypeInterface to determine whether an entity type is content or config. It also affects contrib -- AFAIK, calling getConfigPrefix() was the simplest way to determine if a given entity is a config or content entity.
Comment #45
BerdirWell, now the simplest way is instanceof ConfigEntityTypeInterface. Which is way more explicit.
The official way to do that so far was isSubclassOf(), wich I want to deprecate in favor of the instanceof check.
Comment #46
xjm@joachim, core also does
instanceof ConfigEntityType
in a few places, which (per #41) need to be converted toinstanceof ConfigEntityTypeInterface
. See, for example, EntityManager::loadEntityByConfigTarget().In #45 @Berdir is referring to #2194783: Rename EntityTypeInterface::isSubclassOf() to ::entityClassImplements().
Comment #47
jibranPublished the change record. Config-specific entity type functionality moved to a new ConfigEntityTypeInterface