Problem/Motivation

This is a child issue of #1498720: [meta] Make the entity storage system handle changes in the entity and field schema definitions, and in a way, the primary one. This is where we add the entity.definition_update_manager service and use it within update.php (more accurately, within update.inc and DbUpdateController) and within system_requirements() to report that running update.php is necessary.

Proposed resolution

See API changes.

Remaining tasks

Review. Commit.

User interface changes

Within the "Review database updates" step of update.php, lists entity type and field storage definition updates alongside module update functions:

API changes

  • Moves onFieldStorageDefinition*() methods from FieldableEntityStorageInterface to a new interface: FieldStorageDefinitionListenerInterface.
  • Makes EntityManagerInterface extend FieldStorageDefinitionListenerInterface.
  • Adds getInstalledDefinition() and getInstalledFieldStorageDefinitions() methods to EntityManagerInterface to track the versions of the definitions that handlers have been notified of.
  • Adds a $state dependency to EntityManager to track the above.
  • Adds a EntityDefinitionUpdateManagerInterface with the methods needsUpdates(), getChangeSummary() and applyUpdates() for use by system_requirements() and the update.php process.
  • Adds a EntityDefinitionUpdateManager implementation of the above and makes that core's implementation of the entity.definition_update_manager service. This is a single service that is responsible for all entity types.
  • Renames EntitySchemaHandlerInterface to EntityStorageSchemaInterface for consistency with classes that implement that interface, which are all named *StorageSchema. Adds requiresEntitySchemaChanges() and requiresEntityDataMigration() methods to this interface. The former is called by the EntityDefinitionUpdateManager implementation that's in this patch. The latter isn't yet, but could be in a followup or contrib implementation.
  • Adds a FieldableEntityStorageSchemaInterface interface that extends the above with requiresFieldSchemaChanges() and requiresFieldDataMigration().
  • Implements the above in SqlContentEntityStorageSchema. In followups, that will be all that's needed, but for now, SqlContentEntityStorage also implements that interface and delegate. (Related: #2332857: Decide how secondary entity handlers (like storage_schema) should be instantiated)
CommentFileSizeAuthor
#103 interdiff.txt923 byteseffulgentsia
#103 entity-schema_manager-2333113-103.patch78.17 KBeffulgentsia
#100 interdiff.txt6.03 KBeffulgentsia
#100 entity-schema_manager-2333113-100.patch77.93 KBeffulgentsia
#88 interdiff.txt1.62 KBeffulgentsia
#88 entity-schema_manager-2333113-88.patch75.7 KBeffulgentsia
#85 interdiff.txt702 byteseffulgentsia
#85 entity-schema_manager-2333113-85.patch75.58 KBeffulgentsia
#84 interdiff.txt6.35 KBeffulgentsia
#84 entity-schema_manager-2333113-84.patch75.59 KBeffulgentsia
#75 interdiff.txt1022 byteseffulgentsia
#75 entity-schema_manager-2333113-75.patch74.44 KBeffulgentsia
#74 interdiff.txt16.27 KBeffulgentsia
#74 entity-schema_manager-2333113-74.patch75.18 KBeffulgentsia
#71 interdiff.txt4.76 KBeffulgentsia
#67 interdiff.txt984 byteseffulgentsia
#67 entity-schema_manager-2333113-67.patch69.07 KBeffulgentsia
#66 interdiff.txt5.77 KBeffulgentsia
#66 entity-schema_manager-2333113-66.patch69.07 KBeffulgentsia
#65 entity-definition-update-manager.png68.85 KBeffulgentsia
#61 interdiff.txt6.57 KBeffulgentsia
#61 entity-schema_manager-2333113-60.patch63.15 KBeffulgentsia
#58 interdiff.txt3.8 KBeffulgentsia
#58 entity-schema_manager-2333113-58.patch63.13 KBeffulgentsia
#54 entity-schema_manager-2333113-54.patch63.31 KBeffulgentsia
#52 entity-schema_manager-2333113-52.patch61.33 KBeffulgentsia
#49 interdiff.txt2.18 KBeffulgentsia
#49 entity-schema_manager-2333113-49.patch55.46 KBeffulgentsia
#47 interdiff.txt15.47 KBeffulgentsia
#47 entity-schema_manager-2333113-46.patch55.46 KBeffulgentsia
#44 interdiff.txt5.43 KBeffulgentsia
#43 interdiff.txt728 byteseffulgentsia
#43 entity-schema_manager-2333113-43.patch50.14 KBeffulgentsia
#42 entity-schema_manager-2333113-42.patch50.19 KBeffulgentsia
#41 interdiff.txt1.28 KBeffulgentsia
#41 entity-schema_manager-2333113-41.patch50.17 KBeffulgentsia
#37 interdiff.txt11.24 KBeffulgentsia
#37 entity-schema_manager-2333113-37.patch49.85 KBeffulgentsia
#35 interdiff.txt762 byteseffulgentsia
#35 entity-schema_manager-2333113-35.patch57.76 KBeffulgentsia
#33 interdiff.txt12.94 KBeffulgentsia
#33 entity-schema_manager-2333113-33.patch57.7 KBeffulgentsia
#29 entity-schema_manager-2333113-29.patch64.86 KBeffulgentsia
#27 interdiff.txt728 byteseffulgentsia
#27 entity-schema_manager-2333113-27.patch64.83 KBeffulgentsia
#25 interdiff.txt34 KBeffulgentsia
#25 entity-schema_manager-2333113-25.patch64.78 KBeffulgentsia
#24 interdiff.txt7.07 KBeffulgentsia
#24 entity-schema_manager-2333113-21.patch65.08 KBeffulgentsia
#16 interdiff.txt25.28 KBeffulgentsia
#16 entity-schema_manager-2333113-16.patch67.25 KBeffulgentsia
#13 interdiff.txt2.76 KBeffulgentsia
#13 entity-schema_manager-2333113-13.patch65.57 KBeffulgentsia
#11 entity-schema_manager-2333113-11.patch65.18 KBeffulgentsia
#9 entity-schema_manager-2333113-7.patch65.18 KBeffulgentsia
#7 interdiff.txt10.48 KBeffulgentsia
#7 entity-schema_manager-2333113-7-do-not-test.patch65.18 KBeffulgentsia
#4 entity-schema_manager-2333113-4-do-not-test.patch66.95 KBplach
#4 entity-schema_manager-2333113-4-do-not-test.interdiff.txt2.69 KBplach
#1 entity-schema_manager-do-not-test.patch66.93 KBeffulgentsia
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

effulgentsia’s picture

Status: Active » Needs review
FileSize
66.93 KB

Here's the patch. It's applied on top of #2330121-29: Replace ContentEntityDatabaseStorage::getSchema() with a new EntityTypeListenerInterface implemented by SqlContentEntityStorageSchema, which hasn't landed yet, so until it does, keeping it away from testbot. The combined patch was tested successfully in #2298525-60: Test issue for Make ContentEntityDatabaseStorage handle changes in the entity schema definition.

No need to wait on the first issue landing before providing reviews on this one though.

yched’s picture

Makes SqlContentEntityStorageSchema implement both FieldableEntityStorageInterface and ContentEntitySchemaProviderInterface.

A StorageSchema implements a StorageInterface and a SchemaProviderInterface ?
I'm lost as to what are the animal species in our zoo :-)

effulgentsia’s picture

Issue summary: View changes

Re #2, lol, no that was a copy/paste error. Fixed.

plach’s picture

