Work on this is going on in the entity-decouple branch of the D8 Entity API sandbox.

Overview

In Prague plach (discussion lead), yched, fago, berdir, amateescu and das-peter discussed "unified" field storage, see the notes here. I'm copying the relevant part of the notes here:

Discussion

  • Support field API storage for non-configurable fields brings terminology questions up. How is a field called that’s stored via the field API mechanism but not configurable in the field UI?
  • We determined, in addition to field API, and base-table storage (both part of the storage controller), there is the case of fields being stored in a custom way by modules. That is, modules add the field via hook_entity_field_info() and take care of storage theirselves.
  • After some discussion we came up with the following solution/proposal.

Solution

We iterate over some keys in entity and field definition to clarify terminology + improve the storage controller to work based upon entity field definitions instead of field instances (see steps for details on that).

Entity definitions

Field definitions

  • provider: this key is used to tell which module provides the field defintions; Configurable fields will have field - so this can be used by the Field module to identify its fields. Then, this key allows us to identify base fields (=fields provided with the entity types) as they have a NULL values. Lastly, it’s something important to have in order to be able to track dependencies, e.g. when a module is configured to rely on a field to be there (a rule configuration needs it to run, a module uses a token of that field, ..) - #2143069: Add getProvider() and hasCustomStorage to FieldDefinitionInterface
  • custom_storage: this indicates whether the storage controller should take care of storing the field values - #2143069: Add getProvider() and hasCustomStorage to FieldDefinitionInterface
  • The core SQL storage controller determines a list of fields to be stored in the base table (optimized) storage (single-cardinality base fields). Any other field, except custom storage fields, gets CCK tables.
    • This optimization is something internal to the storage controller, so the method is not on an interface.
    • A MongoDB storage controller would just store every field not having with custom storage enabled, as it does not have to do any special optimizations.

Steps:

  1. this depends on the implementation of class based field definitions for entity fields, so the issue is a prerequesite.
  2. move the getSchema() method for ConfigFieldItem to FieldDefinitionInterface
  3. move getColumns() from Field(ConfigEntity)Interface to FieldDefinitionInterface
  4. customize ContentEntityDatabaseStorage / Interface to work with Field definitionn (this issue):
    • onFieldCreate(FieldDefinitionInterface)
    • onFieldUpdate()
    • onFieldDelete()
    • onBundleFieldCreate(FieldDefinitionInterface, bundle_name)
    • onBundleFieldUpdate()
    • onBundleFieldDelete()

    + adapt the code in the method implementations (probably means adding missing methods in FieldDefinitionInterface)

  5. field purging: we need to abstract the current “storage of deleted field / instance definitions in State()” to “register some FieldDefinitionInterface for purging” (stored in State() too, but needs to store <code>FieldDefinitionInterface objects istead of CMI data. (follow-up)
  6. as fieldable is renamed, we’ll have to rename the FieldableStorageControllerBase // Interface accordingly to extensible storage - #2188613: Rename EntityStorageController to EntityStorage

Postponed until

#2226197: Introduce FieldStorageDefinitionInterface in the Entity Field API

Goals

The goals of this issue are to

  • have FieldableEntityStorageInterface be decoupled from configurable field interfaces, i.e. use entity field interfaces only
  • clean FieldableEntityStorageInterface up, such that it is a good API that modules can use for notifying the entity storage of changes to field definitions in general (for base fields as well)

Remaining tasks

  • Get reviews & address them

API changes

- Added getBundle() to FieldDefinitionInterface and getUniqueStorageIdentifier() to FieldStorageDefinitionInterface
- (Undocumented) support for running queries on deleted fields via Entity Query is gone
- Removed FieldConfig::entityCount() and replaced it by countFieldData() on the storage. One can still count entities using entity query, but not by using deleted fields.
- Added purgeFieldData(), countFieldData() and finalizePurge() to FieldableEntityStorageInterface and respective base classes + clarified docs.
- FieldableEntityStorageInterface has been improved to go with EntityFieldQuery terminology - those methods have been used by field.module only so far though

Files: 
CommentFileSizeAuthor
#132 interdiff.txt665 bytesyched
#132 d8_entity_decouple-2144263-132.patch100.42 KByched
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,790 pass(es).
[ View ]
#130 2144263-reroll.130.patch104.45 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,882 pass(es).
[ View ]
#127 2144263.127.patch104.45 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2144263.127.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#127 120-127-interdiff.txt602 bytesalexpott
#120 interdiff.txt3.33 KBeffulgentsia
#120 2144263-120.patch104.39 KBeffulgentsia
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,786 pass(es).
[ View ]
#118 2144263-yched.118.patch104.62 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,934 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#116 2144263-yched.116.patch104.35 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2144263-yched.116.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#116 113-116-interdiff.txt328 bytesalexpott
#113 2144263-yched.113.patch104.77 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 44,037 pass(es), 14,325 fail(s), and 7,442 exception(s).
[ View ]
#113 111-113-interdiff.txt698 bytesalexpott
#111 interdiff.txt3.49 KByched
#111 d8_entity_decouple-2144263-111.patch100.03 KByched
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,767 pass(es), 2 fail(s), and 1 exception(s).
[ View ]
#109 interdiff.txt699 bytesyched
#109 d8_entity_decouple-2144263-109.patch103.02 KByched
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,807 pass(es).
[ View ]
#107 interdiff.txt1.71 KByched
#107 d8_entity_decouple-2144263-107.patch102.93 KByched
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,797 pass(es), 1 fail(s), and 4 exception(s).
[ View ]
#105 d8_entity_decouple-2144263-105.patch102.43 KByched
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,693 pass(es), 50 fail(s), and 14 exception(s).
[ View ]
#103 interdiff.txt4.55 KByched
#103 d8_entity_decouple-2144263-103.patch292.81 KByched
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch d8_entity_decouple-2144263-103.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#100 d8_entity_decouple-2144263-100.patch101.47 KBplach
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,970 pass(es).
[ View ]

Comments

fago’s picture

Issue summary:View changes
yched’s picture

amateescu’s picture

@yched created a child issue for handling the schema() stuff: #2144327: Make all field types provide a schema()

Maybe we should turn this one into a meta?

fago’s picture

> Maybe we should turn this one into a meta?

Seems reasonable. How do we do metas now - just go with the parent-issue feature?

amateescu’s picture

I would say so :) I already did that for two issues that I knew about.

fago’s picture

Title:Decouple entity field storage from configurable fields» [META] Decouple entity field storage from configurable fields

ok

plach’s picture

Title:[META] Decouple entity field storage from configurable fields» Decouple entity field storage from configurable fields
Priority:Major» Critical
Issue tags:+D8MI, +language-content, +sprint, +beta-blocker

