Re-introduce entity.module as a configuration owner [#1857074]

The 1st part of a CMI filename is supposed to be the owning module. This is used for
- automated config cleanup (when the owning module is uninstalled)
- hook invocations on config import.

We do have a case, though, for config files owned by the entity system, now that the various entity controllers provide unified behavior with unified configuration across entity types.

- #1043198: Convert view modes to ConfigEntity introduces ViewMode config entities, and currently uses the view_mode.* prefix, which matches no existing module. Those config files are per entity type.
- #1852966: Rework entity display settings around EntityDisplay config entity introduces EntityDisplay config entities, and currently keeps the associated config files in the field.* namespace, which makes no sense at all. Those config files are per entity-type+bundle.
- field.module currently holds bundle-specific configuration of view modes in a god-awful variable of it's own. Those should move to a CMI file, that has no reason to be owned by field.module.

Proposals:

1) Move those config files to the system.* namespace as a stopgap fix to unblock those issues.
In the long run, grouping all those config files under 'system' makes very little sense though.
And how much more config items are we going to add in there for the sole arbitrary reason that the owner happens to be an include file rather than a module ?

2) Reintroduce an entity.module...

3) Move those config files to the respective [entity_type_providing_module].* namespaces.
Would mean a dynamic 'config_prefix' entry in the entity type definition. This is not supported by the config entity system.
And again, those are config for generic behavior handled by the entity system, so It's not node or user modules' hooks that should be invoked on config import,

4) other proposal ?

Comment	File	Size	Author
#24	entity_module-1857074-22.patch	1.53 KB	yched
#24
#22	entity_module-1857074-22.patch	1.54 KB	yched
#22
#20	entity_module-1857074-20-test-only.patch	1.06 KB	yched
#20
#20	entity_module-1857074-20.patch	1.54 KB	yched
#20
#16	entity_module-1857074-16.patch	489 bytes	yched
#16
#12	entity_module-1857074-5.patch	672 bytes	yched
#12
#10	entity_module-1857074-5.patch	672 bytes	yched
#10
#5	entity_module-1857074-5.patch	672 bytes	yched
#5

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #0.0

yched CreditAttribution: yched commented 3 December 2012 at 14:53

Issue summary:

View changes

typo

Comment #1

Damien Tournoud CreditAttribution: Damien Tournoud commented 3 December 2012 at 14:58

I think we should re-introduce the entity module. I'm not quite sure what was the rationale of killing it other then working around core systems deficiencies.

Comment #1.0

Damien Tournoud CreditAttribution: Damien Tournoud commented 3 December 2012 at 14:58

Issue summary:

View changes

wrong issue #

Comment #1.1

yched CreditAttribution: yched commented 3 December 2012 at 15:13

Issue summary:

View changes

typo

Comment #2

Berdir

German

Switzerland

CreditAttribution: Berdir commented 3 December 2012 at 22:46

The argument for removing entity.module was that we had dependencies from /includes and /lib on the entity system, which didn't make much sense (we did resolve most of that in the meantime, though) and that by the time entity.module was created, there was no lib.

If we'd re-introduce entity.module, not sure if we'd move everything back or if we'd just re-create it so that we have something that can own entity types and config...

Comment #3

yched CreditAttribution: yched commented 4 December 2012 at 11:50

That's my position too.

In a world before CMI, modules were the place where you could provide hooks implementations. The entity system didn't need to react to hooks, fine, there was no case for an entity.module.

With CMI, modules are also defined by the fact that they are the things that can own pieces of configuration.
The entity system is configurable, ergo, we need an entity.module...

Abusing system.module for that is a workaround that doesn't really scale conceptually.

Re #2:
Well, I guess if a class file reads from a config file, it should belong to the module. Same applies for config entity classes.
Then if a class file depends on the classes above, it should follow along ?

Comment #4

catch

he/him

English

CreditAttribution: catch commented 4 December 2012 at 12:46

I proposed entity module in the first place so don't mind putting it back again. It did actually react to a couple of hooks, but really only a couple (hook_module_pre_enable() iirc). Putting stuff into system module is a no go, the only reason to do that would be to avoid an entity module which is just silly.

If there's stuff from the entity system that can legitimately stay in core/lib then it probably should though.

Comment #5

yched CreditAttribution: yched commented 4 December 2012 at 13:05

Title:	Configuration owned by core subsystems	» Re-introduce entity.module as a configuration owner
Status:	Active	» Needs review

File	Size
entity_module-1857074-5.patch	672 bytes

May I suggest that we start by simply adding the module to unblock the issues mentioned in the OP, and then discuss whether some files need to move over there ?

Patch attached - suggestions welcome for the .info "description" and the .module @file docblock :-)

Comment #6

4 December 2012 at 13:51

Status:

Needs review

» Needs work

The last submitted patch, entity_module-1857074-5.patch, failed testing.

Comment #7

yched CreditAttribution: yched commented 4 December 2012 at 14:14

Status:

Needs work

» Needs review

#5: entity_module-1857074-5.patch queued for re-testing.

Comment #8

4 December 2012 at 15:08

Status:

Needs review

» Needs work

The last submitted patch, entity_module-1857074-5.patch, failed testing.