Fixed some indentation issues.

  1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityDatabaseStorage.php
    @@ -178,6 +179,19 @@ public function __construct(EntityTypeInterface $entity_type, Connection $databa
    +  public function hasData() {
    

    Looking again at this code, I am no longer sure this is a good idea: we could keep the base implementation and move this code in the schema handler.

  2. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
    @@ -994,6 +996,42 @@ public function onEntityTypeDelete(EntityTypeInterface $entity_type) {
    +  public function onFieldStorageDefinitionCreate(FieldStorageDefinitionInterface $storage_definition) {
    

    Here and in the other onField* methods we should probably clear field definition caches.

  3. +++ b/core/lib/Drupal/Core/Entity/Schema/ContentEntitySchemaManager.php
    @@ -0,0 +1,361 @@
    +  public function getChangeSummary($entity_type_id = NULL) {
    ...
    +  public function getSystemRequirements($phase) {
    

    I am no longer sure this code belongs to the service: would it make sense to extract it? The current approach makes it easily swappable though...

  4. +++ b/core/lib/Drupal/Core/Entity/Schema/SqlContentEntityStorageSchema.php
    @@ -101,13 +253,53 @@ public function onEntityTypeUpdate(EntityTypeInterface $entity_type, EntityTypeI
    +  public function onFieldStorageDefinitionCreate(FieldStorageDefinitionInterface $storage_definition) {
    +    $this->schemaManager()->onFieldStorageDefinitionCreate($storage_definition);
    +  }
    +
    

    I guess this works for now, but shouldn't this be notified directly from the entity manager?

yched’s picture

Re @effulgentsia #3 :
Ok, makes more sense :-)
"Schema implements SchemaProvider" still sounds weird though. A schema can provide a schema ? Litterally speaking, true I guess, but...

plach’s picture

Names totally up for discussion :)

That interface defines methods that provide information about schema changes (requires[Entity/Field]SchemaChanges() and requires[Entity/Field]DataMigration()), so a proper name should revolve around that idea.

effulgentsia’s picture

+1 to #4.2 and #4.4, so this implements that.

I don't have opinions yet on #4.1 and #4.3 or on #5.

webchick’s picture

Issue tags: +Needs usability review

Looks like this makes UI changes, but doesn't look like the UX team has chimed in here yet.

effulgentsia’s picture

Status: Needs review » Needs work

The last submitted patch, 9: entity-schema_manager-2333113-7.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
65.18 KB

Reroll.

Status: Needs review » Needs work

The last submitted patch, 11: entity-schema_manager-2333113-11.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
65.57 KB
2.76 KB
effulgentsia’s picture

Re #4.3, I think getChangeSummary() makes sense in the service+interface, but we should document what it returns: an array of array of strings. I think getSystemRequirements() should be removed though, and inlined into system_requirements().

"Schema implements SchemaProvider" still sounds weird though.

What about renaming EntitySchemaHandlerInterface to EntityStorageSchemaInterface (since it's the interface for all of the *StorageSchema classes) and adding requiresEntitySchemaChanges() and requiresEntityDataMigration() to that. And also adding a ContentEntityStorageSchemaInterface that extends EntityStorageSchemaInterface and adds requiresFieldSchemaChanges() and requiresFieldDataMigration(). Then we don't need a ContentEntitySchemaProviderInterface. Temporarily, we would need to make ContentEntityDatabaseStorage implement ContentEntityStorageSchemaInterface to proxy the calls to SqlContentEntityStorageSchema, but only until we fix #2332857: Decide how secondary entity handlers (like storage_schema) should be instantiated at which point, the schema manager will be able to communicate with the storage schema handler directly.

+++ b/core/lib/Drupal/Core/Entity/Schema/EntitySchemaManagerInterface.php
@@ -0,0 +1,99 @@
+  public function getChangeList($entity_type_id = NULL);
+  public function applyChanges($entity_type_id = NULL);
+  public function getChangeSummary($entity_type_id = NULL);

In the patch, nothing other than NULL is ever passed. How about removing the parameter, or are we trying to pre-optimize for contrib use cases?

plach’s picture

+1 on #14

About

In the patch, nothing other than NULL is ever passed. How about removing the parameter, or are we trying to pre-optimize for contrib use cases?

Well, yes, since these operations might be expensive I thought having the ability to limit them to just one entity type could be useful. I don't think those parameter would be problematic even if turns out they are useless. It's just one more way to use the API. I don't feel strongly about this, though.

+++ b/core/modules/field/src/Entity/FieldStorageConfig.php
@@ -344,9 +344,6 @@
-    \Drupal::entityManager()->clearCachedFieldDefinitions();

@@ -410,9 +407,6 @@
-    \Drupal::entityManager()->clearCachedFieldDefinitions();

Sorry, I am missing where we are restoring these.

effulgentsia’s picture

This implements #14, including the extra parameter removal, cause I did that before reading #15. But since you don't feel strongly about it, let's leave it out and have a followup to add it if someone cares.

Sorry, I am missing where we are restoring these.

In the EntityManager, per #4.2.

effulgentsia’s picture

+++ b/core/lib/Drupal/Core/Entity/Schema/EntitySchemaManagerInterface.php
@@ -0,0 +1,78 @@
+   * @return array
+   *   An associative array keyed by entity type id of change descriptors. Every
+   *   entry is an associative array with the following optional keys:
+   *   - entity_type: a scalar having only the DEFINITION_UPDATED value.
+   *   - field_storage_definitions: an associative array keyed by field name of
+   *     scalars having one value among:
+   *     - DEFINITION_CREATED
+   *     - DEFINITION_UPDATED
+   *     - DEFINITION_DELETED
+   *   - data_migration: boolean indicating whether the changes imply a data
+   *     migration.
+   */
+  public function getChangeList();

Arrays with magic keys is old-school. I think the value for each entity type should be an object with an interface (possible name: EntitySchemaChangeInterface for the part without field changes and ContentEntitySchemaChangeInterface that extends it with the field info). Perhaps EntitySchemaChangeInterface could also include a getSummary() method to replace EntitySchemaManagerInterface::getChangeSummary().

Perhaps we should also add a hasChanges() method for the case where the caller doesn't care about the details of the changes?

yched’s picture

+1 on #14 too, thanks @effulgentsia.
Issue summary should be updated accordingly ?

Don't count on me for code reviews though :-/

plach’s picture

+1 on #17 :)

In the EntityManager, per #4.2.

lol, and I even looked twice :D

Bojhan’s picture

Darn, whenever I see the label "entity" a little bit of me dies. It's really hard to parse, putting schema next to it makes it even harder.

Frankly I don't understand why this is a separate item from database updates. For core and most of contrib now, we can assume this would be database updates. Can't we just put this into database updates and when it happens to not be in the database, we show this option?

I do have a bunch of label recommendations too, but I want to tackle this problem first. Because it will be hard overall to explain to the user, he/she has to run 3 different types of updates (files, database, schema). We are making Drupal harder and harder to keep up to date, if we introduce complexities here.

effulgentsia’s picture

Issue summary: View changes

Updated summary per #18.

effulgentsia’s picture

Issue summary: View changes
plach’s picture

@Bojhan:

From a developer POV, I find it useful to know that entity updates are needed vs plain db updates, at least in the status report. I know developers should not be our primary concern in general, but the status report already displays quite technical information, so it did not look so inappropriate to me.

Would it be an acceptable compromise to keep the separate items on the status report and unify the update UI? We should try to avoid the "database" word in that case, because there is no easy way to tell whether we are running DB updates for entities without doing dirty things from an architectural POV.

I am thinking to something like "Module updates", without specifying where they are applied.

effulgentsia’s picture

Issue summary: View changes
FileSize
65.08 KB
7.07 KB

This addresses #4.1, which takes care of all the feedback so far except #17 and #20.

effulgentsia’s picture

This doesn't completely fix #17, but addresses it by moving getChangeList() to a protected method. We may have a use case at some point for making it public, and we may want to change the return value from an array of arrays to an array of objects even if we keep it protected, but I think that can all be figured out after beta.

I also renamed ContentEntitySchemaManager to EntitySchemaManager, because since it's a manager class, it's cross-entity-type, and it's a single service that needs to support all entity types. It just so happens that at this time, it doesn't need to do anything for config entities, but maybe that'll change at some point, so there's no need to lock that assumption into the name. That we have some content-entity-specific logic in there is ok IMO: just like we also have some content-entity-specific logic in EntityManager.

As far as the usability feedback in #20/#23, would it be ok to leave the patch as-is as far as that goes, and have a post-beta critical issue for finalizing the UX? I don't see any reason we can't tweak the UX after beta, but we need to get the functionality into place by beta.

Status: Needs review » Needs work

The last submitted patch, 25: entity-schema_manager-2333113-25.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
64.83 KB
728 bytes

FieldUITestNoBundle sets 'fieldable' to TRUE, but EntityManager::buildBaseFieldDefinitions() doesn't like that, so here's a workaround.

effulgentsia’s picture

+++ b/core/lib/Drupal/Core/Entity/Schema/SqlContentEntityStorageSchema.php
@@ -73,9 +83,90 @@ public function __construct(EntityManagerInterface $entity_manager, ContentEntit
+  public function requiresEntitySchemaChanges(EntityTypeInterface $entity_type, EntityTypeInterface $original) {
+  public function requiresFieldSchemaChanges(FieldStorageDefinitionInterface $storage_definition, FieldStorageDefinitionInterface $original) {

Why do we want these as separate methods? (same question for the data migration ones). Why not have a single requiresSchemaChanges() method that compares the complete schema needed by $entity_type plus all its field storages (which it can get from $entity_manager) with what it has stored in state? In addition to making the code simpler, wouldn't that also support a broader set of table mapping strategies (as well as the ability to update to a new table mapping strategy) than trying to make the determination field by field? It would also allow EntitySchemaManager to be less coupled to content entities in its API connection points.

effulgentsia’s picture

effulgentsia’s picture

Issue summary: View changes
effulgentsia’s picture

Issue summary: View changes
effulgentsia’s picture

Never mind about #28. I do think it makes sense now to do it field-by-field, since we already have onFieldStorageDefinition(Create|Update|Delete)(), which also operates field by field. And being able to update to a new table mapping strategy isn't the scope of the parent issue: that scope is only about supporting changes to entity and field definitions, which is what's handled by this patch.

This patch doesn't have tests for EntitySchemaManager, but I suggest deferring adding those tests until after we've completed the other parts of the parent issue, since for example, the next step involves a change of the schema layout logic and includes test coverage for that (see #2298525-63: Test issue for Make ContentEntityDatabaseStorage handle changes in the entity schema definition).

So, with that in mind, I'm happy with the state of this patch, but obviously can't RTBC it, nor can plach. Anyone else have additional feedback or up for RTBCing? Getting this in quickly will help with rolling the next (and possibly last beta-blocking) step.

effulgentsia’s picture

+++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
@@ -190,6 +381,70 @@ protected function getTables() {
+  protected function getEntitySchemaData(EntityTypeInterface $entity_type, array $schema) {
+    $schema_data = array();
+    $entity_type_id = $entity_type->id();
+    $keys = array('indexes', 'unique keys');
+    $unused_keys = array_flip(array('description', 'fields', 'foreign keys'));
+
+    foreach ($schema as $table_name => $table_schema) {
+      $table_schema = array_diff_key($table_schema, $unused_keys);
+      foreach ($keys as $key) {
+        // Exclude data generated from field storage definitions, we will check
+        // that separately.
+        if (!empty($table_schema[$key])) {
+          $data_keys = array_keys($table_schema[$key]);
+          $entity_keys = array_filter($data_keys, function ($key) use ($entity_type_id) {
+            return strpos($key, $entity_type_id . '_field_') !== 0;
+          });
+          $table_schema[$key] = array_intersect_key($table_schema[$key], array_flip($entity_keys));
+        }
+      }
+      $schema_data[$table_name] = array_filter($table_schema);
+    }
+
+    return $schema_data;
+  }

I was finding this code and its callers difficult to reason about, especially without test coverage, so I decided to revert it from this issue's scope. This was here primarily as a refinement for allowing a broader set of changes that could be done even in the presence of existing data, but for this issue's scope, I think we can do without that, and treat such cases as needing migrations, and then open a separate issue for re-adding extra automation like this.

I hope this patch's changes make sense, and make the patch as a whole a little easier for others to review, RTBC, and commit.

Status: Needs review » Needs work

The last submitted patch, 33: entity-schema_manager-2333113-33.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
57.76 KB
762 bytes
plach’s picture

+++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
@@ -105,7 +105,7 @@
-      $original->getStorageClass() != $entity_type->getStorageClass() ||
+      !in_array($original->getStorageClass(), array($entity_type->getStorageClass(), 'Drupal\Core\Entity\ContentEntityNullStorage')) ||

I don't get this change: why is it now necessary? I think it was more conservative to assume that any change in the storage class could imply a schema change. If we keep this approach we should at least include subclasses of the null storage.

effulgentsia’s picture

I decided to try to address the usability concerns of #20 by making the UX changes required by this patch as minimal as possible, so that we can have a followup to make any more substantive changes. This now doesn't change the UX of the status report page at all (if entity schema updates are needed, they go in the existing "database updates" line), and it keeps the same sequence in update.php, just adds each entity type that needs updating to the count (so if there's 1 entity type that needs updating plus 1 module update function, that just gets listed as "2 pending updates"). I think #23 makes some good points about the originally proposed UX being better in some ways, but let's deal with that in a post-beta issue.

effulgentsia’s picture

Issue summary: View changes
effulgentsia’s picture

Issue summary: View changes
effulgentsia’s picture

Issue summary: View changes
effulgentsia’s picture

Re #36, that was needed because I hardened SqlContentEntityStorageSchema::onEntityTypeUpdate() to check if migration is needed, since it can be called outside of EntitySchemaManager (and is in contact_storage_test_install()). Good point though about allowing subclasses of NULL storage: fixed.

I think I'm done with this pending feedback. I think this is stripped down as much as it can be now, so looking forward to an RTBC soon.

Hint for testing: one way you can test this manually is by removing the "revision" = "revision_id" line in the annotation of Drupal\block_content\Entity\BlockContent. Then when you go to the status report page, it should tell you you need an update, and then you can proceed with the update.php process, at the conclusion of which, you can go back to the status report page and see that you no longer need an update. Then you can put that annotation line back in and repeat the process. You can also do that with a custom block already existing, in which case update.php will report that it can't perform the update due to existing data. Note, that changing revisionability is less interesting than adding/removing/changing fields, but the latter requires more of the parent issue to make work; this issue is just getting the basic scaffolding in place.

effulgentsia’s picture

Rerolled for HEAD changes to EntityManager.

effulgentsia’s picture

#27 got fixed in HEAD, so this reverts that.

effulgentsia’s picture

This resolves the todos from #37, which also makes the UX even more consistent with HEAD, and ensures that an error triggered during the update (e.g., from having existing entity data that needs migration) is caught and reported.

Hint for testing: one way you can test this manually is by removing the "revision" = "revision_id" line in the annotation of Drupal\block_content\Entity\BlockContent.

Actually, upon further testing, I found that this isn't a convenient test case, because if you try a scenario where there's an existing custom block in order to confirm that update.php fails, then you end up with errors when you try to view any site page due to block queries not working. Which is the expected situation (you have a mismatch between the schema and the code, so it's expected for some queries to fail) and not something for this issue to fix, but makes it not a convenient use case for manual testing. So instead, you can enable the entity_test module and do that same revisionability change on EntityTestRev instead.

fago’s picture

Status: Needs review » Needs work

Thanks for all the updates. This looks already quite solid to me, remarks below (reviewed #43):

  1. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
    @@ -991,6 +999,57 @@ public function onEntityTypeDelete(EntityTypeInterface $entity_type) {
    +    $this->clearCachedFieldDefinitions();
    

    It seems random to clear the caches in between two listener calls. Could it be done first or last instead? If not, there should be a comment why it has to happen in between. Same for others.

  2. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
    @@ -1047,4 +1106,13 @@ public function onBundleDelete($entity_type_id, $bundle) {
    +  protected function schemaManager() {
    +    return $this->container->get('entity.schema.manager');
    

    Even though we've got the container, we should use injected dependencies. Or is there a specific reason not to?

  3. +++ b/core/lib/Drupal/Core/Entity/Schema/ContentEntityStorageSchemaInterface.php
    @@ -0,0 +1,44 @@
    + * Contains \Drupal\Core\Entity\Schema\ContentEntityStorageSchemaInterface.
    

    As this is actually about fields and we do already name field-specific storage interface using "fieldable", I think we should follow the pattern here and call this FieldableEntityStorageSchemaInterface. This plays well FieldableEntityStorageInterface and #2275659: Separate FieldableEntityInterface out of ContentEntityInterface.

  4. +++ b/core/lib/Drupal/Core/Entity/Schema/ContentEntityStorageSchemaInterface.php
    @@ -0,0 +1,44 @@
    +   * Checks whether the definition changes imply field schema changes.
    

    Checks for a single definition change, not multiple.

  5. +++ b/core/lib/Drupal/Core/Entity/Schema/ContentEntityStorageSchemaInterface.php
    @@ -0,0 +1,44 @@
    +   * Checks whether the field storage definition changes imply a data migration.
    

    same here.

  6. +++ b/core/lib/Drupal/Core/Entity/Schema/EntitySchemaManager.php
    @@ -0,0 +1,373 @@
    + * Manages entity schema changes.
    

    inheritdoc. But even more, I guess this could use some overview on how changes are calculated, i.e. it would be good to have it summarized that it keeps definitions in state for comparing them later on.

  7. +++ b/core/lib/Drupal/Core/Entity/Schema/EntitySchemaManager.php
    @@ -0,0 +1,373 @@
    +class EntitySchemaManager implements EntitySchemaManagerInterface {
    

    That together with EntityStorageSchema makes it bit unclear what is charge of what. I could assume that both work with storage schema, but the name and docs should tell me which object is in charge of what.

    Given that the implementation is actually not dealing with schema at all, I think we might want to name it different in general. As it detects and applies changes to entity and field definitions, what about EntityChangeManager?

  8. +++ b/core/lib/Drupal/Core/Entity/Schema/EntitySchemaManager.php
    @@ -0,0 +1,373 @@
    +   * @param \Drupal\Core\Entity\EntityManagerInterface $entity_manager
    +   * @param StateInterface $state
    

    bad docs.

  9. +++ b/core/lib/Drupal/Core/Entity/Schema/EntitySchemaManager.php
    @@ -0,0 +1,373 @@
    +    $entity_type_ids = isset($entity_type_id) ? array($entity_type_id) : array_keys($definitions);
    

    how could $entity_type_id be set?

  10. +++ b/core/lib/Drupal/Core/Entity/Schema/EntitySchemaManager.php
    @@ -0,0 +1,373 @@
    +      $definition = $definitions[$entity_type_id];
    

    can be simplified by not throwing away array values in the first place.

  11. +++ b/core/lib/Drupal/Core/Entity/Schema/EntitySchemaManager.php
    @@ -0,0 +1,373 @@
    +      if ($definition instanceof ContentEntityTypeInterface && $storage instanceof ContentEntityStorageSchemaInterface) {
    

    superfluous check, definitions are filtered already.

  12. +++ b/core/lib/Drupal/Core/Entity/Schema/EntitySchemaManager.php
    @@ -0,0 +1,373 @@
    +        if ($storage->requiresEntitySchemaChanges($definition, $original)) {
    

    It seems weird to ask the storage about it, shouldn't the schema manager know better / itself?

  13. +++ b/core/lib/Drupal/Core/Entity/Schema/EntitySchemaManager.php
    @@ -0,0 +1,373 @@
    +            if (!$definition->hasCustomStorage()) {
    

    What if a field changes from custom to not custom, or the other way round? requiresFieldSchemaChanges() seems to take care of that case so removing the if should be enough.

  14. +++ b/core/lib/Drupal/Core/Entity/Schema/EntityStorageSchemaInterface.php
    @@ -0,0 +1,43 @@
    + * Defines an interface for handling the storage schema of entities.
    

    It handles te schema of an entity type, not entities - that's the differentiation to the entity schema manager imo. -> The schema manager takes care of handling entity schema changes for all entity types, while the per-type entity storage schema performs them. Still, the naming could be better :/

    Thinking about it, what the schema manages does is that it actually keeps track of applied definitions vs. the definitions in code, i.e. it handles the schema state. So what about "EntityStateManager" ? So you ask the EntityStateManager whether there are changes to be applied and ask it to apply then. Given that naming it makes also sense that EntityStateManager informs the EntityManager about changes in applyChanges().

  15. +++ b/core/lib/Drupal/Core/Entity/Schema/EntityStorageSchemaInterface.php
    @@ -0,0 +1,43 @@
    +   * Checks whether the definition changes imply entity schema changes.
    

    change, singular :)

  16. +++ b/core/lib/Drupal/Core/Entity/Schema/EntityStorageSchemaInterface.php
    @@ -0,0 +1,43 @@
    +   * Checks whether the entity type definition changes imply a data migration.
    

    singular.

  17. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
    @@ -1366,6 +1367,34 @@ protected function usesDedicatedTable(FieldStorageDefinitionInterface $definitio
    +  public function requiresEntitySchemaChanges(EntityTypeInterface $entity_type, EntityTypeInterface $original) {
    +    return $this->schemaHandler()->requiresEntitySchemaChanges($entity_type, $original);
    +  }
    

    As noted above, it's weird to have the storage dealing with schema when there is a separate storage schema. I do think we should fix that before commit as this makes it quite confusing. :/ Compared two that, accepting the trade-off of the simple-solution suggested in #2332857: Decide how secondary entity handlers (like storage_schema) should be instantiated (assuming that both storage and storage get the same database injected) seems to be the better option to me.
    If we can find a better solution for #2332857: Decide how secondary entity handlers (like storage_schema) should be instantiated later on, that's fine but it does not require polluting our interfaces meanwhile then.

  18. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
    @@ -75,7 +78,66 @@ public function __construct(EntityManagerInterface $entity_manager, ContentEntit
    +    // If we're updating from NULL storage, then there's no stored data that
    +    // requires migration.
    +    $original_storage_class = $original->getStorageClass();
    

    It seems weird to hard-code the check for null storage plus non-null storage might not have any data either.

    Can we simply ask the storage whether it has data instead? I'd vote for adding hasData() to EntityStorageInterface as it useful to know anyway for all entity types I think. You could already check it by using loadMultiple(NULL) but that's wasteful/dangerous, or one could use countFieldData() with the id field but that's bad DX and bound to extendable entities.
    That could be a follow-up though.

  19. +++ b/core/modules/system/src/Controller/DbUpdateController.php
    @@ -209,6 +209,38 @@ protected function info() {
    +  function entitySchema() {
    

    public ?

  20. +++ b/core/modules/system/src/Controller/DbUpdateController.php
    @@ -209,6 +209,38 @@ protected function info() {
    +      $build['summary'] = array('#markup' => $this->t('No entity schema changes available.'));
    

    Should be deprecated since the count is added to db changes now.

yched’s picture

Agreed with @fago that SchemaManager is not a great name - it doesn't really "manage" schemas ? Not in the sense where a PluginManager manages its Plugins.

Sure, "manage" is kind of vague/open, but still it doesn't seem really fit for what this thing does withs Schemas. We've added quite a few classes in the "sql storage" ecosystem, getting the names right would certainly help reducing the perceived complexity ?

effulgentsia’s picture

Thanks for the review!

This fixes 1-5 and 14-16.

I'll address 6-13 (EntitySchemaManager) in a separate comment/patch.

Re 17: would it be okay to push that to a follow-up that we do after the rest of the parent issue, because the remaining step of that issue is to move a bunch of code, including some of those implementations, from SqlContentEntityStorage to SqlContentEntityStorageSchema. Note that this patch doesn't pollute the interfaces, only the SqlContentEntityStorage implementation, which is already partially polluted with the field schema implementations.

Re 18: opened a followup and added @todos.

Re 19-20: Already fixed in #44.

Status: Needs review » Needs work

The last submitted patch, 47: entity-schema_manager-2333113-46.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
55.46 KB
2.18 KB

It seems random to clear the caches in between two listener calls. Could it be done first or last instead?

#47 tried to do it first, but some tests fail with that. So, here's trying it last.

effulgentsia’s picture

Assigned: Unassigned » effulgentsia
Status: Needs review » Needs work

I made some progress on #45's various EntitySchemaManager issues, but ran out of steam, so will continue after some sleep.

Additional reviews welcome in the meantime, but setting status to "needs work" to reflect reality.

fago’s picture

@naming: After sleeping about it I sorta like EntityStateManager. It could be used to replace the field-config-state baked into field-config storage controllers right now also. Having the field-config storage talking to an EntityStateManager to fetch them makes sense, while it doesn't make sense for it to fetch it from a schema manager.

Re 17: would it be okay to push that to a follow-up that we do after the rest of the parent issue, because the remaining step of that issue is to move a bunch of code, including some of those implementations, from SqlContentEntityStorage to SqlContentEntityStorageSchema. Note that this patch doesn't pollute the interfaces, only the SqlContentEntityStorage implementation, which is already partially polluted with the field schema implementations.

Ah, sure, that's the better option. Yeah when it's not in the interface but just some quirks in the implementation I do think it's fine to solve it as follow-up.

effulgentsia’s picture

Assigned: effulgentsia » Unassigned
Status: Needs work » Needs review
FileSize
61.33 KB

Changed a lot. Interdiff is bigger than patch, so not attaching it. Hope you like the changes.

Status: Needs review » Needs work

The last submitted patch, 52: entity-schema_manager-2333113-52.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
63.31 KB
effulgentsia’s picture

Title: Add an entity schema manager to track the difference between what's installed vs. what code expects, and to let update.php update the former to the latter or report that it can't » Add an EntityDefinitionUpdateManager so that entity handlers can respond (e.g., by updating db schema) to code updates in a controlled way (e.g., from update.php)
Issue summary: View changes

Updated title and summary for latest patch.

effulgentsia’s picture

Issue summary: View changes

Status: Needs review » Needs work

The last submitted patch, 54: entity-schema_manager-2333113-54.patch, failed testing.

effulgentsia’s picture

Based on the comments in #45 and #46, I renamed EntitySchemaManager to EntityDefinitionUpdateManager and moved it out of the Schema subnamespace in #52. I also tried to fully decouple it from schema concerns (at least in theory, storage schema is not the only handler that might care about definition changes), but as the failures in #54 show, couldn't quite do it.

So opened #2336895: Allow entity type and field storage definition objects to be compared for definition equality for finishing that job after beta. See interdiff for the affected code.

plach’s picture

Overall this looks great to me! I was playing with similar ideas when first working on this code but I was afraid to put too much stuff on the table. However IMO making the Entity Manager entirely responsible for tracking definitions is simply the right thing to do, and I totally agree with trying to decouple the Entity Definition Update Manager from schema handling. Again, I was trying to achieve the same in my early work but then I was afraid of making too many changes. To sum up, a very big +1 on this: I think that the fact that we arrived independently to very similar conclusions is a signal we are going the right way :)

  1. +++ b/core/lib/Drupal/Core/Entity/EntityDefinitionUpdateManager.php
    @@ -0,0 +1,235 @@
    +        $summary[$entity_type_id][] = $this->t('The %entity_type entity type has definition updates that require schema changes.', array('%entity_type' => $entity_type->getLabel()));
    ...
    +              $summary[$entity_type_id][] = $this->t('The %field_name field has storage definition updates that require schema changes.', array('%field_name' => $storage_definitions[$field_name]->getLabel()));
    

    I think these messages are a bit too technical for a user-facing UI. I tried to stay a bit more generic in my initial version. What about not mentioning schema and just say that they "need to be updated"/"need updates"?

  2. +++ b/core/lib/Drupal/Core/Entity/Schema/EntityStorageSchemaInterface.php
    @@ -0,0 +1,77 @@
    +namespace Drupal\Core\Entity\Schema;
    +use Drupal\Core\Entity\EntityTypeInterface;
    

    Missing blank line

  3. +++ b/core/lib/Drupal/Core/Entity/Schema/EntityStorageSchemaInterface.php
    @@ -0,0 +1,77 @@
    +   * This is only called if requiresEntityStorageSchemaChanges() returns TRUE,
    

    "This *should* be called", otherwise we are assuming an implementation ;)

  4. +++ b/core/lib/Drupal/Core/Entity/Schema/EntityStorageSchemaInterface.php
    @@ -0,0 +1,77 @@
    +   * This function can return TRUE if any of these conditions apply:
    +   * - There are no existing entities for the entity type.
    +   * - There are existing entities, but the schema changes can be applied
    ...
    +   * When this function returns FALSE, site administrators will be unable to
    

    This should be FALSE or the text logic should be reversed.

  5. +++ b/core/lib/Drupal/Core/Entity/Schema/EntityStorageSchemaInterface.php
    @@ -0,0 +1,77 @@
    +   *   the new definition (e.g., if changing an entity type from revisionable
    +   *   to non-revisionable, then it's okay to drop data for the non-default
    +   *   revision).
    

    This is ok as example, but our current implementation requires a data migration even when switching from revisionable to non-revisionable, as the langcode column needs to be moved from the revision table to the base table.

  6. +++ b/core/lib/Drupal/Core/Entity/Schema/FieldableEntityStorageSchemaInterface.php
    @@ -0,0 +1,68 @@
    +   * This function can return TRUE if any of these conditions apply:
    

    Same as above

  7. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
    @@ -1366,6 +1366,34 @@ protected function usesDedicatedTable(FieldStorageDefinitionInterface $definitio
    +  public function requiresEntityStorageSchemaChanges(EntityTypeInterface $entity_type, EntityTypeInterface $original) {
    

    What about marking these as deprecated so non one tries to use them while we are still working our way through the meta?

  8. +++ b/core/modules/field/src/Entity/FieldStorageConfig.php
    @@ -290,8 +290,8 @@ protected function preSaveNew(EntityStorageInterface $storage) {
    +    $entity_manager->onFieldStorageDefinitionCreate($this);
    

    This is SO better :)

tim.plunkett’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityManagerInterface.php
@@ -8,11 +8,12 @@
+interface EntityManagerInterface extends PluginManagerInterface, EntityTypeListenerInterface, FieldStorageDefinitionListenerInterface {

This really doesn't seem right to me. Why does EntityManagerInterface know about FieldStorageDefinition anything?

Can't we please create a new class and move all of this stuff out? I don't know at what point EntityManager became the dumping ground for all of this code that only pertains to half of entity types, but this is getting out of control.

I posted this to #2298525: Test issue for Make ContentEntityDatabaseStorage handle changes in the entity schema definition instead of here, but @effulgentsia asked me to open a new issue. So we now have #2337191: Split up EntityManager into many services too

effulgentsia’s picture

Fixes #59's 1-6.

Re item 7, I don't think it makes sense to mark something deprecated if we can't document a working alternative. So either we'll fix #2332857: Decide how secondary entity handlers (like storage_schema) should be instantiated in time for a core committer to allow removing the wrapper from SqlContentEntityStorage (on the grounds that few modules will be affected by that break), or we'll need to leave the wrapper in, and can then mark it as deprecated and slated for removal in whichever Drupal version we agree to allow that break.

The last submitted patch, 54: entity-schema_manager-2333113-54.patch, failed testing.

Bojhan’s picture

Are there some new screens to review?

effulgentsia’s picture

Issue summary: View changes
FileSize
68.85 KB

Here's the screenshot. It's the same "review updates" screen as in HEAD, no difference when "X pending updates" is collapsed. When it's expanded, it lists entity and field definition updates above module updates (I added a dummy update function to book module, locally, to show what the screen looks like when there are both kinds of updates: entity/field definitions and regular module update functions). The concept of database updates that are triggered by a change to entity/field definitions rather than by an explicit update function is the new thing being introduced here. Per #37, I don't think this is necessarily the UI we'll want in the long run, just the minimal change from HEAD to complete this issue, and then perhaps we can have a followup to optimize the experience.

effulgentsia’s picture

Added a couple simple tests for EntityDefinitionUpdateManager. Did not add tests for creating/updating/deleting fields since that requires the rest of the parent issue to work.

effulgentsia’s picture

Once more. With grammar.

The last submitted patch, 66: entity-schema_manager-2333113-66.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 67: entity-schema_manager-2333113-67.patch, failed testing.

fago’s picture

I like the more general wording in the update-info - it makes a lot of sense in particular when more changes might be triggered later. Great to see some basic tests added as well, thanks!

I reviewed the changes, here are some remarks (all tests only):

  1. +++ b/core/modules/system/src/Tests/Entity/EntityDefinitionUpdateTest.php
    @@ -0,0 +1,112 @@
    + * Definition of Drupal\system\Tests\Entity\EntityDefinitionUpdateTest.
    

    Contains

  2. +++ b/core/modules/system/src/Tests/Entity/EntityDefinitionUpdateTest.php
    @@ -0,0 +1,112 @@
    +namespace Drupal\system\Tests\Entity;
    +use Drupal\Core\Entity\EntityStorageException;
    

    Missing new line between them.

  3. +++ b/core/modules/system/src/Tests/Entity/EntityDefinitionUpdateTest.php
    @@ -0,0 +1,112 @@
    +   * The entity definition update manager service.
    

    Just mentioning, not an issue - but i think it should not call it a "service" as it's just a dependency and the class does not know whether it receives a service or not. -> It'S just the manager.

  4. +++ b/core/modules/system/src/Tests/Entity/EntityDefinitionUpdateTest.php
    @@ -0,0 +1,112 @@
    +  protected function setUp() {
    

    misses inheritdoc.

  5. +++ b/core/modules/system/src/Tests/Entity/EntityDefinitionUpdateTest.php
    @@ -0,0 +1,112 @@
    +  function testNoUpdates() {
    ...
    +  function testUpdateWithoutData() {
    ...
    +  function testUpdateWithData() {
    

    missing public keyword.

  6. +++ b/core/modules/system/src/Tests/Entity/EntityDefinitionUpdateTest.php
    @@ -0,0 +1,112 @@
    +    // Install every entity type's schema. Start with entity_test_rev not
    +    // supporting revisions, and ensure its revision table isn't created.
    

    It's not ensured in this case.

  7. +++ b/core/modules/system/src/Tests/Entity/EntityDefinitionUpdateTest.php
    @@ -0,0 +1,112 @@
    +    entity_create('entity_test_rev', array())->save();
    

    Should use entity manager which is already injected.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
71.1 KB
4.76 KB

Re #70, I guess that's what happens when I blindly copy/paste from another test that's not up to date with our coding standards :)

Fixed here, plus the test failure that was exposed in #66 from resetting $handlers within EntityManager::clearCachedDefinitions(). I wonder if this fix will expose yet some other bugs :)

fago’s picture

Adds getInstalledDefinition() and getInstalledFieldStorageDefinitions() methods to EntityManagerInterface to track the versions of the definitions that handlers have been notified of.

While initially, I thought it should be the EntityDefinitionUpdateManager who keeps track of old definitions, I can get behind having the EM doing it. It makes sense to have getting definitions centralized in a single service.

Generally, I am worried by the getInstalled(FieldStorage)Definitions() naming and possible confusion it might add. From reading method names only you might think getDefinition() is not necessarily safe to call as definitions might be not installed. Indeed, they might not be installed, but still getDefinition() should be always used.

I had to chew about this a bit - so sry this took a while. I think the following rename would already suffice to avoid confusions:

  • rename getInstalled(FieldStorage)Definition(s)() to getLastInstalled(FieldStorage)Definition(s)()

Then, I've been thinking whether it makes sense to have an API method like installDefinition(), which then internally can handle getting the last installed definition while optionally getting one passed. This seems like a reasonable API which people can use when for some reasons updates did fail / not work in order to manually recover their sites. Without that you would have to set the old version and trigger applying updates in order to manually run a certain update, what seems unnecessarily complex/indirect compared to a direct API call, e.g. if you need to run it from an update function. Let's discuss this in a follow-up maybe?

Else this seems ready to me.

+++ b/core/lib/Drupal/Core/Entity/Schema/EntityStorageSchemaInterface.php
@@ -0,0 +1,80 @@
+ * During the life of the website, an entity type's definition can change in a

oh you assume Drupal is handling websites only? ;-)

The last submitted patch, 71: entity-schema_manager-2333113-71.patch, failed testing.

effulgentsia’s picture

Without that you would have to set the old version and trigger applying updates in order to manually run a certain update, what seems unnecessarily complex/indirect compared to a direct API call, e.g. if you need to run it from an update function. Let's discuss this in a follow-up maybe?

Opened #2337679: Create an easier API for installing an entity type or field storage definition update.

This fixes the rest of #72. It also fixes the test failures of #71 (https://qa.drupal.org/pifr/test/862378) by relaxing unnecessary ->exactly(2) requirements to ->atLeastOnce(). Specifically needed because the patch removes a superfluous and erroneous (because it is defaulted in SqlContentEntityStorage::__construct()) call to getDataTable(), for which there's a @todo to remove, to fix the failure from #67. But I cleaned up the other ones too, because there's no functional difference in calling idempotent functions multiple times, so it makes no sense to assert for that.

In doing so, I also realized there were stray hunks in that same test that aren't part of the scope of this issue (they're in the parent meta, so made their way into this issue by accident), so I reverted them.

effulgentsia’s picture

In doing so, I also realized there were stray hunks in that same test that aren't part of the scope of this issue (they're in the parent meta, so made their way into this issue by accident), so I reverted them.

Um, looking at #74's interdiff and earlier patches, those stray hunks weren't there. PHPStorm is playing tricks on me. But in fixing it, I introduced a stray hunk, which this fixes.

fago’s picture

Status: Needs review » Needs work

Thanks, changes look good.

I gave this a manual test drive but ran into troubles :/ I tried duplicating the node promoted field while no nodes were created (copied it to promoted2). The change was detected fine, update reported to be successful but the column has not been added and consequently adding a node fatal-ed. Tried the same with users's timezone (copied to users2) while there is already a user: same results, upgrade was sucessful, but column not added. Trying to remove the column lead to errors during the next upgrade.php run:

Notice: Undefined index: timezone2 in Drupal\Core\Entity\EntityDefinitionUpdateManager->applyUpdates() (line 133 of /var/www/core/lib/Drupal/Core/Entity/EntityDefinitionUpdateManager.php).

Recoverable fatal error: Argument 1 passed to Drupal\Core\Entity\EntityManager::onFieldStorageDefinitionDelete() must implement interface Drupal\Core\Field\FieldStorageDefinitionInterface, null given, called in /var/www/core/lib/Drupal/Core/Entity/EntityDefinitionUpdateManager.php on line 133 and defined in Drupal\Core\Entity\EntityManager->onFieldStorageDefinitionDelete() (line 1064 of /var/www/core/lib/Drupal/Core/Entity/EntityManager.php).

effulgentsia’s picture

@fago: yes, per #66, fields don't work yet, because HEAD's SqlContentEntityStorage::_fieldSqlSchema(), and therefore, onFieldStorageDefinition(Create|Update|Delete)() has hard-coded assumptions that it's working with dedicated table (non-base) fields. There's 150k of changes in #2298525-77: Test issue for Make ContentEntityDatabaseStorage handle changes in the entity schema definition to fix that, which is the last beta blocking step of the meta. Do you think it's okay for this patch to proceed with that known bug, or do we need to throw an exception somewhere as an interim step?

effulgentsia’s picture

and consequently adding a node fatal-ed

While this sounds pretty bad, I'll point out that I think the same is true in HEAD: if you deploy new code that contains a new base field after already installing the module, I think you'll get the same fatal errors when trying to save the entity. Which is why the entire parent issue is beta blocking.

effulgentsia’s picture

Status: Needs work » Needs review

Oops. Should have changed the status in #77.

fago’s picture

Status: Needs review » Needs work

>Which is why the entire parent issue is beta blocking.
right, agreed.

@fago: yes, per #66, fields don't work yet, because HEAD's SqlContentEntityStorage::_fieldSqlSchema(), and therefore, onFieldStorageDefinition(Create|Update|Delete)() has hard-coded assumptions that it's working with dedicated table (non-base) fields.

ah sure, sry. Given that's beta blocking anyway I do not think throwing exceptions meanwhile is better than just having the known bug - there is no exception right now in HEAD either. However, I think adding a short summary on what's left for the beta blocker after this issue getting in would make sense - as with this one we are already a big step closer.

I gave this another test drive with moving back and forth in block content revision-ability, what works as it should. However, after adding some data it basically dies during the update, due to the data-change exception. That's not nice UX and needs improvement, but not a deal-breaker for moving on with this now imo.

However, then - after a fresh standard installation this presents two bogus updates:
>node entity type
Create the URL alias field.
>taxonomy_term entity type
Create the URL alias field.

After applying them once, they go away though.

Also the update category title should use the entity label and misses translatability (sry, missed that earlier):
'#title' => $entity_type_id . ' entity type',

The bogus update represents a regression compared to HEAD, so I think this needs to be fixed before we can move on with this.

Berdir’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityManager.php
@@ -1047,4 +1129,81 @@ public function onBundleDelete($entity_type_id, $bundle) {
+  protected function setLastInstalledFieldStorageDefinitions($entity_type_id, array $storage_definitions) {
+    $this->state->set('entity.manager.' . $entity_type_id . '.field_storage_definitions', $storage_definitions);
+  }

Is there a reason we're using state instead of a custom key value collection(s)?

I'm still considering to add caching to state (possibly in contrib), and we don't want to end up with all those field definitions in the cache that we load on every request I think? Would also allow to get all definitions if we ever need them? And shorter keys.

yched’s picture

@Berdir: this raises interesting questions about the heuristics of State vs some other dedicated persistent storage...
This really feels like "metadata about the current site instance" to me ?

effulgentsia’s picture

However, after adding some data it basically dies during the update, due to the data-change exception. That's not nice UX and needs improvement, but not a deal-breaker for moving on with this now imo.

Opened #2337921: Improve the UX of update.php's handling of entity type updates for that and other UX issues identified while working on this issue. @Bojhan and others: I don't know what tags to add to it (i.e., does it "Needs usability review" prior to having a patch to review), so for anyone who knows, please tag it accordingly.

However, I think adding a short summary on what's left for the beta blocker after this issue getting in would make sense

Opened #2337927: SqlContentEntityStorage::onFieldStorageDefinition(Create|Update|Delete)() is broken for base fields. Will post a patch there once this issue lands (or maybe sooner), but roughly, the "-do-not-test" patch from #2298525-77: Test issue for Make ContentEntityDatabaseStorage handle changes in the entity schema definition.

Will respond to the other feedback next.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
75.59 KB
6.35 KB

However, then - after a fresh standard installation this presents two bogus updates:
>node entity type
Create the URL alias field.
>taxonomy_term entity type
Create the URL alias field.

Fixed this in standard.install. However, if you install Minimal profile, and then install Path module, then those updates will show up as needed. Or, if you install Standard profile and then uninstall Path module, the "Delete" instead of "Create" messages will show up as needed updates. Which raises a question: should all module installation/uninstallation automatically trigger an applyUpdates() as well? However, what if a data migration is needed? Let's tackle those questions in #83's UX followup.

Also the update category title should use the entity label and misses translatability

In HEAD, DbUpdateController::selection() sets the #title for module updates to $module . ' module', not to the module's human-friendly label, and without translating " module". So, what's here is consistent with that, but if we want to change that, let's do it in that UX followup.

Is there a reason we're using state instead of a custom key value collection(s)?

this raises interesting questions about the heuristics of State vs some other dedicated persistent storage

Yeah, I'm not too clear either on whether state has any intended semantics other than a default kv collection, and if so, whether installed entity definitions fit those semantics or not. But, in HEAD, system.schema is a separate kv collection, so based on that, I changed this to use its own collection as well.

effulgentsia’s picture

Fixed indentation.

The last submitted patch, 84: entity-schema_manager-2333113-84.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 85: entity-schema_manager-2333113-85.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
75.7 KB
1.62 KB

Fixes EntityManagerTest for the change from State to KV. This should be green. I think all feedback has been addressed. Anything else standing in the way of an RTBC?

fago’s picture

Status: Needs review » Reviewed & tested by the community

In HEAD, DbUpdateController::selection() sets the #title for module updates to $module . ' module', not to the module's human-friendly label, and without translating " module". So, what's here is consistent with that, but if we want to change that, let's do it in that UX followup.

uhm, I see.

Which raises a question: should all module installation/uninstallation automatically trigger an applyUpdates() as well? However, what if a data migration is needed? Let's tackle those questions in #83's UX followup

Yeah, it probably should apply updates but only if there is no data migration involved. Howsoever, that can be improved in follow-ups.

>Anything else standing in the way of an RTBC?
Nope, I agree this is ready to go - I reviewed the patch several times, all concerns have been addressed and plach is happy with the approach taken. We've got basic test coverage and there are no UX changes other than adding new update messages (see summary) -> rtbc! Great work!

Bojhan’s picture

Happy that we toned this down to just messages in the updates. That feels like the MVP approach!

The UX follow up is #2337921: Improve the UX of update.php's handling of entity type updates. Lets make sure this doesn't end up with just a bunch of comments from me :)

chx’s picture

I still disagree with this issue, the parent issue and all that. Another 75kb patch. Where is the end of this? I see already five siblings. Why are we building so vehemently parallel subsystems instead of just migrating?

effulgentsia’s picture

Why are we building so vehemently parallel subsystems instead of just migrating?

I don't think this system is vehemently parallel to Migrate. If anything, it's somewhat parallel to config deployment. When you deploy new config, there are events and hooks for flagging an incompatibility or taking whatever action is required if the deployment is ok. But entity type definitions aren't config: they're code. So we need something that says either:

  • Sorry, can't make this change. You'll need to use Migrate. Or
  • Yep, no problem, I can handle the change (e.g., all I need to do is drop and recreate some indexes, or drop and recreate tables that are currently empty).

For config deployments, we have a dedicated UI for doing this. For code deployments, our current UI is update.php.

plach’s picture

RTBC +1, awesome work @eff, thanks for getting this here!

In reading @fago's reviews I started wondering whether the EM should return stored/installed definitions instead of actual ones: this way code can keep working until changes are applied. However this can be totally addressed in a follow-up, I'm sure there are non-API breaking ways to achieve it, if we agree it's a good change.

@chx:

Aside from what @eff is pointing out, which I totally agree with, I think it's not acceptable to ask people to run a migration every time they install a module that adds a base field. The whole meta is just unifying (and streamlining during the process) how entity/field definitions are handled for base and configurable fields. I know you are concerned by the amount of changes we are doing, but let me state once again that the only ones that are somehow "competing" with Migrate are (almost only) those in this issue. All the rest is a huge API clean-up that we would need anyway to support dynamic table layouts and full consistency between base and configurable fields.

int’s picture

@plach

"it's not acceptable to ask people to run a migration every time they install a module that adds a base field."

+1 on this.

ParisLiakos’s picture

+++ b/core/modules/system/src/Controller/DbUpdateController.php
@@ -285,6 +285,23 @@ protected function selection() {
+    if (\Drupal::service('entity.definition_update_manager')->needsUpdates()) {
...
+      $summary = \Drupal::service('entity.definition_update_manager')->getChangeSummary();

@@ -501,9 +518,18 @@ protected function triggerBatch(Request $request) {
+    if (\Drupal::service('entity.definition_update_manager')->needsUpdates()) {

not important, but if this gets rerolled, lets inject this

chx’s picture

So you expect modules adding base fields as a common thing to happen?? Wow. Just ... wow. OK. Be it.

Berdir’s picture

We've done it a dozen and more times in core already, we just don't have to care about upgrade path yet (like adding a changed field everywhere, renaming language to langcode, making entities revisionable/translatable). Happens all the time.

Changes are very common in contrib projects, they start small and then get new features, new things that can be stored/configured. And unlike core now, most of the time in contrib you do have to care about the upgrade path, nobody is going to run migrations every time they're upgrading contrib modules.

fago’s picture

So you expect modules adding base fields as a common thing to happen?? Wow. Just ... wow. OK. Be it.

Yeah, I agree that's not a rare case. The d7 equivalent of that is adding some db columns to entity base tables what was done with hand-written db update statements, but worked fine.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/lib/Drupal/Core/Entity/EntityDefinitionUpdateManager.php
    @@ -0,0 +1,231 @@
    +  protected function requiresEntityStorageSchemaChanges($entity_type, $original) {
    

    Missing typehints

  2. +++ b/core/lib/Drupal/Core/Entity/EntityDefinitionUpdateManagerInterface.php
    @@ -0,0 +1,63 @@
    + * The entity manager tracks both the current code-defined definitions and the
    + * most recent definitions that entity handlers have had a chance to react to,
    + * and implements \Drupal\Core\Entity\EntityTypeListenerInterface and
    + * \Drupal\Core\Field\FieldStorageDefinitionListenerInterface for bringing the
    + * latter up to date with the former, but makes no decisions about when to
    + * invoke those events. This interface is for managing that.
    

    That is one long sentence. Any chance we can make this a bit simpler to understand?

  3. diff --git a/core/profiles/standard/standard.install b/core/profiles/standard/standard.install
    index 98c279f..c2b5582 100644
    --- a/core/profiles/standard/standard.install
    +++ b/core/profiles/standard/standard.install
    @@ -14,6 +14,12 @@
      * @see system_install()
      */
     function standard_install() {
    +  // Now that all modules are installed, make sure the entity storage and other
    +  // handlers are up to date with the current entity and field definitions. For
    +  // example, Path module adds a base field to nodes and taxonomy terms after
    +  // those modules are already installed.
    +  \Drupal::service('entity.definition_update_manager')->applyUpdates();
    +
    

    How was this working in HEAD?

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
77.93 KB
6.03 KB

Fixes #95 and #99.

Re #99.3, Path module's field uses custom storage (the {url_alias} table managed by system_schema()), so HEAD's lack of notifying the entity handlers of the creation/deletion made no difference. However, as far as standard.install and EntityDefinitionUpdateManager are concerned, it shouldn't care about that: it should report the changes, and allow the handlers to do something or not as they see fit.

Status: Needs review » Needs work

The last submitted patch, 100: entity-schema_manager-2333113-100.patch, failed testing.

effulgentsia’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityDefinitionUpdateManager.php
@@ -130,7 +130,7 @@
-              $this->entityManager->onFieldStorageDefinitionDelete($storage_definitions[$field_name]);
+              $this->entityManager->onFieldStorageDefinitionDelete($original_storage_definitions[$field_name]);

#100 also fixed the above, which I noticed while fixing EntityDefinitionUpdateManager. That #88 passed tests anyway indicates a lack of test coverage for this, but see #77 about why that's the case.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
78.17 KB
923 bytes

Good call on #99.1. That found a bug :)

effulgentsia’s picture

Assigned: Unassigned » alexpott
Status: Needs review » Reviewed & tested by the community

Interdiffs after #99 are minor enough that review by alexpott should be sufficient, so back to RTBC, to get it in front of him.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

In answer to what's in the state vs other key value collections - in general I think state should contain runtime values and values that are used throughout the life of a Drupal install. For 99.9% of a site's life these values won;t be used so a separate collection is a good idea.

Committed 32d5530 and pushed to 8.0.x. Thanks!

  • alexpott committed 32d5530 on 8.0.x
    Issue #2333113 by effulgentsia, plach: Add an...
catch’s picture

In answer to what's in the state vs other key value collections - in general I think state should contain runtime values and values that are used throughout the life of a Drupal install. For 99.9% of a site's life these values won;t be used so a separate collection is a good idea.

General +1 on this.

xjm’s picture

Issue tags: +beta blocker

Retrotag.

dixon_’s picture

yukin’s picture

after unzip the file(drupal-8.0.0-alpha15.tar.gz),found a problem.
problem:
why core/update.php file is not exist?
the file is used in Extend(install modules or update modules).
anyone can help me.

Status: Fixed » Closed (fixed)

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