This should not actually be a META issue, in the OP we have a concrete plan to implement (as soon as #2226197: Introduce FieldStorageDefinitionInterface in the Entity Field API is done).

plach’s picture

Status:Active» Postponed
jessebeach’s picture

Issue tags:-beta-blocker+beta blocker

beta-blocker tag should be beta blocker

jessebeach’s picture

Issue summary:View changes
plach’s picture

Issue summary:View changes

I will be working on steps #4 and #5 here as soon as #2226197: Introduce FieldStorageDefinitionInterface in the Entity Field API is done.

plach’s picture

Issue summary:View changes
jessebeach’s picture

Title:Decouple entity field storage from configurable fields» [PP-1] Decouple entity field storage from configurable fields
xjm’s picture

Title:[PP-1] Decouple entity field storage from configurable fields» Decouple entity field storage from configurable fields
Status:Postponed» Active

Unpostponed!

fago’s picture

Not sure what's the best way forward here, but I guess best would be to tackle the entity definition renames first. Then, we could focus on this one once #2183231: Make ContentEntityDatabaseStorage generate static database schemas for content entities has been done?

A difficult part here is probably field data purging, i.e. we'd have to make sure all fields of an uninstalled module are properly purged even if they are not configurable.

Berdir’s picture

Status:Active» Needs review
StatusFileSize
new31.27 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,299 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

I'm confused by the entity definition chances, so I started working on switching the interfaces in the various methods and renaming variables.

Here are the things that I found so far that aren't "proper" yet:

- deleted property: Used internally in schema generation methods to check if a field was deleted, if so, uses the uuid
- uuid property and method: Used for deleted fields and hash when the table name would be too long, possibly more. Maybe uniqueStorageIdentifier() ? It needs to be storage specific, see below.
- original property: Used in onFieldUpdate(), I think we should simply change the arguments there and pass in a separate $original_storage_definition, then the caller can figure out how to get the old definition?
- hasData(): Should probably move to the storage controller? Didn't we add a method for that?
- FieldConfigUpdateForbiddenException exception is thrown when columns don't match, that's a field.module exception.
- bundle property in onInstanceDelete()
- FieldDefinition::getField(): Needs the field to pass it to the table name methods, can be dropped when we have a replacement for the uuid, then we can pass the call from the instance to field_config.
- onBundleRename() manually loads field_instance_config entities.
- _fieldColumnName() calls FieldConfig::getReservedColumns() (which only return array('deleted'), not sure if we should move that somewhere else.

Status:Needs review» Needs work

The last submitted patch, 17: update-interfaces-2144263-17.patch, failed testing.

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new40.6 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,658 pass(es), 0 fail(s), and 2 exception(s).
[ View ]
new14.22 KB

Added getUniqueStorageIdentifier(), fixed unit test.

Status:Needs review» Needs work

The last submitted patch, 19: update-interfaces-2144263-19.patch, failed testing.

fago’s picture

so here is a straight re-roll of the patch.

Maybe uniqueStorageIdentifier() ? It needs to be storage specific, see below.

Seems reasonable.

- bundle property in onInstanceDelete()

As discussed on IRC I could see us adding FieldDefinitionInterface::getBundle():
For base fields, it returns NULL
For field instances or bundle fields in general, it returns the bundle - what should make it a convenient addition.
For overridden base fields, it returns the bundle the override is specific to.

Then, I think we want to rename onField*() methods to onFieldStorageDefiniton*() and onInstance*() methods to onFieldDefinition*().

xjm’s picture

@fago, I think the attachment is missing? :)

fago’s picture

Status:Needs work» Needs review
StatusFileSize
new40.64 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,646 pass(es), 0 fail(s), and 4 exception(s).
[ View ]

opsie

Status:Needs review» Needs work

The last submitted patch, 23: d8_entity_decouple.patch, failed testing.

fago’s picture

Status:Needs work» Needs review
StatusFileSize
new2.37 KB
new41.18 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,770 pass(es).
[ View ]

I had a look at the fails, this should fix it. Problem was "deleted" being checked on the wrong definition - so we need to consider this when working on deleted.

I think it should be possible to solve the "deleted" issue by introducing an optional deleted parameter to the table name methods.

fago’s picture

StatusFileSize
new41.56 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,278 pass(es), 132 fail(s), and 12 exception(s).
[ View ]
new4.87 KB

ok, so let's see where and how much this approach fails.

Status:Needs review» Needs work

The last submitted patch, 26: d8_entity_decouple.patch, failed testing.

Berdir’s picture

That doesn't work because the table name needs to use the deleted field table name transparently during the field data purge process. You're just covering the initial rename of the table now but later you actually need to read from it too ;)

fago’s picture

Status:Needs work» Needs review
StatusFileSize
new30.86 KB
new62.33 KB
new406.46 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,848 pass(es).
[ View ]

Actually, the deletion process per se is not so problematic, but what makes it more complicated are entity queries on on already deleted field data. During deletion it happens that we are purging field item data in either the deleted or non-deleted table though, which is rather easily covered by checking for the field storage definition being known to the entity manager.

However - right now, we've got entity query support on deleted field data. That's an undocumented hack which anyone implementing entity query based on the interface would totally miss. Besides, that I think it would be preferable to have entity field storage working (including configurable fields) without a working entity query. Thus, I worked on removing the hack to support entity queries on deleted field data. There were only two places where that was used:
1. field data purging
2. counting the number of entities which have data for a field, which then is only used by the config import batch to show progress on field data purging

I was able to solve 1) by refactoring the interface between field_purge_batch() and entity storage. I did so by moving the query for determining the field data items into the storage as well, what removes the entity query on deleted field data. Incidentally, this refactoring improves the purging process to use a multiple query to fetch all data to delete at once instead of having an additional query per entitiy what should give field purging a performance boost also.

2) was a bit problematic as it was relying on field_purge_batch() calculating batch sizes by entity despite its documentation talking of field data records. I fixed it by introducing a countFieldData() method on the storage controller, which allows for efficient hasData() checks while providing the config import patch counter as needed by the config import batch process. Therefore, I removed FieldConfig::entityCount() which was introduced and used for config import batch processing counting only. Any usages of it that are not for checking hasData() can be replaced by entity queries anyway.

I created the patch based on #2183231: Make ContentEntityDatabaseStorage generate static database schemas for content entities and pushed it to my sandbox into the branch "entity-decouple-2144263-fago" based on plach's branch of #2183231: Make ContentEntityDatabaseStorage generate static database schemas for content entities. Anyone who works on that, please continue working based from that Git branch and post where to find the updated branch to continue working on it. Attached is a complete patch including #2183231: Make ContentEntityDatabaseStorage generate static database schemas for content entities, a patch for this issue only and an interdiff.

Despite that, I think we should build upon the issue #597236: Add entity caching to core as well, as it refactors entity storage for configurable fields quite a bit and is already ready.

fago’s picture

fago’s picture

StatusFileSize
new40.98 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,854 pass(es).
[ View ]
Berdir’s picture

+++ b/core/tests/Drupal/Tests/Core/Entity/ContentEntityDatabaseStorageTest.php
@@ -955,15 +955,17 @@ public function testFieldSqlSchemaForEntityWithStringIdentifier() {
+    // Define a field definition for a test_field fuuidield.

fuuidield :)

Probably even added by myself ;)

michaelfavia’s picture

StatusFileSize
new40.88 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,781 pass(es).
[ View ]

Rerolled with typo fix.

andypost’s picture

StatusFileSize
new12.74 KB
new41.66 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,715 pass(es).
[ View ]

+1 rtbc

andypost’s picture

StatusFileSize
new1.33 KB

real interdiff

fago’s picture

Issue summary:View changes

updated the issue summary based on a call with plach and timplunkett

fago’s picture

StatusFileSize
new9.99 KB
new65.39 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,761 pass(es).
[ View ]

I figured I missed #29 when rerolling the patch, thus integrated that again. Apart from that I worked on adding getBundle() to field definitions and added the $original parameter. Please check the interdiff.

TODO: move FieldConfigUpdateForbiddenException to the component

fago’s picture

StatusFileSize
new19.8 KB
new75.12 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,762 pass(es), 0 fail(s), and 3 exception(s).
[ View ]

Figured some stuff uses FieldConfig or FieldDefinition::getReservedColumns, but fixing that is not really related nor needed for this issue, thus opened #2280073: Remove FieldConfig::getReservedColumns().

Attached interdiff does:
- Move the field update forbid exception to a new variant below the core component + documents that this might be thrown
- fixed bundleRename which still loaded field instance entities

So far everything should be addressed with this patch. So what's left would be just renaming methods, and researching/discussing whether removing onField*() methods from the interface makes sense + of course reviews.

Status:Needs review» Needs work

The last submitted patch, 38: d8_entity_decouple.patch, failed testing.

fago’s picture

Status:Needs work» Needs review
StatusFileSize
new76.67 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,772 pass(es).
[ View ]
new1.85 KB
fago’s picture

Issue summary:View changes
Issue tags:+Needs tests
StatusFileSize
new79.04 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,753 pass(es).
[ View ]
new12.85 KB

Looking at the methods, I think it would be doable to remove onField*() methods from the interface and invoke them internally from onInstance*() methods. For create() and delete() we can figure out when an instance was the first or last one in order to trigger the right storage definition changes. More problematic is update(): I think for update we'd have to trigger onFieldUpdate() foreach onInstanceUpdate() call as well. That means onFieldUpdate() would probably run a bit more often, but it has to check for any changes already anyway. I'd love to get feedback on this before doing that, but I think it's fine to move on without doing this here as we have achieved decoupling without it. Then, we can do it as part of #1498720: [meta] Make the entity storage system handle changes in the entity and field schema definitions if we want to.

