Problem/Motivation

The code from SqlContentEntityStorageSchemaConverter was somewhat generic but mostly targeted for converting an entity type from non-revisionable to revisionable, and it had a big flaw: it was relying on live (in-code) entity type and field storage definitions for the conversion process.

We managed to change that code to be completely agnostic of the conversion type and not use the live definitions anymore in #2984782: Add an API for converting entity type schemas to (non-)revisionable/(non-)translatable, with or without data, so SqlContentEntityStorageSchemaConverter does not have any purpose to exist anymore :)

Proposed resolution

Make SqlContentEntityStorageSchemaConverter a slim wrapper around the new API for converting entity type schemas.

Remaining tasks

- make SqlContentEntityStorageSchemaConverter use the new API from #2984782: Add an API for converting entity type schemas to (non-)revisionable/(non-)translatable, with or without data internally
- mark it as deprecated
- rework its test coverage to test the new API instead

User interface changes

Nope.

API changes

Nope.

Data model changes

Nope.

CommentFileSizeAuthor
#14 interdiff-14.txt2.91 KBamateescu
#14 3004642-14.patch44.85 KBamateescu
#10 interdiff-10.txt3.49 KBamateescu
#10 3004642-10.patch43.01 KBamateescu
#8 interdiff-8.txt17.76 KBamateescu
#8 3004642-8.patch42.16 KBamateescu
#7 interdiff-7.txt5.6 KBamateescu
#7 3004642-7.patch24.4 KBamateescu
#5 3004642-5.patch20.63 KBamateescu
#3 interdiff-3.txt789 bytesamateescu
#3 3004642-3.patch20.63 KBamateescu
#2 deprecate_entity_schema_converter.patch20.62 KBamateescu

Comments

amateescu created an issue. See original summary.

amateescu’s picture

amateescu commented
Title: Deprecate SqlContentEntityStorageSchemaConverter in favor of the new API provided by EntityTypeSchemaListenerInterface » [PP-1] Deprecate SqlContentEntityStorageSchemaConverter in favor of the new API provided by EntityTypeSchemaListenerInterface
Issue summary: View changes
Status: Active » Postponed
StatusFileSize
new deprecate_entity_schema_converter.patch20.62 KB

This patch is split from #2984782-15: Add an API for converting entity type schemas to (non-)revisionable/(non-)translatable, with or without data and does the first item from the list of remaining tasks.

amateescu’s picture

amateescu commented
StatusFileSize
new 3004642-3.patch20.63 KB
new interdiff-3.txt789 bytes

Updated the patch for the changes in #2984782-39: Add an API for converting entity type schemas to (non-)revisionable/(non-)translatable, with or without data.

\Drupal\Tests\system\Functional\Entity\Update\SqlContentEntityStorageSchemaConverterTranslatableTest::testMakeRevisionableErrorHandling() needs some work because entity_test_update_entity_presave() is not invoked anymore.

amateescu’s picture

amateescu commented
Title: [PP-1] Deprecate SqlContentEntityStorageSchemaConverter in favor of the new API provided by EntityTypeSchemaListenerInterface » [PP-1] Deprecate SqlContentEntityStorageSchemaConverter in favor of the new API provided by EntityTypeListenerInterface::onFieldableEntityTypeUpdate()
amateescu’s picture

amateescu commented
Title: [PP-1] Deprecate SqlContentEntityStorageSchemaConverter in favor of the new API provided by EntityTypeListenerInterface::onFieldableEntityTypeUpdate() » Deprecate SqlContentEntityStorageSchemaConverter in favor of the new API provided by EntityTypeListenerInterface::onFieldableEntityTypeUpdate()
Status: Postponed » Needs review
StatusFileSize
new 3004642-5.patch20.63 KB

The blocker is in, let's run this through the testbot.

Status: Needs review » Needs work

The last submitted patch, 5: 3004642-5.patch, failed testing. View results

amateescu’s picture

amateescu commented
Status: Needs work » Needs review
StatusFileSize
new 3004642-7.patch24.4 KB
new interdiff-7.txt5.6 KB

Fixed the fails and cleaned up a few more things.

amateescu’s picture

amateescu commented
StatusFileSize
new 3004642-8.patch42.16 KB
new interdiff-8.txt17.76 KB

And, as promised in #2984782-53: Add an API for converting entity type schemas to (non-)revisionable/(non-)translatable, with or without data (point 2), moved the error handling test coverage to the new test class.

This is now ready for final reviews :)

The last submitted patch, 7: 3004642-7.patch, failed testing. View results

amateescu’s picture

amateescu commented
StatusFileSize
new 3004642-10.patch43.01 KB
new interdiff-10.txt3.49 KB

The fail from #7 is a very good example why we don't want to run arbitrary hooks during the 'restore' process :/

amateescu’s picture

amateescu commented

Also opened #3030086: Clean up content_translation_entity_presave() for the bug discovered by #7.

The last submitted patch, 8: 3004642-8.patch, failed testing. View results

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented
Status: Needs review » Needs work

Looks good! I found only a minor issue:

+++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchemaConverter.php
@@ -2,23 +2,17 @@
+ * @deprecated in Drupal 8.7.0, will be removed before Drupal 9.0.0.
+ *   Use \Drupal\Core\Entity\EntityDefinitionUpdateManagerInterface::updateFieldableEntityType()
+ *   instead.

Missing @trigger_error + deprecation test.

Also, missing CR reference.

amateescu’s picture

amateescu commented
Issue summary: View changes
Status: Needs work » Needs review
StatusFileSize
new 3004642-14.patch44.85 KB
new interdiff-14.txt2.91 KB

Good points, done!

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented
Status: Needs review » Reviewed & tested by the community

Cool, thanks!

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented
Priority: Normal » Major

This is semi-blocking #2976035-59: Entity type CRUD operations must use the last installed entity type and field storage definitions , so it is at very least major.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 14: 3004642-14.patch, failed testing. View results

amateescu’s picture

amateescu commented
Status: Needs work » Reviewed & tested by the community

Random test fail.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed 57de633 and pushed to 8.7.x. Thanks!

  • catch committed 57de633 on 8.7.x 
    Issue #3004642 by amateescu, plach: Deprecate...
plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented

Thanks!

Status: Fixed » Closed (fixed)

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