Problem/Motivation

A couple of issues (#2721313: Upgrade path between revisionable / non-revisionable entities and #2854732: Use initial values for content translation metadata fields and fix existing data) have a need for update path database dumps that contain a test entity type with actual data.

Proposed resolution

  • Move entity_type_update into its own module so we don't have to worry about all the entity types provided by the entity_test module
  • Provide two new test database dumps which contain data for the entity_test_update entity type:
    • drupal-8.0.0-rc1-filled.standard.entity_test_update.php.gz contains 102 non-translated and non-revisioned entities
    • drupal-8.0.0-rc1-filled.standard.entity_test_update_mul.php.gz contains 102 translated and non-revisioned entities

Both database dumps were created on Drupal 8.0.0-rc1 because we should try to support sites that want to upgrade from 8.0.0 directly to 8.4.0 without going through each minor version in between.

Remaining tasks

Review the patch.

User interface changes

Nope.

API changes

Nope.

Data model changes

Nope.

CommentFileSizeAuthor
#18 2856808-17.patch340 KBcatch
#18 2856808-interdiff.txt3.36 KBcatch
#16 2856808-16.patch340 KBcatch
#16 2856808-interdiff.txt3.36 KBcatch
#11 interdiff.txt3.47 KBamateescu
#11 2856808-11.patch340 KBamateescu
#11 2856808-11-for-easy-review.patch27.68 KBamateescu
#8 interdiff.txt4.47 KBamateescu
#8 2856808-8.patch341.91 KBamateescu
#8 2856808-8-for-easy-review.patch27.61 KBamateescu
#6 interdiff.txt2.74 KBamateescu
#5 2856808-5.patch339.58 KBamateescu
#5 2856808-5-for-easy-review.patch26.2 KBamateescu
#2 2856808.patch338.88 KBamateescu
#2 2856808-for-easy-review.patch25.5 KBamateescu

Comments

amateescu created an issue. See original summary.

amateescu’s picture

amateescu commented
Status: Active » Needs review
StatusFileSize
new 2856808-for-easy-review.patch25.5 KB
new 2856808.patch338.88 KB

Here it is :)

The last submitted patch, 2: 2856808-for-easy-review.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 2: 2856808.patch, failed testing.

amateescu’s picture

amateescu commented
Status: Needs work » Needs review
StatusFileSize
new 2856808-5-for-easy-review.patch26.2 KB
new 2856808-5.patch339.58 KB

Right, the problem is that this entity type needs to be used for generating dump from early D8 releases (i.e. 8.0.0-rc1), and ContentEntityBase was not providing base field definitions for entity keys at that time so we need to duplicate most of its code.

This fixes the fails and now I have to try and see if the two db dumps are actually useful for #2721313: Upgrade path between revisionable / non-revisionable entities and #2854732: Use initial values for content translation metadata fields and fix existing data.

amateescu’s picture

amateescu commented
StatusFileSize
new interdiff.txt2.74 KB

Oops, forgot the interdiff.

The last submitted patch, 5: 2856808-5-for-easy-review.patch, failed testing.

amateescu’s picture

amateescu commented
StatusFileSize
new 2856808-8-for-easy-review.patch27.61 KB
new 2856808-8.patch341.91 KB
new interdiff.txt4.47 KB

Finally managed to validate that #2721313: Upgrade path between revisionable / non-revisionable entities works with this patch, after making the following small changes.

The dumps were updated as well but I didn't include them in the interdiff.

Now I'm going to also validate that tests can be written for #2854732: Use initial values for content translation metadata fields and fix existing data using the dumps added here.

Status: Needs review » Needs work

The last submitted patch, 8: 2856808-8.patch, failed testing.

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
commented
Status: Needs work » Reviewed & tested by the community

Looks good. Going straight from Needs Word to RTBC because it did actually pass in #8.

+++ b/core/modules/system/tests/modules/entity_test_update/entity_test_update.module
@@ -0,0 +1,171 @@
+    throw new \Exception('Peekaboo!');

Ha!

amateescu’s picture

amateescu commented
StatusFileSize
new 2856808-11-for-easy-review.patch27.68 KB
new 2856808-11.patch340 KB
new interdiff.txt3.47 KB

Rerolled for the array syntax conversion and also realized that I didn't delete the existing EntityTestUpdate class in the latest patches and that we should mark some of its fields storages as required in anticipation for #2841291: Fix NOT NULL handling in the entity storage and 'primary key' changes when updating the storage definition of an identifier field.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 11: 2856808-11.patch, failed testing.

amateescu’s picture

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

Most likely a bot fluke, let's try again.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 11: 2856808-11.patch, failed testing.

amateescu’s picture

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

And again :)

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new 2856808-interdiff.txt3.36 KB
new 2856808-16.patch340 KB

Didn't pass coding standards checks, uploading a patch that should.

Status: Needs review » Needs work

The last submitted patch, 16: 2856808-16.patch, failed testing.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Needs review
StatusFileSize
new 2856808-interdiff.txt3.36 KB
new 2856808-17.patch340 KB

Well.

amateescu’s picture

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

Thanks, @catch :)

The first change doesn't look right to me but if that's what the coding standards say..

  • catch committed 4d92ef4 on 8.4.x 
    Issue #2856808 by amateescu, catch: Break out the 'entity_test_update'...
catch’s picture

catch
he/him
Primary language English
commented
Version: 8.4.x-dev » 8.3.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed/pushed to 8.4.x, thanks!

I think we should cherry-pick this to 8.3.x so that any changes don't need porting between branches, but waiting for a +1 from another committer.

Fixed this on commit:

FILE: ...tests/modules/entity_test_update/src/Entity/EntityTestUpdate.php
----------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 1 LINE
----------------------------------------------------------------------
 94 | ERROR | [x] No space found after comma in function call
 94 | ERROR | [x] Expected one space after the comma, 0 found
----------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

I agree that this should be cherry-picked to 8..3.x - whilst there might be some disruption to contrib tests it is just tests and maintaining different tests across 8.3.x and 8.4.x is more painful than 8.2.x and 8.3.x/8.4.x since 8.2.x is about to be obsolete.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented

+1 to #22.

catch’s picture

catch
he/him
Primary language English
commented
Status: Patch (to be ported) » Fixed

Cherry-picked to 8.3.x, thanks!

  • catch committed c6b7207 on 8.3.x 
    Issue #2856808 by amateescu, catch: Break out the 'entity_test_update'...

Status: Fixed » Closed (fixed)

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