Howsoever, attached patch renames existing onField*() and onInstance*() methods to use the terms of the entity field API, what means the patch is complete and imo ready apart from tests. Needs review & feedback!

yched’s picture

Will try to look at the patch tonight.

For now, just hinting that you should probably run the "purge progress" stuff by @elexpott while you have him around :-)
(I chimed in back then, but the "batch purge fields & figure out progress" work was his)

yched’s picture

On that topic, reading @fago #29 :

introduced StorageController::countFieldData(), allows for efficient hasData() checks while providing progress for batch process.
--> removed FieldConfig::entityCount()

IIRC from #2198429: Make deleted fields work with config synch, counting entities rather than field records was precisely what the batch process needed - see #2198429-46: Make deleted fields work with config synch.
Also, counting entities/documents is easy for a MongoStorage, but not sure it will be able to count "field values across entities".

fago’s picture

oh, I see. Thanks I'll reach out to him.

IIRC from #2198429: Make deleted fields work with config synch, counting entities rather than field records was precisely what the batch process needed - see #2198429-46: Make deleted fields work with config synch.

Not any more - with the change of the purging process it moves to count field data items. That was I thought field_purge_batch() is counting by reading its documentation though anyway.

Also, counting entities/documents is easy for a MongoStorage, but not sure it will be able to count "field values across entities".

I'd be surprised if it would not, but yeah, let's check this.

fago’s picture

Component:field system» entity system
Issue tags:-Needs tests
StatusFileSize
new11.94 KB
new88.62 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,237 pass(es), 1,113 fail(s), and 41 exception(s).
[ View ]

Here is an updated patch including a small test module that implements a custom field as bundle field.

A problem doing so was that we do not have a FieldStorageDefinition class, i.e. a class that implements FieldStorageDefinition interface only. Opened #2280639: Add a FieldStorageDefinition class for that and just went with a FieldDefinition class for now; although that one incorrectly implements FieldDefinintionInterface also (but that does not hurt).

The test module perfectly shows that with just decoupling the storage the DX for adding a separate field sucks a lot and is quite complicated to do. For having the field disappear during hook_uninstall() where the module is still enabled, I even had to introduce a function keeping the "is-uninstalling" state, ouch.

Finally, the module shows that we've got the problem of purging data during module uninstall what is not run as batch.

We might have to keep track of deleted entity fields in state even for being able to properly fix that all, but that's something we should take care of in #1498720: [meta] Make the entity storage system handle changes in the entity and field schema definitions I think. While the test module is not nice as is, it proofs that the storage class works decoupled from configurable fields now.

Status:Needs review» Needs work

The last submitted patch, 45: d8_entity_decouple.patch, failed testing.

fago’s picture

Status:Needs work» Needs review
StatusFileSize
new89.81 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,891 pass(es).
[ View ]
new2.25 KB

hm, we should not try to store computed fields. Fixed that.

yched’s picture

haven't processed the last comments yet but re: @fago #44 :

Basically, this boils down to : the $batch_size param of field_purge_batch() and the "count stuff" method in the ContentStorage class work together, and need to be in the same "units" - either they are both "number of entities" or both "number of field records (including multiple values)".

The former ("number of entities") is what curent HEAD does, because,
a) it works :-) (it's what we do since D7)
b) as was discussed in #2198429: Make deleted fields work with config synch, it's very likely to be easier for mongo/document storages.

So, not sure why we'd want/need to move to counting "number of field records" instead ?

yched’s picture

To state it more clearly :

I haven't looked at the field_purge_batch() / "count stuff" refactors in detail yet, but from the descriptions posted in the comments they look great in terms of "moving code around / who's in charge of what".

Changing the internal logic (count entities or count field records), OTOH, doesn't seem required, and isn't really within the scope of this issue here ?

yched’s picture

re #45 :

Finally, the module shows that we've got the problem of purging data during module uninstall what is not run as batch

Well, an unfriendly consequence of "fields defined in code can come and go" is that the problem of "field deletion and field purge" is extended to non-config fields.

So :
- when a module that defines a field is uninstalled, we need to persist the field definition in the "state() registry of fields that are pending deletion"
- and similarly whenever a code-defined-field is suddenly not present after a code update (e.g removed from EntityType;;baseFieldDefinitions() in a point release of a module)

fago’s picture

Changing the internal logic (count entities or count field records), OTOH, doesn't seem required, and isn't really within the scope of this issue here ?

Initially, I thought it's required for being able to refactor it without having to introduce another count in the storage. Turns out, we've do the count in the storage anyway, however the refactoring to data records allowed me to reduce the number of read-field-items query to 1 instead of having a count+1 per entity query. The purge query should be optimizable to a single query as well, I've not done that yet though.

tldr, it's not strictly necessary anymore and we could redo it, but the approach implemented by the patch is a bit more efficient, at least for sql.

fago’s picture

StatusFileSize
new2.25 KB
new90.14 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,924 pass(es).
[ View ]

Figured reserved columns are not needed any more once this lands, see #2280073: Remove FieldConfig::getReservedColumns().

I talked to chx about counting entities vs counting field items and he was suggesting to keep it internal to the storage, i.e. not define what it counts at all. As we only need the numbers for being able to show the batch progress that's totally fine imo, plus it allows each storage to do whatever implementation works well for fit. Therefore, I'd suggest to follow that suggestion and add the respective docu improvments to clarify that. That way we can keep the improvements done here already, what is less work at this point than reverting to the old, less efficient approach and decoupling it from configurable fields.

fago’s picture

Issue summary:View changes

updated the issue summary to include API changes.

fago’s picture

Assigned:Unassigned» yched

Assigning to yched for his review :)

yched’s picture

I talked to chx about counting entities vs counting field items and he was suggesting to keep it internal to the storage, i.e. not define what it counts at all. As we only need the numbers for being able to show the batch progress that's totally fine imo, plus it allows each storage to do whatever implementation works well for fit

I don't see how this works. That number is not just an arbitrary amount used to show progress, the batch purge does calculations based on that count to determine the number of batch steps (i.e calls to field_purge_batch()) that will be needed - see ConfigImporterFieldPurger::initializeSandbox(). So it can't be "anything the storage feels like counting", it has to be in the same unit than the $size argument of field_purge_batch() ?

Still think this would be best in a separate issue :-)

fago’s picture

I don't see how this works. That number is not just an arbitrary amount used to show progress, the batch purge does calculations based on that count to determine the number of batch steps (i.e calls to field_purge_batch()) that will be needed - see ConfigImporterFieldPurger::initializeSandbox(). So it can't be "anything the storage feels like counting", it has to be in the same unit than the $size argument of field_purge_batch() ?

It does + it documents that it has to be that way now.

Still think this would be best in a separate issue :-)

If the approach suggested does not work, yep fine - but if I'd rather keep existing work instead of putting more work into throwing it away just to bring it back in another issue later on.

xjm’s picture

@fago, I also would agree with moving it into a separate issue. It needn't be "thrown away" -- we can separate out those parts of the patch into a postponed issue now where possible.

It's important because it's a point that needs further discussion, and this patch is significant and blocking further work (and the beta). So the right thing to do is to reduce the scope here and then discuss that part separately.

fago’s picture

StatusFileSize
new90.73 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,951 pass(es).
[ View ]
new7.44 KB

It seemed that people are on board with the new approach, but ok - so here we go. -> Back to counting by entity id and read query per entity id. I've left documentation clarifications in place but updated it to clarify that is by entity id.

fago’s picture

yched’s picture

Still reviewing, sorry for the delay.

