Problem/Motivation

I first noticed this because any DisplayInterface-implementing subclass of ConfigEntityBase wasn't saving correctly. We were running into this over in #1841584: Add and configure master displays.

The problem is that they use a mix of public and protected properties, and ConfigStorageController::getProperties() assumes that only public properties are of interest.

Proposed resolution

My initial approach was to:

  1. Introduce a DisplayInterface::export() that each Display class could use to determine which of its own properties to export.
  2. Introduce a DisplayStorageController class that would interact with the above method.

Then, I realized this should be entirely generally applicable - we just shift the logic from ConfigStorageController::getProperties() into ConfigEntityBase::export(), and we're good to go. This should be completely transparent to other config entities.

Marking this major and a bug report because, without this fix, Displays CANNOT save properly. It's maybe better scoped as a task to do it at the generic level, but if it doesn't mess with any other config entities (as I'm hoping), then we can just evaluate it in the context of Displays.

User interface changes

Nada.

API changes

Other ConfigEntityBase children could use this if they wanted to, but the change SHOULD be entirely transparent to them. Testbot will tell.

Files: 
CommentFileSizeAuthor
#22 drupal-1849480-22.patch6.19 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 49,245 pass(es). View
#20 drupal-1849480-20.patch5.48 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 48,578 pass(es), 455 fail(s), and 174 exception(s). View
#1 configentity_export-1849480-1.patch2.69 KBsdboyer
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

Comments

sdboyer’s picture

Status: Active » Needs review
FileSize
2.69 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

first pass.

probably needs tests...like, the ones we never wrote before to verify that Displays save properly :/

Status: Needs review » Needs work

The last submitted patch, configentity_export-1849480-1.patch, failed testing.

sdboyer’s picture

Status: Needs work » Needs review

#1: configentity_export-1849480-1.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, configentity_export-1849480-1.patch, failed testing.

sun’s picture

Yes, we definitely want to do something about that, so thanks for creating this issue. I think Views has a similar need.

I think a new method on ConfigEntityInterface makes sense, but I'm not too fond of the name "export", since that is veeery ambiguous in the configuration system space. ;)

Long-term, I'd think we'd also need the counter-operation, actually; i.e., "import" (bleheheh).

The current/original code and logic was rather quickly invented for the initial Configurables/ConfigEntity concept patch. At that time, I looked into how Views and CTools are handling it and only saw some negated logic in there (i.e., extract all properties, except some reserved property names), which didn't really make sense to me, so I essentially added a positive/non-negated extraction of properties, and limited it to public properties.

In light of EntityNG and current attempts to add metadata to config entities, I wonder whether it wouldn't make sense to describe config entity properties a bit more formally. That said, I'm not up to speed with the new EntityNG world order yet.

sun’s picture

Oh, and btw, a more formal definition of properties would most probably also help with #1653026: [META] Use properly typed values in module configuration — i.e., the lack of data types. If we'd know what properties exist and of which data type they are, then we'd be one step closer to removing the string-casting for all config object values.

tim.plunkett’s picture

Views lets its storage controller handle how it is stored, which I think is correct. (It's completely different now from how it was as described in #5).
That's why getProperties even exists, see #1824484: Allow config storage controllers to define their own properties.

I disagree with this entire premise.

sdboyer’s picture

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

#1: configentity_export-1849480-1.patch queued for re-testing.

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

The last submitted patch, configentity_export-1849480-1.patch, failed testing.

sun’s picture

Sorry, I obviously missed that getProperties() change.

I don't understand why it was added on the storage controller though. The storage should not have a business in deciding what makes up the actual entity class/properties. Or in other words, the entity class properties are always entity-specific, not storage-specific — if you swap out the storage, the entity properties won't be different.

sdboyer’s picture

i included the getProperties change as a way of making this minimally invasive. The removal or discontinued use of that method is an API change (albeit a contained one, as the method is protected), and therefore is a wider discussion. i only know one place the method is called.

since it seems we won't come to an immediate resolution on this, i will file a separate patch that solves the immediate blocker for displays.

sdboyer’s picture

Really, the point at issue here is whether it is more appropriate for the entity class itself to know which of its properties should be written out, or if that is a concern more properly situated with the controller.

IMO, the simple case is the one where the entity itself keeps track of these sorts of things - keeps things self-contained. And there needs to be a reason to shift something out into a more complex pattern. But I'm not apprised of the whole concept and scope that goes into EntityStorageController, so I can't really weigh in on why that could be a more appropriate place to put this logic.

yched’s picture

I'd tend to agree with this. It's the role of the storage controller to decide how to save, but not *what* to save, which shouldn't change if the controller was swapped.
Same pattern as for non-config entities IMO. An entity type knows its own components.

sun’s picture

Yeah, that's what I'm saying, too (not sure whether my comments were fuzzy).

Entity::getProperties(), however, is apparently taken by the new EntityNG TypedData stuff already. So we cannot override that in ConfigEntityBase but need to find an alternative method name instead.

Since this might be migrated and merged into the EntityNG design in the long run, I don't think we need to spend a long time with finding a method name — anything except "import" and "export" will probably work for me :)

tim.plunkett’s picture

Should entities that use a subclass of ConfigStorageController be required to be an implementation of ConfigEntityInterface?

That question I think determines the resolution of this issue, and also clarifies the entire separation between ConfigEntity and Entity.

Should a contrib module be able to implement hook_schema() for a ConfigEntity and switch its storage controller to a subclass of DatabaseStorageController?

If so, we've made a mistake with the existence of ConfigEntityInterface. Fields are unfortunately bound to a base table, so currently a ConfigEntity could never be fieldable. Most of the overridden methods in ConfigEntityBase are done because "configuration entities are not fieldable".

Most of this belongs in another issue, but my point is that DatabaseStorageController depends on an externally defined schema, but not one controlled by the Entity class. ConfigStorageController should also not depend on the Entity class for its "schema" (list of properties to export).

Gábor Hojtsy’s picture

Looks like this is indeed a bigger conceptual problem, but we need a resolution for displays sooner in #1841584: Add and configure master displays, so looking forward to where @sdboyer submitted that other issue he mentions would be opened :)

sdboyer’s picture

@Gábor Hojtsy - yeah, i'm just gonna do a direct fix over in that issue that'll work for displays for right now. i'll put that up today.

we should let this discussion play out, it's important.

tim.plunkett’s picture

Component: block.module » configuration entity system

I'd love for this to be won't fixed. See #1841584-66: Add and configure master displays

sun’s picture

The minimum task and adjustment I see for this issue here is to move the getProperties() code from the storage controller to the entity class, since the storage controller should not have any knowledge about how the entity is composed/structured. The storage controller is about CRUD and I/O only.

Any kind of k/v store will be able to read and write config data just fine. To use the Entity\DatabaseStorageController, yes, one would have to define, supply, and set up a table schema first, which got inherently lost by moving from SQL to a file-based storage (and that's ultimately one more reason for #1648930: Introduce configuration schema and use for translation), but that's a different issue.

Speaking of, the closely related, low-level Config\DatabaseStorage backend still exists. Along those lines, a Config\Entity\DatabaseStorageController that simply uses a keyvalue table schema to store entities is perfectly possible and does not require a formal schema definition.

Apparently, Entity::getProperties() exists already, and it seemingly appears to have the exact same purpose, so perhaps all we need to do is to override that method in ConfigEntityBase::getProperties()?

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
5.48 KB
FAILED: [[SimpleTest]]: [MySQL] 48,578 pass(es), 455 fail(s), and 174 exception(s). View

That's actually ComplexDataInterface::getProperties() :(
To clarify my -1, I don't think there should be more than one place to control this.

This resolves my qualms about this issue.

Status: Needs review » Needs work

The last submitted patch, drupal-1849480-20.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
6.19 KB
PASSED: [[SimpleTest]]: [MySQL] 49,245 pass(es). View

Or, you know, a patch that works.

sun’s picture

Thanks! Still not a fan of the term "export", but happy to rename the method later.

Looks RTBC to me. @sdboyer?

sun’s picture

Status: Needs review » Reviewed & tested by the community
Dries’s picture

Status: Reviewed & tested by the community » Fixed

This looks good so I committed this to 8.x. Instead of 'getExportProperties' we could have used 'getConfiguration' or something. We can debate that separately; we can stick with the existing terminology for now. This is a major bug report so I want to get it out of the way for people to make progress.

sun’s picture

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