Follow-up from #1497374: Switch from Field-based storage to Entity-based storage.

From me in comment #232:

- We should have unit test coverage for _generateFieldTableName() (and other things, when possible), as this issue has shown, it's not trivial code, so it would be good to hit it with all kinds of combinations and long fields and entity type names to verify that there can be no clashes or other bugs.

From @catch in comment #236:

What I didn't see in the patch ...

Separate issue: #2078507: Add explicit upgrade path tests for fields shared between entities

#5 field_table_names-tests-2079013-5.patch5.4 KByched
PASSED: [[SimpleTest]]: [MySQL] 58,880 pass(es). View
#5 interdiff.txt1.27 KByched
#3 field_table_names-tests-2079013-3.patch4.83 KByched
PASSED: [[SimpleTest]]: [MySQL] 58,735 pass(es). View
Members fund testing for the Drupal project. Drupal Association Learn more


Berdir’s picture

Status: Needs review » Active
Berdir’s picture

Title: Better test coverage for field table names and upgrade path with fields shared on multiple entity types. » Better test coverage for field table names
Priority: Critical » Normal

Looks like @catch already opened, this is just about the first part then.

yched’s picture

Issue summary: View changes


yched’s picture

Status: Active » Needs review
4.83 KB
PASSED: [[SimpleTest]]: [MySQL] 58,735 pass(es). View

This adds some tests. Note that the existing FieldSqlStorageTest::testLongNames() already tests a case for non clashing.

Also note that the moment DatabaseStorageController::_generateFieldTableName() starts truncating stuff, it also puts a hash of the field UUID in the table name, so there *really* can't be a clash - which is why I'm not multiplying test combinations here, there are not really combinations that make more sense to test than others...

Patch does change one thing: truncate the entity type at the same length for the "current data" and the "revision data" tables, because having the two tables at different prefixes is confusing.

Berdir’s picture

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/FieldSqlStorageTest.php
@@ -459,4 +459,73 @@ function testFieldSqlStorageForeignKeys() {
+    $expected = 'short_entity_type_revision__short_field_name';
+    $expected = 'short_entity_type_r__' . substr(hash('sha256', $field->uuid), 0, 10);

Still consider it a bit weird that we do that r/revision thing, I think in most cases, the problem will be long field names, not entity types name, so we'll end up with way shorter field names anyway. Anyway, don't care *that* much :)

A test for a deleted field would be nice, do we already have unit test coverage for that?

This would be a nice case for a PHPUnit test, where we could use a data provider for the different versions and mock the field object. That would however require that we switch to method calls for $field so that we can actually mock it and not too sure about splitting test coverage like that.

yched’s picture

1.27 KB
5.4 KB
PASSED: [[SimpleTest]]: [MySQL] 58,880 pass(es). View

Not that I care too much either, but If we keep _revision, we still want to truncate the entity_type to the same (shorter - 27 instead of 34) length for the current and revision tables.

But IMO we want to reconsider:
- not always creating the 'revision' tables if the entity type is not revisionable
so on non-revisionable entities, we'd truncate more than needed to account for a revision table that won't exist.
- maybe drop the separate revision tables completely, as several people pointed they are more a pain than a gain
[edit: opened #2083451: Reconsider the separate field revision data tables]

So I'd rather leave it as is here for now, and just add the tests for the current behavior :-)

Added a test for a deleted field.

yched’s picture

Also: yes, a PHPUnit test seems out of reach for now.

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Yeah, reading those expected outputs got me thinking again :)

Agreed, this issue is to add test coverage for the existing behavior, so let's get that in.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

Status: Fixed » Closed (fixed)

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

Anonymous’s picture

Issue summary: View changes