Select from correct entity tables for additional entity/field definitions when moving data to dedicated table storage from shared table storage for the purposes of purging

Seeing if this passes the existing tests.

Thanks for reviewing. I'll have a look at the test for this now.

Good catch, updating.

Adding a test for this too.

Fixing the views test.

Removing unused use.

I missed the comment in #5, $is_translatable was coming up FALSE because I believe the field was not translatable.

The code snippet in the issue summary is actually incorrect. Must have pulled that from somewhere else. Updating.

I'm not sure how the field is assigned to the tables, however the test case fails in the same fashion as I encountered. The entity_test_mul table is used however I believe the base field ends up in entity_test_mul_property_data.

Some good additional coverage might be additional entity revisions in testBaseFieldDeleteWithExistingData or would you suggest a new test method?

Adding the revisions instantly exposed a bug in #13, so great catch to check this. To fix this part I think it makes sense to add the equivalent method to getFieldTableName for revisions.

I think it also allows us to add coverage for another scenario which I also think has issues, a non-revisionable base field on a revisionable entity type. That's based on the following part of the patch:

      if ($this->entityType->isRevisionable()) {
        $dedicated_revision_table_name = $table_mapping->getDedicatedRevisionTableName($deleted_storage_definition, TRUE);
        ...
      }
...
        if (isset($dedicated_revision_table_name)) {
          $revision_table = $is_translatable ? $this->storage->getRevisionDataTable() : $this->storage->getRevisionTable();

In this case I don't think the field appears in either getRevisionDataTable or getRevisionTable because it's not revisionable, but the condition is based on the entity type.

I haven't had much luck debugging these and it's getting a bit late but I think it makes sense to include these test cases in the scope of this issue and rejigging the title and IS. Feel free to adjust for brevity, the title got a bit wordy.

Helps to upload the patch files...

I was able to get the test cases I introduced to pass with this approach.

And a test only patch for #23.

Reroll and a few tweaks.

1. Is there a test entity type that meets this criteria we can use?
2. We create an additional entity in #2923572: Account for NULL base field values in shared table storage when copying to dedicated table storage for field purging., so the revision IDs in these test cases are 3 and 2.
3. Good point.

1. Both new test cases indeed failed!
2. Hard coding both revision IDs seems clearer to me.
3. Fixed.

I experimented with a few ways of representing the list tables. I'm not sure my matrix implementation makes it any more understandable :D

$tables = [
  [$this->storage->getBaseTable(), $this->storage->getRevisionTable()],
  [$this->storage->getDataTable(), $this->storage->getRevisionDataTable()],
];
$revision_table = $tables[$this->entityType->isTranslatable()][$storage_definition->isRevisionable()];

Rerolling.

Had a look into this. I can confirm for the Revisionable entity type, non revisionable base field test case, a deleted revision and data and revision table exist, with a single row for both. Not sure if that is intentional or was just easier to implement by design. Maybe @amateescu can help out here.

Dug into this a little bit deeper. In SqlContentEntityStorageSchema::getDedicatedTableSchema, the code which checks if a revision table is created for some FieldStorageDefinitionInterface is:

    if ($entity_type->isRevisionable()) {
      ...
      $dedicated_table_schema += [$table_mapping->getDedicatedRevisionTableName($storage_definition) => $revision_schema];
    }

So, I think the answer is that currently if a field requires dedicated table storage and is not revisionable but the entity type it is attached to is, field revision tables are created. This could be so that in ::updateDedicatedTableSchema, table layouts don't have to be changed if a field is changed from revisionable to non-revisionable and back again?

Perhaps we could detect in getDedicatedTableSchema if the schema was being created for a previously shared table field which had been deleted, and was thus unlikely to change from non-revisionable to revisionable. In that sense we'd have less tables but also more complexity, at which point I'd ask, is it worth it? Perhaps worth further discussion in a follow up?

Here is an attempt at this. Do we purge field data for all revisions of a field or only the default?
I had a read through SqlContentEntityStorage::readFieldItemsToPurge, but am a little out of my depth.

Not confident this is the right approach to take or even why this works.

Thanks @plach and @amateescu! Looks good to me.

Congrats on the first commit @plach! 🎉