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.
Problem
- Having the entity system in a module causes a circular dependency for other base modules in core.
Details
- The entity system was moved into entity.module, but that happened before we introduced PSR-0 and OO code in Drupal\Core.
- The entity system itself is pure API code/infrastructure. It does not have a database schema or anything else attached to it that would require the module system for itself to function.
- hook_entity_info() will soon most probably be replaced with plugins.
- The entity system invokes module hooks, but that doesn't prevent itself from being autoloadable and located in Drupal\Core.
Related issues
Comment | File | Size | Author |
---|---|---|---|
#36 | drupal8.entity-core-comment.36.patch | 622 bytes | sun |
#23 | entity-core.sh_.txt | 2.05 KB | sun |
#23 | entity.core_.23.patch | 141.13 KB | sun |
#21 | entity-core.sh_.txt | 2.05 KB | sun |
#21 | entity.core_.21.patch | 141.49 KB | sun |
Comments
Comment #1
sunEssentially these commands:
Then, globally search & replace: (trailing backslash is important)
Lastly,
cat modules/entity/entity.api.php >> modules/system/system.api.php
and clean up the resulting artifact:» We should consider to allow .api.php files in core/includes/ though.
git rm -r modules/entity
_drupal_bootstrap_code()
:Comment #3
sunSorry, forgot a commit.
Comment #5
sunMerged in HEAD. I so love those typo fix commits. :(
Comment #7
catchWe won't be able to do that properly until the module/hook system is also a core component, but it's no worse than it being a module.
Comment #8
sunAnother highly important aspect to this is:
Every module that uses Configurables needs to have a dependency on config.module (which in turn has a dependency on entity.module).
Since Configurables are just merely a minor extension of Entities, that entire dependency chain makes even less sense.
The early/original patches in #1668820: Concept, base class, and interface for Configurables (e.g., node types, image styles, vocabularies, views, etc) added the Configurable class into Drupal\Core to circumvent that problem, but when it got reworked onto the entity system, that no longer made sense, so the Configurable class was added to config.module instead.
Lastly, there's also a plan to #1763974: Convert entity type info into plugins, which further removes dependencies on procedural code and has been discussed a couple of times already, but didn't exist as an issue yet, so I just created it.
Comment #9
dagmarThis patch need a full re-roll due #1761040: Rename Storable, Entity, and Configurable to Entity, ContentEntity, and ConfigEntity.
I'm moving this to major since we need this in order to implement the following patches:
#1779026: Convert Text Formats to Configuration System
#1754246: Languages should be configuration entities
#1588422: Convert contact categories to configuration system
Comment #10
cosmicdreams CreditAttribution: cosmicdreams commentedI tried to apply this patch and update my site and was able to get the following feedback :
Comment #11
sunI've converted the instructions in #1 into a shell script.
Comment #13
sundang! :(
Comment #14
sunFixed the script.
Comment #16
sunNo change to the script; only needed an additional tweak to the post-script patch.
Comment #18
sunManually searched for remaining "'entity'" references and hit a couple of instances that need to be corrected in the post-script patch.
Comment #20
tim.plunkettI think this is wrong
Comment #21
sunThanks! Attached patch should fix those.
Slight adjustment to the script; same post-script patch from #18.
Comment #22
sunwhoopsie!
Comment #23
sunComment #24
tim.plunkettAssigning to fago for his opinion, but I think this is RTBC.
Comment #25
fagoYeah, this totally makes sense to me. I was not able to find any glitches in the patch, thus marking RTBC.
Yep. crell mentioned that moving to symfony events would help us to untangle this dependency on the module system. Being able to load an entity without requiring the module system to be loaded would certainly be a good goal.
Comment #26
catchWe could also just do #1331486: Move module_invoke_*() and friends to an Extensions class to resolve the dependency issues - i.e. make the module sytem a component that can be dependency injected. I'd like to take a proper look at this before it goes in, so assigning to me, that shouldn't stop review from any other core committers though.
Comment #27
plachAssigning as per #26.
Comment #28
sunThere's a difference to #1331486: Move module_invoke_*() and friends to an Extensions class in that the primary problem here is that the entity system functionality is a required dependency for almost every module (which even applies to modules that do not provide "content" entities, but only configurable things now).
However, the entity system itself does not have any requirements other than having to be loaded. It doesn't have a db schema, it doesn't need to be installed, and so on. It doesn't have a UI. In the end, it is just code that is required all over the place, and just needs to be loaded.
Those are the main differences from my perspective. Thus, it doesn't really change anything whether the module/extension system will be able to be bootstrapped earlier or similar - the situation with the entity system is still the same; it's not a module, it's a component.
Comment #29
catchSorry sun, I meant an injectable Extensions class as an alternative (or more doable first step) to wholesale moving to Symfony's events, not as an alternative to moving this back to a component which I'm in favour of (despite having started the 'move it to a module' issue but that was a long time ago).
Comment #30
Lars Toomre CreditAttribution: Lars Toomre commentedReading #28 makes me wonder whether the entity system should be registered in DIC. Am I wrong with that thought?
Comment #31
catchThere's a very long list of dependencies that would need to be registered in the DIC before the entity system could be. Eventually that would be a good idea but we're a long way off at the moment, we need to tackle issues like #1764474: Make Cache interface and backends use the DIC first.
Comment #32
catchThis doesn't sit right with the comment, should probably be above it. Also we only added those hooks because they were necessary to allow entity.module - should we remove the hooks too?
It's a bit sad we have to re-introduce knowledge of the entity system to module.inc again with this patch, but we might be able get around that by converting hook_entity_info() to plugins or plugins + config or similar.
Apart from the comment agreed with RTBC, unassigning myself.
Comment #33
sun#23: entity.core_.23.patch queued for re-testing.
Comment #34
sunFor now, I wanted to make sure that the entity info cache clears happen at the right time, so I put them right in front of the hook invocations.
Can we make that line/comment change a follow-up patch?
Comment #35
catchYep. Committed/pushed to 8.x. We'll need to delete or strike out [#1287074] so leaving open for that and the comment follow-up.
Comment #36
sunThe comment fix.
I've deleted the change notice that told about the entity system being moved into a module. Considered to replace it with something new, but alas, there is factually nothing changed between D7 and D8 with the move forth and back. The other entity system changes are covered in other change notices already.
Comment #37
alexpottComment move looks good :D
Comment #38
sunFollow-up: #1785974: Move ConfigEntity into a Core component
Comment #39
aspilicious CreditAttribution: aspilicious commentedWith the change notice deleted this isn't critical anymore.
Comment #40
catchCommitted/pushed the follow-up. Marking fixed. Thanks!