If you have an entity type that is revisionable and has a non-revisionable base field with multiple columns \Drupal\Core\Entity\Sql\SqlContentEntityStorage::loadFromSharedTables() will fatal making it impossible to load the entity.
The offending code is:
// Find revisioned fields that are not entity keys. Exclude the langcode
// key as the base table holds only the default language.
$base_fields = array_diff($table_mapping->getFieldNames($this->baseTable), array($this->langcodeKey));
$fields = array_diff($table_mapping->getFieldNames($this->revisionDataTable), $base_fields);
// Find fields that are not revisioned or entity keys. Data fields have
// the same value regardless of entity revision.
$data_fields = array_diff($table_mapping->getFieldNames($this->dataTable), $fields, $base_fields);
if ($data_fields) {
$fields = array_merge($fields, $data_fields);
$query->leftJoin($this->dataTable, 'data', "(revision.$this->idKey = data.$this->idKey)");
$query->fields('data', $data_fields);
}
The important detail here is that $table_mapping->getFieldNames() returns a list of entity field names, not database field (i.e. column) names.
$table_mapping->getColumnNames($field_name) should be used to convert the list of field names into a list of column names or $table_mapping->getAllColumns() should be used directly to get a list of column names.
| Issue category | Bug because a fatal error is triggered from e.g. MyEntityType::load() |
|---|---|
| Issue priority | Major because there is a fatal error and this is an integral part of Drupal 8's API and there is no workaround |
| Prioritized changes | Prioritized because it is a bug fix |
| Comment | File | Size | Author |
|---|---|---|---|
| #20 | non_revisionable_base-2506805-20.patch | 6.43 KB | jibran |
Comments
Comment #1
tstoecklerNote that this code was last touched in #2431329: Make (content) translation language available as a field definition, but it was just moved around there. I didn't dig any further to find where we introduced this.
Comment #2
tstoecklerI just realized fields have to be non-revisionable to reproduce this.
Updating issue summary and title.
Comment #3
jhedstromI looked into writing a test and realized I don't fully understand the issue as described above. The Redirect module has non-revisionable base fields with multiple columns and works as expected.
Comment #4
berdirredirect however isn't revisionable itself. This only occurs with entity types that have revision tables and some revisionable fields.
Comment #6
jibranI faced this issue in #2773099: Add tests for base fields of revisonable entity.
Comment #7
jibranIt was happening with DER so I treid ER with same test case scenario and it passes because it has only one column but when I used ShapeItem as a base field then it's a different story.
Comment #8
jibranHere is a complete patch.
Comment #12
jibranHere is the green patch with the fix.
Comment #13
jibranMinor doc update and removed useless changes.
Comment #18
chx commentedLet's step back and consider what caused the bug. I nominate lack of comments. The code fixing it is correct but I believe this is a very hard to understand, rarely used piece of code and as such very heavy commenting is in order. The existing amount of very useful comments in loadFromSharedTables concur but we need even more.
Well, if that's the case then why not add it to the getFieldNames doxygen?
So loadFromSharedTables has similar sounding, somewhat explained variables -- but $fields is not explained at all:
Oh god. So the first are $key_fields. Then it's revisionable fields. Or non_key_fields? I am not sure what goes into the revision data table but $fields is an extremely poor name. Please check and rename. And then $data_fields, did you mean non_revisionable_fields? And then why do we merge these two? While I did delete some comments on copy paste, there's no comment for this. I think this would be a wonderful time to add a number of lines to the SqlContentEntityStorageSchema doxygen about what stores what and refer to it from here.
$column_names = array_merge($column_names, array_values($table_mapping->getColumnNames($data_field)));this needs a comment reminding the dear reader that getColumNames is keyed by property and we want to collect them all like so many pokemon so we want numeric keys instead which array_merge will then renumber...Comment #19
jibranThank you the review @chx and I appreciate you being brutally honest about the doxygen issue. :-)
Here is a new patch I hope this addresses your concerns.
Comment #20
jibranUpdated the var name as per @chx suggestion.
Comment #21
chx commentedThis is lovely. Sooner or later both jibran and I will understand the field mapping system and hopefully others too :)
Comment #22
alexpottCommitted and pushed 28356f4e9fbeaf94a40ffeca479c40e2c77906ce to 8.2.x and fc06148 to 8.1.x. Thanks!
Nice comments and yes it is easy to see how we got this wrong before.
Comment #25
jibranThanks @alexpott. It actually helped me to build this list https://gist.github.com/jibran/7afa1a6d90387e5ff674b6705b71af9b