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

CommentFileSizeAuthor
#5 field_table_names-tests-2079013-5.patch5.4 KByched
#5 interdiff.txt1.27 KByched
#3 field_table_names-tests-2079013-3.patch4.83 KByched

Comments

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Active
berdir’s picture

berdir
Primary language German
Location Switzerland
commented
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 https://drupal.org/node/2078507, this is just about the first part then.

yched’s picture

yched commented
Issue summary: View changes

edit

yched’s picture

yched commented
Status: Active » Needs review
StatusFileSize
new field_table_names-tests-2079013-3.patch4.83 KB

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

berdir
Primary language German
Location Switzerland
commented 
+++ 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

yched commented
StatusFileSize
new interdiff.txt1.27 KB
new field_table_names-tests-2079013-5.patch5.4 KB

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

yched commented

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

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
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

catch
he/him
Primary language English
commented
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

(not verified) commented
Issue summary: View changes

formatting