Follow-up to #1760786: Move entity system "back" into a Drupal\Core component

Problem

  • Every module that has configurable thingies needs to depend on config.module.

Goal

  • Untangle ConfigEntity from Config module.

Proposed solution

  1. Move the ConfigEntity* classes into Drupal\Core\ConfigEntity\.

  2. Remove dependencies on config.module from existing modules in HEAD providing Configurables (config_test.module).

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott’s picture

Assigned: Unassigned » alexpott

Gonna take a crack at this tonight.

alexpott’s picture

Assigned: alexpott » Unassigned
Status: Active » Needs review
FileSize
32.6 KB

Moves the ConfigEntity classes to Drupal\Core\ConfigEntity\

Status: Needs review » Needs work

The last submitted patch, 1785974_3.drupal8.configentity_to_core.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: +VDC
FileSize
1.01 KB
47.8 KB

Because config_test.module used to depend on config.module, the entity_info cache would be cleared just at the right time to prevent the test from blowing up.

Now that it has no dependencies, ConfigInstallTest is exposing the bug, which is shown in the interdiff.

Also, rerolled with renames on so that the patch is easier to read.

effulgentsia’s picture

Wrong patch posted to #4?

Status: Needs review » Needs work

The last submitted patch, views-1751358-12.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
5.58 KB

Whoops! That's what I get for working on two issues at once.

sun’s picture

Looks ready to fly for me if bot comes green.

Last call for bikeshed: Do we wanna go with Core\ConfigEntity\ ? ;)

(Backstory: Neither @alexpott nor me knew whether it belongs into Entity or Config, so I just simply put ConfigEntity into the summary :P)

tim.plunkett’s picture

I was curious about that as I rerolled it, it seems really ugly as-is.

Since we have Drupal\Core\Entity\ContentEntityInterface, it would make sense to have it all live under Drupal\Core\Entity. And we could clean up a good amount of use statements too...

gdd’s picture

Core\ConfigEntity makes sense to me. If it needs to live somewhere else then personally I think Config\ is a better fit than Entity\. It is really a member of the Config subsystem, not the Entity subsystem (even though it is an implementation of Entity.)

effulgentsia’s picture

My vote would be to put it into Drupal\Core\Config, which may require renaming ConfigStorageController to ConfigEntityStorageController to disambiguate ConfigEntity storage from Config storage. But I don't feel strongly enough about this to hold up the issue for it. I agree with #10 that ConfigEntity is no more part of the Entity system than Node is.

Status: Needs review » Needs work
Issue tags: -Entity system, -Configuration system, -VDC, -Configurables

The last submitted patch, config-1785974-4.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
Issue tags: +Entity system, +Configuration system, +VDC, +Configurables

#7: config-1785974-4.patch queued for re-testing.

tim.plunkett’s picture

Oh, hmm, after reading the other replies I agree.
I was thinking solely about ContentEntityInterface, which is probably also misplaced...

sun’s picture

FileSize
4.26 KB
5.6 KB

Let's do it in a true component way.

Moved Core\ConfigEntity\* into Core\Config\Entity\*.

alexpott’s picture

Status: Needs review » Reviewed & tested by the community

Makes sense 1+

andypost’s picture

+1 it removes a lot of dependencies

catch’s picture

#15: config.core_.15.patch queued for re-testing.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

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