Better test coverage for field table names [#2079013]

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

Comment	File	Size	Author
#5	field_table_names-tests-2079013-5.patch	5.4 KB	yched
#5
#5	interdiff.txt	1.27 KB	yched
#3	field_table_names-tests-2079013-3.patch	4.83 KB	yched
#3

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

Berdir

German

Switzerland

CreditAttribution: Berdir commented 1 September 2013 at 21:51

Status:

Needs review

» Active

Comment #2

Berdir

German

Switzerland

CreditAttribution: Berdir commented 1 September 2013 at 22:59

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.

Comment #2.0

yched CreditAttribution: yched commented 4 September 2013 at 21:25

Issue summary:

View changes

edit

Comment #3

yched CreditAttribution: yched commented 4 September 2013 at 23:03

Status:

Active

» Needs review

File	Size
field_table_names-tests-2079013-3.patch	4.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.

Comment #4

Berdir

German

Switzerland

CreditAttribution: Berdir commented 5 September 2013 at 21:25

+++ 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.

Comment #5

yched CreditAttribution: yched commented 6 September 2013 at 14:59

File	Size
interdiff.txt	1.27 KB
field_table_names-tests-2079013-5.patch	5.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.