Problem/Motivation

I have a non-translatable and non-revisonable entity type that I want to convert into a non-translatable and revisionable entity type. When I use the SqlContentEntityStorageSchemaConverter to convert my entity type, I get an error, Unknown field revision_translation_affected.

Proposed resolution

Check if the entity type is translatable before using the revision_translation_affected entity key.

Remaining tasks

Tests.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

benjy created an issue. See original summary.

benjy’s picture

Status: Active » Needs review
FileSize
938 bytes

Here's a patch that fixes the error.

One other issue I noticed is that revision_user and revision_created don't get a default value from the original entity?

benjy’s picture

FileSize
1.16 KB

Something like this fixes that for me.

Sam152’s picture

+++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchemaConverter.php
@@ -291,7 +291,9 @@ protected function copyData(array &$sandbox) {
+        if ($original_entity_type->isTranslatable()) {

Can we check if the $revision_translation_affected_key field exists instead? Do all translatable entities have this?

I suppose based on #2896845: Provide the 'revision_translation_affected' base field by default for all revisionable and translatable entity types, this isn't a problem for 5.x, but possibly could if this is backported as a bugfix?

Sam152’s picture

Issue tags: +Needs tests

It would also be good to run the upgrade path on a non-translatable entity and see what happens.

benjy’s picture

I expected this to pass, it fails but i'm not sure it's for the correct reason. The null field in ContentEntityBase that blows up is default_langcode not sure if i'm doing something wrong in the test.

Status: Needs review » Needs work

The last submitted patch, 6: 2919157-6-FAIL.patch, failed testing. View results

amateescu’s picture

@benjy, that's probably because SqlContentEntityStorageSchemaConverterTest uses drupal-8.0.0-rc1-filled.standard.entity_test_update_mul.php.gz as the test database dump, which contains a translatable entity type.

A test for this bug would need to use drupal-8.0.0-rc1-filled.standard.entity_test_update.php.gz instead :)

Also, I agree with #4, let's switch to an hasKey() check on the entity type.

benjy’s picture

Status: Needs work » Needs review
FileSize
3.04 KB
3.99 KB

Here's another attempt at a test only patch, it fails for the right reason but you can only see that if you manually tweak the exception thrown in copyData() on line 314 to include the original exception message.

amateescu’s picture

Great, I'm glad that #8 helped you :)

  1. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchemaConverter.php
    @@ -291,7 +291,9 @@ protected function copyData(array &$sandbox) {
    +        if ($original_entity_type->hasKey($revision_translation_affected_key)) {
    

    We need to use the temporary entity type here, and the check needs to be on the 'revision_translation_affected' string, not the variable defined outside of the foreach.

  2. +++ b/core/modules/system/tests/src/Functional/Entity/Update/SqlContentEntityStorageSchemaConverterNotTranslatableTest.php
    @@ -0,0 +1,91 @@
    +  public function testMakeRevisionableNotTranslatable() {
    ...
    +    $this->runUpdates();
    

    We should also check the field values like we do in the other test, which probably means we need an abstract base test class for these two tests in order to share at least that bit of code between them.

benjy’s picture

Sorry @amateescu I was meant to say thanks for the help in #8, I got distracted and then came back and just hit post.

The test doesn't pass when using the temporary_entity_type, is that definitely right? I've left as original entity type for now which should pass.

  1. Fixed the entity key to use the string.
  2. Copied across the relevant assertions, we didn't want all of translations ones. We can probably still have a base class to share some of these assertions.

The last submitted patch, 9: 2919157-9-FAIL.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

amateescu’s picture

Status: Needs review » Needs work
Issue tags: -Needs tests

@benjy, yes, using the temporary entity type is definitely the right thing to do. However, in this case the 'revision_translation_affected' entity key is always defined, even for non-translatable entity types, so the solution here would be change the if check to:

if ($temporary_entity_type->isTranslatable()) {

I tested locally and the test passes with that change :)

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

amateescu’s picture

Version: 8.6.x-dev » 8.5.x-dev
Category: Task » Bug report
Status: Needs work » Needs review
FileSize
16.94 KB
17.85 KB
27.98 KB

Implemented the change proposed in #13 and completed the test changes for #10.2.

Also attaching a test-only patch to show that new test coverage with a base class still catches the bug properly.

The last submitted patch, 15: 2919157-15-test-only.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 15: 2919157-15.patch, failed testing. View results

amateescu’s picture

Status: Needs work » Needs review
FileSize
17.85 KB
723 bytes

Oops :)

Sam152’s picture

One comment, one style thing and one clarification. If the last point is clarified, this looks great and can be RTBC.

  1. +++ b/core/modules/system/tests/src/Functional/Entity/Update/SqlContentEntityStorageSchemaConverterNonTranslatableTest.php
    @@ -0,0 +1,23 @@
    +/**
    + * Tests converting a non-translatable entity type with data to revisionable.
    + *
    + * @group Entity
    + * @group Update
    + */
    +class SqlContentEntityStorageSchemaConverterNonTranslatableTest extends SqlContentEntityStorageSchemaConverterTestBase {
    
    +++ b/core/modules/system/tests/src/Functional/Entity/Update/SqlContentEntityStorageSchemaConverterTranslatableTest.php
    @@ -0,0 +1,123 @@
    +/**
    + * Tests converting a translatable entity type with data to revisionable.
    + *
    + * @group Entity
    + * @group Update
    + */
    +class SqlContentEntityStorageSchemaConverterTranslatableTest extends SqlContentEntityStorageSchemaConverterTestBase {
    

    Okay, so the same test with a key difference: different test fixtures. The best docs for the characteristics of each fixture were on the docblock of _entity_test_update_create_test_entities. I was somewhat confused why we weren't just using the state based entity overrides, but each fixture does a whole lot more, nice!

  2. +++ b/core/modules/system/tests/src/Functional/Entity/Update/SqlContentEntityStorageSchemaConverterTestBase.php
    @@ -92,8 +86,10 @@ public function testMakeRevisionable() {
    -    $field_storage_definitions = $this->lastInstalledSchemaRepository->getLastInstalledFieldStorageDefinitions('entity_test_update');
    -    $this->assertTrue(isset($field_storage_definitions['revision_translation_affected']));
    +    if ($translatable) {
    +      $field_storage_definitions = $this->lastInstalledSchemaRepository->getLastInstalledFieldStorageDefinitions('entity_test_update');
    +      $this->assertTrue(isset($field_storage_definitions['revision_translation_affected']));
    +    }
    

    Could we assert the negative case here too? $this->assertEquals($translateable, isset(...

    We obviously get implicit coverage of this because of the original error from the IS, but it makes the code a bit nicer too I think.

  3. +++ b/core/modules/system/tests/src/Functional/Entity/Update/SqlContentEntityStorageSchemaConverterTranslatableTest.php
    @@ -0,0 +1,123 @@
    +  /**
    +   * Tests that a failed "make revisionable" update preserves the existing data.
    +   */
    +  public function testMakeRevisionableErrorHandling() {
    

    Is there a reason this test isn't applicable to the non-translatable entity type as well? Is that just out of scope here, given the restructured tests provide coverage for the bug in the IS and don't touch the error handling?

amateescu’s picture

Thanks for the review!

Re #19:
1. Yep, those fixtures were quite hard to create, and I had to document the process otherwise no one else would be able to touch them in the future :)

2. That's very clever, I hope I understood your point correctly in this interdiff.

3. The problem with that test method is that it asserts entity UUIDs, and those are different in the two fixtures. So I thought that having explicit if/else blocks is not really worth it :)

+++ b/core/modules/system/tests/src/Functional/Entity/Update/SqlContentEntityStorageSchemaConverterTranslatableTest.php
@@ -0,0 +1,123 @@
+    $this->assertEqual('843e9ac7-3351-4cc1-a202-2dbffffae21c', $base_table_row[1]->uuid);
Sam152’s picture

Status: Needs review » Reviewed & tested by the community

Nice, this looks great to me.

  • catch committed 7096493 on 8.6.x
    Issue #2919157 by benjy, amateescu, Sam152: Unable to convert a non-...

  • catch committed 0d2b0ce on 8.5.x
    Issue #2919157 by benjy, amateescu, Sam152: Unable to convert a non-...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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