The BlockContent entity type does not specify its revision data table [#2858109]

Comment	File	Size	Author
#7	2858109-7.patch	7.02 KB	amateescu
8.3.x: PHP 5.6 & MySQL 5.5 21,435 pass
#7	2858109-7-test-only.patch	2.54 KB	amateescu
8.3.x: PHP 5.6 & MySQL 5.5 21,426 pass, 1 fail
#5	interdiff.txt	1.81 KB	amateescu
#5	2858109-5.patch	3.34 KB	amateescu
8.3.x: PHP 5.6 & MySQL 5.5 21,426 pass
#2	2858109.patch	1.53 KB	amateescu
8.3.x: PHP 5.6 & MySQL 5.5 21,406 pass, 23 fail

Comment #1

6 March 2017 at 13:41

amateescu created an issue. See original summary.

Log in or register to post comments

Comment #2

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented 6 March 2017 at 13:41

Status:

Active

» Needs review

File	Size
2858109.patch	1.53 KB
8.3.x: PHP 5.6 & MySQL 5.5 21,406 pass, 23 fail

Let's fix it.

Log in or register to post comments

Comment #3

jibran

he/him

Urdu

Sydney, Australia

CreditAttribution: jibran as a volunteer commented 6 March 2017 at 13:58

Let's add some tests.

Log in or register to post comments

Comment #4

6 March 2017 at 17:15

Status:

Needs review

» Needs work

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

Log in or register to post comments

Comment #5

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented 6 March 2017 at 19:22

Status:

Needs work

» Needs review

File	Size
2858109-5.patch	3.34 KB
8.3.x: PHP 5.6 & MySQL 5.5 21,426 pass
interdiff.txt	1.81 KB

The problem discovered by this patch is that if an entity type doesn't specify a base table in its definition, the entity storage will create it anyway because it needs it but the entity type definition can no longer be updated to include the table name.

This is one way of fixing it but I'm not entirely happy with it :/

Log in or register to post comments

Comment #6

jibran

he/him

Urdu

Sydney, Australia

CreditAttribution: jibran as a volunteer commented 7 March 2017 at 02:56

I still think we should add a test to check that entity definition is updated after running update hook.

Log in or register to post comments

Comment #7

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented 7 March 2017 at 10:23

File	Size
2858109-7-test-only.patch	2.54 KB
8.3.x: PHP 5.6 & MySQL 5.5 21,426 pass, 1 fail
2858109-7.patch	7.02 KB
8.3.x: PHP 5.6 & MySQL 5.5 21,435 pass

Ok. Since update path tests are expensive, let's re-use the existing one and add a couple of assertions to it.

The test-only patch is also the interdiff.

Log in or register to post comments

Comment #8

7 March 2017 at 13:15

The last submitted patch, 7: 2858109-7-test-only.patch, failed testing.

Log in or register to post comments

Comment #9

jibran

he/him

Urdu

Sydney, Australia

CreditAttribution: jibran as a volunteer commented 7 March 2017 at 15:57

Status:

Needs review

» Reviewed & tested by the community

Awesome!

Log in or register to post comments

Comment #10

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott at Chapter Three commented 14 March 2017 at 11:23

Status:

Reviewed & tested by the community

» Fixed

Committed and pushed 85860ba to 8.4.x and c1eaa26 to 8.3.x. Thanks!

diff --git a/core/modules/block_content/block_content.install b/core/modules/block_content/block_content.install
index 97c3362..6af2ac4 100644
--- a/core/modules/block_content/block_content.install
+++ b/core/modules/block_content/block_content.install
@@ -63,11 +63,6 @@ function block_content_update_8003() {
 }
 
 /**
- * @addtogroup updates-8.3.x
- * @{
- */
-
-/**
  * Fix the block_content entity type to specify its revision data table.
  */
 function block_content_update_8300() {
@@ -77,7 +72,3 @@ function block_content_update_8300() {
   $definition_update_manager->updateEntityType($entity_type);
 
 }
-
-/**
- * @} End of "addtogroup updates-8.3.x".
- */

We've just removed all of these. See #2860096: Remove api doc groups for updates eg. updates-8.2.x-to-8.3.x

Log in or register to post comments

Comment #11

14 March 2017 at 11:23

alexpott committed 85860ba on 8.4.x

Issue #2858109 by amateescu: The BlockContent entity type does not...

Log in or register to post comments

Comment #12

14 March 2017 at 11:23

alexpott committed c1eaa26 on 8.3.x

Issue #2858109 by amateescu: The BlockContent entity type does not...

Log in or register to post comments

Comment #13

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott at Chapter Three commented 14 March 2017 at 11:23

Should we file a follow-up to help custom entities not get into the same position?

Log in or register to post comments

Comment #14

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented 14 March 2017 at 12:24

Well, we can't write update functions for custom entities and the changing the storage to not create the tables automatically if they're not defined in the entity definition would be an API change..