Comment #8.0

(not verified) CreditAttribution: commented 4 December 2012 at 15:08

Issue summary:

View changes

typos

Comment #9

yched CreditAttribution: yched commented 4 December 2012 at 18:33

Er, #5 had two test runs, each with one fail in a different test (views\Tests\Wizard\SortingTest for the last run, the previous one is not accessible anymore but the fail was in Image tests) - wtf ?

Comment #10

yched CreditAttribution: yched commented 4 December 2012 at 19:01

Status:

Needs work

» Needs review

File	Size
entity_module-1857074-5.patch	672 bytes

Heh, reuploading then :-)

Comment #11

4 December 2012 at 19:54

Status:

Needs review

» Needs work

The last submitted patch, entity_module-1857074-5.patch, failed testing.

Comment #12

yched CreditAttribution: yched commented 5 December 2012 at 13:16

Status:

Needs work

» Needs review

File	Size
entity_module-1857074-5.patch	672 bytes

So, interesting - three bot runs gave three different kinds of fails :
- one fail somewhere in Image tests (can't access the result log after a retest)
- one fail in views\Tests\Wizard\SortingTest
- one "Failed to run tests: PHP Fatal error encountered during run_tests.sh" - log shows a couple "Exception thrown without a stack frame in Unknown on line 0" exceptions somewhere between "Comment 'new' indicator", "Comment interface", "Book functionality"

All those tests individually run fine locally :-/

I've seen a bot hiccup in an another issue yesterday, so - trying my luck again ?

Comment #13

tim.plunkett

he/him

English

Philadelphia

CreditAttribution: tim.plunkett commented 5 December 2012 at 17:35

Status:

Needs review

» Reviewed & tested by the community

Quick! Before the bot eats it.

Comment #14

webchick

she/they

English

Vancouver 🇨🇦

CreditAttribution: webchick commented 6 December 2012 at 00:36

Assigned:

Unassigned

» catch

Normally we would mark this "needs work" pending a hook_help() implementation, per the docs gate. However, since this is just a stop-gap and we don't ultimately know what this module will provide by the end (similar to Layout module), I think this is probably fine.

I think catch might want to take a look at this though to make sure the "ultra lite" version passes his sniff test.

Comment #15

catch

he/him

English

CreditAttribution: catch commented 6 December 2012 at 12:33

Status:	Reviewed & tested by the community	» Fixed
Issue tags:		+revisit before beta

OK so this seems like the only way that we can get entity-centric functionality out of the field system into the entity system, which has to be done eventually if we want a complete entity API, and a field API that begins with fields.

I can't remember the last time we moved something to a module, moved it out again, then moved it back again, and I'm apparently agreeing with all three of those decisions. But.. the reasons have been different each time, so I'm going commit this - but moving anything to entity module (i.e. out of core/lib) needs to be evaluated case by case. The plumbing for configuration entities in general should very likely not depend on a specific module (even if they depend on the module system itself), in the same way the module system shouldn't be a module.

What we absolutely cannot do is have configuration entities as a system depend on the module system, because that'd be a bit too much like #1859110: Circular dependencies and hidden bugs with configuration overrides.

Tagging with 'revisit before release' - this really needs to be provisional based on those patches landing and a proper hook_help().

Comment #16

yched CreditAttribution: yched commented 6 December 2012 at 15:24

Status:

Fixed

» Needs review

File	Size
entity_module-1857074-16.patch	489 bytes

Oops, new required module, forgot the update function to enable it :-/

Comment #17

yched CreditAttribution: yched commented 6 December 2012 at 15:25

Title:

Re-introduce entity.module as a configuration owner

» [missing update] Re-introduce entity.module as a configuration owner

Comment #18

yched CreditAttribution: yched commented 8 December 2012 at 14:11

Seems like a no-brainer ? RTBC anyone ?

Comment #19

tim.plunkett

he/him

English

Philadelphia

CreditAttribution: tim.plunkett commented 8 December 2012 at 14:23

Issue tags:

+Needs tests

Maybe we should write a generic upgrade path test to say "ensure all required modules are enabled"?

Comment #20

yched CreditAttribution: yched commented 8 December 2012 at 15:10

File	Size
entity_module-1857074-20.patch	1.54 KB

entity_module-1857074-20-test-only.patch	1.06 KB

Fair enough. Added a check in BareMinimalUpgradePathTest.

Comment #21

8 December 2012 at 15:11

Status:

Needs review

» Needs work

The last submitted patch, entity_module-1857074-20.patch, failed testing.

Comment #22

yched CreditAttribution: yched commented 8 December 2012 at 15:15

Status:

Needs work

» Needs review

File	Size
entity_module-1857074-22.patch	1.54 KB

Update number needed a bump.
Not reuploading the 'test-only' patch, it's the same as in #20.

Comment #23

Berdir

German

Switzerland

CreditAttribution: Berdir commented 8 December 2012 at 16:39

Status:

Needs review

» Reviewed & tested by the community

Fix and test looks good to me, other than the trailing space there.

Commiters should be able to do a git apply --whitespace=fix to make it go away. Or does that happen by default now anyway with the .gitattributes? not sure...