Problem/Motivation
Currently when you have a revisionable entity type with a non-revisionable field storage definition requiring a dedicated table, the (default) table mapping returns no dedicated revision table, while the storage and storage schema handlers expect the revision table to be there and try to read from/write to it.
This is bad, because the more we start relying on the table mapping to write schema-agnostic code, the more this discrepancy will bite us. Here's a prime example: #3056539: Updating an entity type from non-revisionable to revisionable fails if it has non-revisionable fields stored in dedicated tables.
Proposed resolution
Use the same logic across the table mapping and the SQL storage/schema. Since reconciling the two behaviors is a BC break in either direction, we will need a BC layer and some form of deprecation.
We need to figure out whether we want the table mapping to behave as the SQL storage/schema or viceversa. Given that it’s perfectly legal to have a non-revisionable field on a revisionable entity type and that in such case there is absolutely no point in having a revision table, since it would be a straight copy of the data table, it's probably more correct to extend the table mapping logic to the SQL schema, however this is likely to require a more complex BC layer, as people might be relying on the existence of revision tables. In this case we'd likely be deprecating the existence of revision field tables for non-revisionable fields on revisionable entity types without actually dropping them.
Remaining tasks
- Decide how to reconcile the two behaviors
- Write a patch
- Reviews
User interface changes
None
API changes
TBD
Data model changes
TBD
Release notes snippet
TBD
| Comment | File | Size | Author |
|---|---|---|---|
| #2 | 3113639-2.patch | 8.75 KB | plach |
Comments
Comment #2
plachHere's a start at making things work the table mapping way.
Comment #3
plachClosed #2083451: Reconsider the separate field revision data tables as obsolete.
Comment #4
plachMore accurate title/IS
Comment #6
catchIn terms of the bc layer, seems there are two ways to inform people, trying to think through what it might look like:
1. A hook_requirements() that detects fields with the wrong schema, and warns the site administrator.
2. A deprecation error triggered by the bc layer itself.
We could then link to instructions informing people how to fix the schema via a hook_update_N() for one field at a time, or to mark the field as revisionable in the field definition if that's what they wanted after all.
Then when we want to remove that bc layer, seems like there are two options:
1. A hook_requirements('update') that prevents sites from updating unless they either update the schema, or set the field to revisionable.
2. A hook_update_N() that changes the schema for any fields it finds.
Comment #13
acbramley commentedPopped up in BSI triage, it seems like this is still an issue based on the code in HEAD. Would be good to have some steps to reproduce this bug.