Getting rid of the nitpicky remarks first :

  1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityDatabaseStorage.php
    @@ -1132,10 +1128,25 @@ protected function doDeleteFieldItemsRevision(EntityInterface $entity) {
    +  protected function usesDedicatedTable(FieldStorageDefinitionInterface $definition) {
    +    // Everything that is not provided by the entity type is stored in a
    +    // dedicated table.
    +    return $definition->getProvider() != $this->entityType->getProvider() && !$definition->hasCustomStorage();

    Probably good enough for now, but won't work for e.g user.roles ? Or for base fields that are multiple, more generally ?

    I guess we'll get back to it when we get there ?

  2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityDatabaseStorage.php
    @@ -1132,10 +1128,25 @@ protected function doDeleteFieldItemsRevision(EntityInterface $entity) {
    +  protected function usesDedicatedTable(FieldStorageDefinitionInterface $definition) {

    IMO delivering this information is the role of the TableMapping rather than of ContentEntityDatabaseStorage.
    I'm fine with deferring this as part of the refactor in #2274017: Make SqlContentEntityStorage table mapping agnostic , though.

  3. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
    @@ -391,6 +391,7 @@ protected function buildBaseFieldDefinitions($entity_type_id) {
           if ($base_field_definition instanceof FieldDefinition) {
             $base_field_definition->setName($field_name);
             $base_field_definition->setTargetEntityTypeId($entity_type_id);
    +        $base_field_definition->setBundle(NULL);

    @@ -488,6 +489,7 @@ protected function buildBundleFieldDefinitions($entity_type_id, $bundle, array $
           if ($field_definition instanceof FieldDefinition) {
             $field_definition->setName($field_name);
             $field_definition->setTargetEntityTypeId($entity_type_id);
    +        $field_definition->setBundle($bundle);

    In both cases, the comment above the if() is out of date.

  4. +++ b/core/lib/Drupal/Core/Entity/Exception/StorageDefinitionUpdateForbiddenException.php
    @@ -0,0 +1,13 @@
    +class StorageDefinitionUpdateForbiddenException extends \Exception { }

    Albeit long, should be FieldStorageDefinitionUpdateForbiddenException :-/, we have no such thing as a StorageDefinition.

  5. +++ b/core/lib/Drupal/Core/Entity/FieldableEntityStorageInterface.php
    @@ -7,71 +7,78 @@
    +   * Reacts to the creation of field storage definition.

    "of a"

  6. +++ b/core/lib/Drupal/Core/Field/FieldDefinitionInterface.php
    @@ -55,6 +55,15 @@
    +   *   The bundle the field is defined for, or NULL if it is base field; i.e.,

    "or NULL if it is base field" misses an article.

  7. +++ b/core/modules/field/src/Entity/FieldInstanceConfig.php
    @@ -847,4 +854,11 @@ public static function loadByName($entity_type_id, $bundle, $field_name) {
    +  public function getUniqueStorageIdentifier() {
    +    return $this->field->uuid();

    Should be $this->getField()->uuid(); now - or, better, $this->getField()->getUniqueStorageIdentifier();

  8. +++ b/core/modules/system/src/Tests/Entity/EntityBundleFieldTest.php
    @@ -0,0 +1,102 @@
    +  public function testCustomBundleFieldCreateDelete() {

    Code structure - the empty line seems randomly placed and doesn't really reflect the structure of the test (do A and check it has effect X, then do B and check it has effect Y)

  9. +++ b/core/modules/system/src/Tests/Entity/EntityBundleFieldTest.php
    @@ -0,0 +1,102 @@
    +  public function testCustomBundleFieldUsage() {

    Similarly, could use a couple inline comments ?

  10. +++ b/core/modules/system/tests/modules/entity_bundle_field_test/entity_bundle_field_test.info.yml
    @@ -0,0 +1,8 @@
    \ No newline at end of file

    No good :-)

  11. +++ b/core/tests/Drupal/Tests/Core/Entity/ContentEntityDatabaseStorageTest.php
    @@ -955,15 +955,17 @@ public function testFieldSqlSchemaForEntityWithStringIdentifier() {
    +    // Define a field definition for a test_field fuuidield.

    That must hurt ;-)

yched’s picture

More meaty stuff - still didn't go into the purge part yet, though.

  1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityDatabaseStorage.php
    @@ -953,8 +951,8 @@ protected function doLoadFieldItems($entities, $age) {
           foreach ($this->entityManager->getFieldDefinitions($this->entityTypeId, $bundle) as $field_name => $instance) {
    -        if ($instance instanceof FieldInstanceConfigInterface) {
    -          $fields[$field_name] = $instance->getField();
    +        if ($this->usesDedicatedTable($instance)) {
    +          $fields[$field_name] = $instance;
             }

    So yeah, var names :-)

    Here we have $instances that are placed in a $fields array and are then used as foreach (... as $field) :-p.

    The usesDedicatedTable() method was added to handle "fields that are stored in per-field table", whether they are configuration or code fields.
    This means that the code in ContentEntityDatabaseStorage now contains no special case on FieldinstanceConfig (which is great), and there is no reason whatsoever to name those variables $instance.

    Code like :
    foreach (EM->getFieldDefinitions($entity_type, $bundle) as $field_name => **$instance**) {
    is pure misnaming now that we don't have "instanceof" checks that de facto restrict the results of getFieldDefinitions() to FieldInstanceConfig objects.

    --> Drop $instance in this file, go with $field_definition all the way ? (which in itself is full of yay if you ask me)

  2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityDatabaseStorage.php
    @@ -1045,13 +1042,13 @@ protected function doSaveFieldItems(EntityInterface $entity, $update) {
    +      foreach ($instance->getColumns() as $column => $attributes) {
    +        $columns[] = static::_fieldColumnName($instance, $column);

    @@ -1097,12 +1094,11 @@ protected function doSaveFieldItems(EntityInterface $entity, $update) {
    +      $table_name = static::_fieldTableName($instance);
    +      $revision_name = static::_fieldRevisionTableName($instance);

    So, fine for now (after s/$instance/$field_definition/ ?), but, as discussed the other day, we really should lose "FieldDefinitionInterface extends FieldStorageDefinitioninterface" at some point...

  3. +++ b/core/lib/Drupal/Core/Entity/ContentEntityDatabaseStorage.php
    @@ -1045,13 +1042,13 @@ protected function doSaveFieldItems(EntityInterface $entity, $update) {
    -      $langcodes = $field->isTranslatable() ? $translation_langcodes : array($default_langcode);
    +      $langcodes = $instance->isTranslatable() ? $translation_langcodes : array($default_langcode);

    So in HEAD this code was using FieldConfig::isTranslatable(), now it's using FieldInstanceConfig::isTranslatable() - but as we know, the two are not interchangeable ?

  4. +++ b/core/lib/Drupal/Core/Entity/ContentEntityDatabaseStorage.php
    @@ -1231,60 +1240,57 @@ public function onFieldUpdate(FieldConfigInterface $field) {
    +    // Make sure any deleted field definitions disappear.
    +    $this->entityManager->clearCachedFieldDefinitions();

    We sure should clear EM's cache when a field storage definition is removed, but it doesn't feel like the responsibility of ContentEntityDatabaseStorage ?

    Or why would we add it in onFieldStorageDefinitionDelete() and not in onFieldDefinitionDelete() ? Who clears the cache on FieldDefinition deletion currently ?

  5. +++ b/core/lib/Drupal/Core/Entity/ContentEntityDatabaseStorage.php
    @@ -1231,60 +1240,57 @@ public function onFieldUpdate(FieldConfigInterface $field) {
    -  public function onBundleRename($bundle, $bundle_new) {
    ...
    +  public function onFieldDefinitionUpdate(FieldDefinitionInterface $field_definition, FieldDefinitionInterface $original) {

    So the housekeeping we did in onBundleRename() is now done in onFieldDefinitionUpdate() ?
    Why ? FieldableEntityStorageInterface still has onBundle[CRUD]() methods, why not keep doing this in onBundleRename() ?

    This happens to work because a bundle rename triggers a FieldinstanceConfig re-save() - which is incidental, and is something I still don't really agree with :-/ (in D7 we updated the field_instance_config.bundle db column directly without going through the API. It's just internal housekeeping we're doing, we shouldn't be notifying others IMO, they can react to the bundle rename itself).

    Also - code fields have no such housekeeping to do on bundle rename, so who will call onFieldDefinitionUpdate() for code fields in reaction to a bundle being renamed ?

    In short : IMO we really want to react to a bundle being renamed, not to the fact that a FieldConfig did some housekeeping in reaction to a bundle being renamed. React to the event, not to the fact that someone else reacted to the event ?

  6. +++ b/core/modules/field/field.module
    @@ -211,20 +211,17 @@ function field_entity_bundle_field_info(EntityTypeInterface $entity_type, $bundl
    -  $instances = entity_load_multiple_by_properties('field_instance_config', array('entity_type' => $entity_type, 'bundle' => $bundle_old));
    +  $instances = entity_load_multiple_by_properties('field_instance_config', array('entity_type' => $entity_type, 'bundle' => $bundle_old, 'include_deleted' => TRUE));

    This now updates deleted instances as well (which is a right thing to do), but we ->save() them all the same ? This would put them back in the config store, we don't want that.

    At some point I was considering that maybe we should have FieldConfigStorage/FieldinstanceConfigStorage::save() take care of saving in either config or the "state() list of deleted fields". Since we're talking about having that "state() list" encompass code fields as well, I'm not sure it's still relevant.

    But at any rate, $deleted_instance->save() currently doesn't put stuff into state().

  7. +++ b/core/modules/field/field.module
    @@ -211,20 +211,17 @@ function field_entity_bundle_field_info(EntityTypeInterface $entity_type, $bundl
    -    if ($instance->entity_type == $entity_type && $instance->bundle == $bundle_old) {
    -      $id_new = $instance->entity_type . '.' . $bundle_new . '.' . $instance->field_name;
    -      $instance->set('id', $id_new);
    -      $instance->bundle = $bundle_new;
    -      $instance->allowBundleRename();
    -      $instance->save();
    -    }
    +    $id_new = $instance->entity_type . '.' . $bundle_new . '.' . $instance->field_name;
    +    $instance->set('id', $id_new);
    +    $instance->bundle = $bundle_new;
    +    $instance->allowBundleRename();
    +    $instance->save();

    Why remove the if ? At least the check on $entity_type is required, otherwise we'll change bundle names in field instances for other, unrelated entity types.

yched’s picture

Finally got around the purge stuff - awesome work :-D

A couple (non major) remarks :

  1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityDatabaseStorage.php
    @@ -1292,54 +1298,139 @@ public function onBundleRename($bundle, $bundle_new) {
    +    $query = $this->database->select($table_name, 't', array('fetch' => \PDO::FETCH_ASSOC));
    ...
    +    foreach ($query->execute() as $row) {
    +      $query = $this->database->select($table_name, 't', array('fetch' => \PDO::FETCH_ASSOC))

    Nitpick: for clarity, could we name those nested queries differently ? $entity_query and $item_query ?

    Also, it looks like we should be able to do "one query to fetch entity ids" + "one (single) query to load items across all the entity ids" ?
    Well, looks like you have a plan to reduce that down to one single query in #2282527: Allow storages to implement more efficient field purging anyway, so let's leave it that way here :-)

  2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityDatabaseStorage.php
    @@ -1292,54 +1298,139 @@ public function onBundleRename($bundle, $bundle_new) {
    +    $or = $query->orConditionGroup();
    +    foreach ($field_definition->getColumns() as $column_name => $data) {
    +      $or->isNotNull(static::_fieldColumnName($field_definition, $column_name));
    +    }

    That $or group could use a word of comment. If I get things right, it's needed because fields stored in the base table can have "all NULL" values for rows about bundles where the field does not actually exist ?
    Also, we could possibly avoid it and have a more efficient query on fields stored in "per field tables" ?

    Same applies to countFieldData() - but actually, it looks like both methods use rougly the same query - which is not a coincidence, since they are respectiveley "just count" and "actually fetch". Since those two queries should be kept in sync on a possible later bugfix/refactor, maybe building the query could be mutualized in a helper method ?

  3. +++ b/core/lib/Drupal/Core/Entity/ContentEntityDatabaseStorage.php
    @@ -1292,54 +1298,139 @@ public function onBundleRename($bundle, $bundle_new) {
    +    $revision_id = $entity->getRevisionId() !== NULL ? $entity->getRevisionId() : $entity->id();

    Isn't there a more straightforward way to check whether an entity type is revisionable ?
    Check RevisionableInterface ?

  4. +++ b/core/modules/field/src/Tests/BulkDeleteTest.php
    @@ -308,7 +308,7 @@ function testPurgeField() {
    -    field_purge_batch(0);
    +    field_purge_batch(1);

    @@ -340,7 +340,7 @@ function testPurgeField() {
    -    field_purge_batch(0);
    +    field_purge_batch(1);

    Oh ? Why ?
    Is that still relevant even if we don't change the internal counting logic of field_purge_batch() in this issue ?

  5. +++ b/core/modules/system/tests/modules/entity_bundle_field_test/entity_bundle_field_test.module
    @@ -0,0 +1,66 @@
    +function entity_bundle_field_test_entity_bundle_delete($entity_type, $bundle) {
    +  if ($entity_type->id() == 'entity_test' && $bundle == 'custom') {
    +    // Notify the entity storage that our field is gone.
    +    \Drupal::entityManager()->getStorage('entity_test')
    +      >onFieldDefinitionDelete(FieldDefinition::create('string')
    +      ->setName('custom_field')
    +      ->setLabel(t('A custom field')));
    +  }
    +}

    - Looks like this isn't actually called, there's a syntax error with the onFieldDefinitionDelete() call (s/>/->/) ;-)

    - Indentation error for the "setName() / setLabel()" lines,
    and the onFieldDefinitionDelete() closing parenthesis should be on its own line

    - Feels a bit weird calling onFieldDefinitionDelete() with an on-the-fly FieldDefinition, but I guess that's the "sucky DX" you mention in #45 ?

plach’s picture

@61.2: This is already happening in #2143291: Clarify handling of field translatability (or at least a start, more properties could be duplicated in the two interfaces if needed), reviews welcome ;)
@61.3: I think currently instance just reads the field value, which is ok in HEAD. That line is being fixed in #2143291: Clarify handling of field translatability, reviews welcome ;)

yched’s picture

Just occurred to me :

adding FieldDefinitionInterface::getBundle() means we should remove FieldInstanceConfigInterface/FieldInstanceConfig::getTargetBundle(), they are duplicate...

Berdir’s picture

StatusFileSize
new89.5 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,575 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
new25.77 KB

Re-roll and addressing what I can from the review...

#60
1. user roles currently have custom storage right now, but yeah, it would be awesome if this would just work, I've already seen use cases for this in contrib module ports, but doesn't need to be fixed here I guess?
3. Fixed I think.
4. Renamed class.
5. Fixed.
6. Fixed I think.
7. Fixed.
8. Better now?
9. Added some comments and assert messages.
10. Fixed.
11. Fixed.

#61
1. $instance is gone.
3. No idea what to do, will the clarify field translation issue help?
4. Not sure, not touching this yet. configurable fields currently manually clear the field definition cache.
5. Not done yet.
6. Updated. Do we have a test for this?
7. I don't get this, we have the entity_type and bundle in the load by properties call, why do we need to repeat it?

#62:
1. So just do the rename? Done.
2. Too deep for now, will re-visit later.
3. All content entities implement that interface, it means they could have revisions. $entity_type->getKey('revision') could work, but not changed yet.
4. Me no clue.
5. Yeah, that function was full of fail :) Fixed a bunch of things, also moved the definition to a separate variable, is a bit easier to understand I think. However, who is in charge of doing the purging after this? Added a test but obviously, the table isn't gone yet and it will not ever be gone unless I'm missing something? (I did not look at the rest of the patch yet, just implemented the review.)

#63: Yeah, thought so, so #61 2. and 3. can be ignored here I think?

Left: #61: 4., 5., #62. 2. 3. partially 5. (review is addressed, but my new test will fail)

Status:Needs review» Needs work

The last submitted patch, 65: d8_entity_decouple-2144263-65.patch, failed testing.

yched’s picture

Thanks @Berdir !

#61 7. [if() removal in field_entity_bundle_rename()] - I don't get this, we have the entity_type and bundle in the load by properties call, why do we need to repeat it?

Right, my bad, read the code too fast.

Left: #61: 4., 5., #62. 2. 3. partially 5. (review is addressed, but my new test will fail)

And #62. 4. :-)

Berdir’s picture

Oh, yes 4. too.

@yched. If you have time later today/tomorrow and any clue how the purging is now supposed to work, would be great if you could join IRC and help me out :) Going for lunch now soon, though.

alexpott’s picture

The config import all errors are caused by the active filter.format.plain_text.yml having this. Staging does not.

<   editor_file_reference:
<     id: editor_file_reference
<     provider: editor
<     status: false
<     weight: 0
<     settings: {  }
alexpott’s picture

Status:Needs work» Needs review
StatusFileSize
new3.48 KB
new96.63 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,579 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

So this will fix it but this is a sign of a much deeper problem. I think the fix in the attached patch is not really acceptable.

The problem

  • Editor has a filter plugin.
  • Filter has a default config entity filter.format.plain_text.
  • Editor depends on Filter so the default config entity can not contain configuration for the editor_file_reference plugin.
  • The standard profile enables both filter and editor so it can provide its own filter.format.plain_text default but this is used during the filter install so the editor module is not yet installed so the dependencies are calculated incorrectly.

Why this patch exposes this

I think this patch might be making modules uninstallable that previously were not. However I'm not certain (at all).

Fixes

Either: if a profile is overriding a config entity instead of replacing it will the config entity from the standard profile just don't create when installing filter and then it'll be picked up when the standard profile is installed.

Or: delay all configuration entity creation until the modules have been installed (in the same way configuration sync does).

Status:Needs review» Needs work

The last submitted patch, 70: 2144263.70.patch, failed testing.

yched’s picture

@Berdir #68 : I think @fago & @plach were not planning on this patch adding support for purging of code-defined fields.

That's on the table, but not for this patch here. Will require extending the notion of the "state() store of deleted definitions", and a rework of the whole purge logic out of field.module. Thus, better in a separate issue IMO (not sure that issue was created though)

Also : I mostly off til the end of the week :-)

Berdir’s picture

I see, would have to be at least critical to other, as it would otherwise be impossible to get rid of the data?

Means I will need to update the test to instead create a entity so we have something to test with and assert for deleted rows instead.

Berdir’s picture

Assigned:yched» Berdir

Working on a re-roll.

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new96.53 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,679 pass(es).
[ View ]
new13.01 KB

Re-roll, that was fun ;)

"interdiff" are the changes that I had to make manually after mostly preferring this patch over the changes in HEAD, going back to explicitly getting the storage definition but updated naming and with the new generic field names. Field API and the new test seems to be passing, so let's try this with testbot.

Also updated the new test based on what @yched said.

plach’s picture

I just had some fun realigning the sandbox with the latest patch. Since I'm basing my work in #1498720: [meta] Make the entity storage system handle changes in the entity and field schema definitions on fago's entity-decouple branch, anyone working on this please use the sandbox.

I just pushed my code to the entity-decouple-plach branch in case anyone needed it.

plach’s picture

Issue summary:View changes
plach’s picture

Issue summary:View changes
xjm’s picture

Sorry, I think I gave @alexpott the link to the wrong sandbox; I thought it was in the same sandbox as the field translatability patch.

The entity-decouple-plach branch differs from #75; aside from HEAD changes, it has the following minor cleanup in ContentEntityDatabaseStorage that the patch does not:

--- a/core/lib/Drupal/Core/Entity/ContentEntityDatabaseStorage.php
+++ b/core/lib/Drupal/Core/Entity/ContentEntityDatabaseStorage.php
@@ -10,6 +10,7 @@
use Drupal\Component\Utility\String;
use Drupal\Core\Database\Connection;
use Drupal\Core\Database\Database;
+use Drupal\Core\Entity\Exception\FieldStorageDefinitionUpdateForbiddenException;
use Drupal\Core\Entity\Query\QueryInterface;
use Drupal\Core\Entity\Schema\ContentEntitySchemaHandler;
use Drupal\Core\Entity\Sql\DefaultTableMapping;
@@ -17,7 +18,6 @@
use Drupal\Core\Field\FieldDefinitionInterface;
use Drupal\Core\Field\FieldStorageDefinitionInterface;
use Drupal\Core\Language\LanguageInterface;
-use Drupal\Core\Entity\Exception\FieldStorageDefinitionUpdateForbiddenException;
use Drupal\field\Entity\FieldConfig;
use Symfony\Component\DependencyInjection\ContainerInterface;

@@ -958,8 +958,9 @@ protected function doLoadFieldItems($entities, $age) {
     foreach ($bundles as $bundle => $v) {
       $definitions[$bundle] = $this->entityManager->getFieldDefinitions($this->entityTypeId, $bundle);
       foreach ($definitions[$bundle] as $field_name => $field_definition) {
-        if ($this->usesDedicatedTable($field_definition->getFieldStorageDefinition())) {
-          $storage_definitions[$field_name] = $field_definition->getFieldStorageDefinition();
+        $storage_definition = $field_definition->getFieldStorageDefinition();
+        if ($this->usesDedicatedTable($storage_definition)) {
+          $storage_definitions[$field_name] = $storage_definition;
         }
       }
     }

Regarding #70:

The standard profile enables both filter and editor so it can provide its own filter.format.plain_text default but this is used during the filter install so the editor module is not yet installed so the dependencies are calculated incorrectly.

I don't understand how this happens. Shouldn't standard profile (and therefore its default config entities) be installed last, after both filter and editor are enabled?

Edit: Oh. Is Standard overriding a config entity of the same name?

plach’s picture

@fago opened a dedicated issue to unify field data purging.

plach’s picture

@Berdir:

Is there anything left you wish to work on or should we assign this back to @yched for a final review?

Berdir’s picture

Assigned:Berdir» Unassigned

Based on #65/67, there are still a few things left to do:

Left: #61: 4., 5., #62. 2. 3. and 4

(#62.5 was resolved, as the purging is not happening here and the test was updated)

I will try to look at those, maybe you know more?

=> Unassigning, but not assigning to @yched.

plach’s picture

I don't know much about them too, but I will try have a look tonight if you don't beat me to them :)

plach’s picture

StatusFileSize
new5.09 KB
new96.25 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,777 pass(es), 1 fail(s), and 4 exception(s).
[ View ]

@yched:

#61.4: Fixed. I think the storage class is a good place where centralizing field definition cache clears, otherwise we'd need an explicit call everywhere onField*Delete() methods are called. Ideally I'd see the EM as the responsible for refreshing its own data, but I guess we'd need to add similar onField*() methods on it and make it forward the calls to the storage. But I think this would be a bit out of scope here.
#61.5: Didn't touch this: your objections make total sense to me, but I've no idea why this change happened, so I'll this for @fago to answer.
#62.2: Actually this code is not dealing with base fields yet, thus I cannot explain that or condition. Also this one is for @fago. The rest of the proposed changes make sense to me but I'd address them in #2282119: Make the Entity Field API handle field purging.
#62.3: Fixed.
#62.4: Fixed.

Status:Needs review» Needs work

The last submitted patch, 84: d8_entity_decouple-2144263-84.patch, failed testing.

Berdir’s picture

Confused by entity_bundle_field_test_uninstall() a bit, that does seem to do the field data purging was moved to a different issue? And why is this now called suddenly? This is conflicting with my test which removed that bundle to test the bundle removal hook...

yched’s picture

Thanks !

@plach

#61.4: Fixed. I think the storage class is a good place where centralizing field definition cache clears, otherwise we'd need an explicit call everywhere onField*Delete() methods are called

I disagree. ContentEntityDatabaseStorage is merely the *SQL* storage backend for fields data. Its onField*Delete() method is there to react to some definition being deleted and perform the relevant storage housekeeping, but it shouldn't be in charge of maintaining the upstream EM cache of field definitions. Wrong placement of responsibility, and would mean that any alternate storage backend (Mongo) should also not forget to do the same.

Ideally I'd see the EM as the responsible for refreshing its own data, but I guess we'd need to add similar onField*() methods on it and make it forward the calls to the storage

Agreed, and agreed that it's out of scope here.

Meanwhile, I think the current HEAD code where FieldConfig and FieldInstanceConfig are the ones to clear EM's cache when they deem relevant is correct and should be kept :
- Note that FC does it in its postDelete(), while FIC does it in preDelete(). I don't remember the gory details exactly, but there was a nasty reason in #2020895: Move save() / delete() logic in Field / FieldInstance to [pre|post]Save(), [pre|post]Delete() :-).
- On creation / update, it's FC / FIC that refresh the cache in their postSave(). We should stay consistent with that for deletion.

So I'd suggest keeping the clearCachedFieldDefinitions() in FC / FIC and remove them in CEDS::onField*Delete(). We don't "delete" code fields currently, so let's see when we get there ?

yched’s picture

StatusFileSize
new101.98 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,796 pass(es), 1 fail(s), and 4 exception(s).
[ View ]
new6.77 KB

Also, regarding #62.4 and field_purge_batch(0) :

(from the interdiff in #84)

+++ b/core/modules/field/field.purge.inc
@@ -92,7 +92,7 @@ function field_purge_batch($batch_size, $field_uuid = NULL) {
-    if ($count_purged < $batch_size) {
+    if ($count_purged < $batch_size || $batch_size <= 0) {
       // No field data remains for the instance, so we can remove it.
       field_purge_instance($instance);

Oh, ok, I overlooked that field_purge_batch() now doesn't necessarily need an extra call to actually ditch the field and instance definitions, but does it straight ahead if the number of deleted rows indicated there is no data left.

Then:
- The fix here makes sense, but I don't think "$batch_size <= 0" is right - it would mean that calling field_purge_batch(0) unconditionally ditches the instance straight ahead without purging any data, I don't think we want to allow that.
Checking for $count_purged == 0 would be safer.
- There are a couple of "call field_purge_batch() again to remove the field and instance" that are no longer needed.

Attached patch does that.

- + Expands the tests in BulkDeleteTest to more explicitly track that behavior
- Doing so, fixed FieldInstanceConfigStorage::loadByProperties() to comply to EntityStorageInterface::loadByProperties() (results should be keyed by ID)

(pushed the changes to the 'entity-decouple-yched' branch in the sandbox)

martin107’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 88: d8_entity_decouple-2144263-88.patch, failed testing.

plach’s picture

Assigned:Unassigned» plach

I disagree. [...] Wrong placement of responsibility, and would mean that any alternate storage backend (Mongo) should also not forget to do the same.

I think the storage class is the best place to do that now: atm we need to remember about clearing caches not only when switching storage but also when dealing with every possible field flavor (base, configurable or whatever else contrib may provide). Anyway, I'm glad we agree on the way forward, so I will happily revert this change :)

Actually I think the EM might trigger an event and the storage could be just a subscriber, instead of having calls manually proxied by the EM itself.

plach’s picture

Assigned:plach» fago
Status:Needs work» Needs review
StatusFileSize
new2.7 KB
new101.46 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,789 pass(es).
[ View ]

Ok, reverted #61.4. This should be green again. Assigning to @fago to address the last two items of @yched's review:

#61.5: Didn't touch this: your objections make total sense to me, but I've no idea why this change happened, so I'll this for @fago to answer.
#62.2: Actually this code is not dealing with base fields yet, thus I cannot explain that or condition. Also this one is for @fago. The rest of the proposed changes make sense to me but I'd address them in #2282119: Make the Entity Field API handle field purging.

plach’s picture

StatusFileSize
new101.46 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,792 pass(es).
[ View ]
+++ b/core/vendor/symfony/http-foundation/Symfony/Component/HttpFoundation/Session/Storage/Proxy/AbstractProxy.php
@@ -120,7 +120,7 @@ public function getId()
+//            throw new \LogicException('Cannot change the ID of an active session');

Removed this debugging leftover. Latest code is in entity-decouple-plach.

yched’s picture

StatusFileSize
new101.42 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,805 pass(es), 0 fail(s), and 1 exception(s).
[ View ]
new780 bytes

@plach : thanks !

Patch still adds a clearCachedFieldDefinitions() call within onFieldStorageDefinitionDelete(), though - which was what I originally reacted to.
Just checking what happens if we remove it :-)

Status:Needs review» Needs work

The last submitted patch, 94: d8_entity_decouple-2144263-94.patch, failed testing.

yched’s picture

Status:Needs work» Needs review
StatusFileSize
new161.89 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch d8_entity_decouple-2144263-96.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new1.94 KB

So this triggers a test fail in EntityBundleFieldTest, which is related to the entity_bundle_field_test_uninstall() logic added in the patch to demonstrate the current state of the DX for altered-in code fields.

When I try to run it locally, that test stops at "LogicException: Cannot change the ID of an active session in Symfony\Component\HttpFoundation\Session\Storage\Proxy\AbstractProxy->setId()", not sure why.

But this should work - if the hook_uninstall() has to manually fake the field deletion by manually calling $storage->onField*Delete(), then it feels right that it also needs to clear the field definition cache. As argued in #61.4, it shouldn't be the role of the storage to trigger a clear of the upstream EM field definition cache.

interdiff is with #93

Status:Needs review» Needs work

The last submitted patch, 96: d8_entity_decouple-2144263-96.patch, failed testing.

Berdir’s picture

The LogicException has nothing to do with this issue, the UI test runner and session handling is completely messed up right now, try running it with run-tests.sh instead.

See #86, the problem is that I extended the test and manually deleted the custom bundle to be able to test the hook that deleted the field. Then uninstall runs and at that point, the bundle doesn't exist anymore and it fails.

We can't do both, two separate test methods maybe?

plach’s picture

When I try to run it locally, that test stops at "LogicException: Cannot change the ID of an active session in Symfony\Component\HttpFoundation\Session\Storage\Proxy\AbstractProxy->setId()", not sure why.

This is happening when running DUTB tests from the UI, I just comment line 123 of AbstractProxy before launching tests (see #93).

plach’s picture

StatusFileSize
new101.47 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,970 pass(es).
[ View ]

Rerolled #96

plach’s picture

Status:Needs work» Needs review
yched’s picture

@Berdir

See #86, the problem is that I extended the test and manually deleted the custom bundle to be able to test the hook that deleted the field. Then uninstall runs and at that point, the bundle doesn't exist anymore and it fails.

Not sure that's related, the fail is in testCustomBundleFieldCreateDelete(), and you extended testCustomBundleFieldUsage(), right ?

Let's see what the bot says on the rerolled patch in #100.

yched’s picture

StatusFileSize
new292.81 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch d8_entity_decouple-2144263-103.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new4.55 KB

Green, cool :-)

Attached patch attempts to tackle #61.5.

I think I get why @fago moved handling of bundle rename to onFieldDefinitionUpdate() : because it needs to run on deleted fields too, and for now there's no generic way to get "deleted fields", only "deleted configurable fields".
So @fago's patch :
- updated field_entity_bundle_rename() to run on deleted fields too, updating their bundle with ->save()
- moved the content of Storage::onBundleRename() to ::onFieldDefinitionUpdate(), which is triggered by the above

But :
- as argued in #61.5, that's the wrong logic IMO, we want Storage to react to "a bundle was renamed", not to "a field was updated in reaction to a bundle being renamed"
- that doesn't work anyway because, as pointed in #61.6 and fixed since then, you don't ->save() deleted fields

Updated patch :
- reintroduces onBundleRename(), with specific logic to process "deleted configurable fields", and a @todo to switch to generic logic when we have a "generic state() store of deleted fields, configurable or not" in #2282119: Make the Entity Field API handle field purging.
- adds a missing EM->clearCachedFieldDefinitions() in entity_invoke_bundle_hook() (might not be strictly needed by the patch, but it should logically be there - if some CUD happens on bundles, the field definitions should be rebuilt)
- minor : slighly streamlines field_entity_bundle_rename()

Status:Needs review» Needs work

The last submitted patch, 103: d8_entity_decouple-2144263-103.patch, failed testing.

yched’s picture

Status:Needs work» Needs review
StatusFileSize
new102.43 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,693 pass(es), 50 fail(s), and 14 exception(s).
[ View ]

HEAD moves fast... Patch was borched :-), here's the correct version. Interdiff was correct.

(pushed the latest patch to entity-decouple-yched - is ahead of / contains entity-decouple-plach)

Status:Needs review» Needs work

The last submitted patch, 105: d8_entity_decouple-2144263-105.patch, failed testing.

yched’s picture

Status:Needs work» Needs review
StatusFileSize
new102.93 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,797 pass(es), 1 fail(s), and 4 exception(s).
[ View ]
new1.71 KB

Doh. onBundleRename() housekeeping is only valid for fields that use a dedicated table.

Status:Needs review» Needs work

The last submitted patch, 107: d8_entity_decouple-2144263-107.patch, failed testing.

yched’s picture

Status:Needs work» Needs review
StatusFileSize
new103.02 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,807 pass(es).
[ View ]
new699 bytes

The weird simulation in EntityBundleFieldTest / entity_bundle_field_test_uninstall() again :
Clearing the field definition cache in entity_invoke_bundle_hook('create') makes it never populated again during the test until entity_bundle_field_test_uninstall() asks for the field definitions. At this point though, the module is being uninstalled, and cannot expose its field, and entity_bundle_field_test_uninstall() fails because the field definition it wants to work on is not found.

Clearing the field cache in entity_invoke_bundle_hook() is actually needed for 'rename' and 'delete', but not for 'create', so side-stepping this way...

This should be green.

yched’s picture

So, the remaining stuff AFAICT are

- @alexpott, do you think we need to mull a bit more on the issue you raised in #70 ? - back then you wrote "I think the fix in the attached patch is not really acceptable" :-)

- @fago, #62.2:

+++ b/core/lib/Drupal/Core/Entity/ContentEntityDatabaseStorage.php
@@ -1292,54 +1298,139 @@ public function onBundleRename($bundle, $bundle_new) {
+    $or = $query->orConditionGroup();
+    foreach ($field_definition->getColumns() as $column_name => $data) {
+      $or->isNotNull(static::_fieldColumnName($field_definition, $column_name));
+    }

[yched] That $or group could use a word of comment. If I get things right, it's needed because fields stored in the base table can have "all NULL" values for rows about bundles where the field does not actually exist ?
Also, we could possibly avoid it and have a more efficient query on fields stored in "per field tables" ?

[plach] Actually this code is not dealing with base fields yet, thus I cannot explain that or condition. Also this one is for @fago.

yched’s picture

StatusFileSize
new100.03 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,767 pass(es), 2 fail(s), and 1 exception(s).
[ View ]
new3.49 KB

Regarding @alexpott's fixes in #70 - a couple things have changed since then, let's just check if we still trigger this.

Attached patch reverts #70, just to see...

Status:Needs review» Needs work

The last submitted patch, 111: d8_entity_decouple-2144263-111.patch, failed testing.

alexpott’s picture

Status:Needs work» Needs review
StatusFileSize
new698 bytes
new104.77 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 44,037 pass(es), 14,325 fail(s), and 7,442 exception(s).
[ View ]

Here is a patch doing the utter minimum :)

The other test fail was due to an unrelated HEAD fail at the time the patch was posted.

yched’s picture

+++ b/.htaccess
@@ -110,7 +110,7 @@ DirectoryIndex index.php index.html index.htm
-  # RewriteBase /
+  RewriteBase /

unintended ? :-)

Status:Needs review» Needs work

The last submitted patch, 113: 2144263-yched.113.patch, failed testing.

alexpott’s picture

Status:Needs work» Needs review
StatusFileSize
new328 bytes
new104.35 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2144263-yched.116.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

lol yes...

Status:Needs review» Needs work

The last submitted patch, 116: 2144263-yched.116.patch, failed testing.

alexpott’s picture

Status:Needs work» Needs review
StatusFileSize
new104.62 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,934 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Needed a reroll due to #2286681: Make public properties on ConfigEntityBase protected protecting the uuid property.

Status:Needs review» Needs work

The last submitted patch, 118: 2144263-yched.118.patch, failed testing.

effulgentsia’s picture

StatusFileSize
new104.39 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,786 pass(es).
[ View ]
new3.33 KB
effulgentsia’s picture

Status:Needs work» Needs review
effulgentsia’s picture

+++ b/core/profiles/standard/standard.install
@@ -78,4 +78,10 @@ function standard_install() {
+
+  // Resave the plain_text formatter so that default filter plugins and
+  // dependencies are calculated correctly. This resolves an issue caused by the
+  // fact that filter is installed before editor but the standard profile also
+  // enables the file module.
+  entity_load('filter_format', 'plain_text')->save();

What is it about this patch that's making this necessary? AFAICT, this patch isn't changing the order or dependencies of any modules, so there's something more subtle happening here. This doesn't really make sense to put into standard.install, does it? All it does is add a status=0 filter to the format (along with the corresponding module dependency). Does that solve any actual bug, or just make ConfigImportAllTest not report an unexpected diff? If the latter, shouldn't we fix that in ConfigImportAllTest instead? Since this also touches on the WTF of formats saving filter configurations for filters that were never intentionally added by any deliberate action, and for which there's a @todo in FilterFormat::preSave() to resolve, would be great to spin this off into its own issue if we can isolate what triggers it.

plach’s picture

I just pushed #120 to the sandbox in entity-decouple-plach...

yched’s picture

@plach: thanks, made sure my branch is up to date :-)

@effulgentsia #122 : @alexpott explained the issue in #70, but yeah, we don't really have a clue so far as to why this is triggered by this specific patch here :-/

xjm’s picture

My understanding is that @fago is still AFK another week; does this issue need to be assigned to him?

alexpott’s picture

Currently debugging #70 - it is because with this patch we no long need to call field_purge_data() to twice to actually delete fields and instances. Currently ConfigImporterAllTest only calls it once meaning that filter is not uninstalled because a field using a format exists. See #2295129: Filter formats should not save plugin data when the plugin configuration matches defaults

alexpott’s picture

StatusFileSize
new602 bytes
new104.45 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2144263.127.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Since #2295129: Filter formats should not save plugin data when the plugin configuration matches defaults does not need to be a beta blocker lets add a todo and move on.

Status:Needs review» Needs work

The last submitted patch, 127: 2144263.127.patch, failed testing.

yched’s picture

Aw, nice catch. Thanks for the detective work @alepott ! Keeping the workaround here and cleaning that in #2295129: Filter formats should not save plugin data when the plugin configuration matches defaults is indeed our best bet here.

Regarding the other pending question (#62), I guess we can live without @fago's feedback for now, this code is going to be reshuffled / refactored in #2282119: Make the Entity Field API handle field purging anyway.

So, who's going to RTBC this now that we all posted patches ? :-)

alexpott’s picture

Status:Needs work» Needs review
StatusFileSize
new104.45 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,882 pass(es).
[ View ]

Minor conflict in the use statements in options.module.

Berdir’s picture

+++ b/core/modules/field/src/Entity/FieldInstanceConfig.php
@@ -745,4 +752,11 @@ public static function loadByName($entity_type_id, $bundle, $field_name) {
+  /**
+   * {@inheritdoc}
+   */
+  public function getUniqueStorageIdentifier() {
+    return $this->getField()->getUniqueStorageIdentifier();
+  }

This is no longer necessary, as FieldInstanceConfig no longer implements the field storage definition interface.

That's all I found, this looks good to me. I guess @yched can RTBC if he's happy with it? He worked on the patch as well, but I think we reviewed each other's changes pretty well, and there's just nobody left that could RTBC this that did not work on it.

yched’s picture

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new100.42 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,790 pass(es).
[ View ]
new665 bytes

@Berdir : nice catch.

Updated patch, and as per #131, RTBC.
Awesome team work :-)

xjm’s picture

Assigned:fago» catch

Yay!!

Since @alexpott worked on this patch, I think we probably want @catch to look at it.

catch’s picture

Status:Reviewed & tested by the community» Fixed

Read through, couldn't find anything to complain about. Nice work!

Committed/pushed to 8.x, thanks!

  • catch committed b4f282a on 8.x
    Issue #2144263 by fago, yched, alexpott, Berdir, plach, andypost,...

Status:Fixed» Closed (fixed)

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

Gábor Hojtsy’s picture

Issue tags:-sprint