It seems having BaseFieldOverride causes inconsistencies on what is being exported, at least for computed properties.
This affects for example the path aliases, which get removed as soon as we have a BFO for the path field.
During export, all internal properties are ignored, see:
\Drupal\default_content\Normalizer\ContentEntityNormalizer::getFieldsToNormalize
\Drupal\Core\TypedData\TypedDataInternalPropertiesHelper::getNonInternalProperties
Given 2 content types
- A doesn't have a BFO for the path field
- B does.
\Drupal\Core\Field\BaseFieldDefinition) because will run into: \Drupal\Core\Field\BaseFieldDefinition::isInternal\Drupal\Core\Field\Entity\BaseFieldOverride) because it will run into: \Drupal\Core\Field\FieldConfigBase::isInternalIs this #3005924: Follow-up for #2871591: Marking configurable fields as internal is impossible related ?
| Comment | File | Size | Author |
|---|---|---|---|
| #9 | interdiff_3-9.txt | 5.72 KB | herved |
| #9 | base_field_override_inconsistencies-3203014-9.patch | 9.68 KB | herved |
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #2
herved commentedComment #3
herved commentedHere is what I came up with. It will ignore any internal or read-only properties (but keep computed properties like
path).It is not perfect but I put explanations in comments, check the patch.
The ID, UUID, revision and bundle properties are all read-only, same for media thumbnail. So I removed the related unset's.
Finally, I sorted all the properties which will result in a more consistent and readable output.
Comment #4
herved commentedComment #6
herved commentedOk this was expected, tests need to be updated to reflect the fact that these properties are now removed (revision_translation_affected is read-only).
If you agree on the approach let me know, and I can fix the tests.
Comment #7
berdirInteresting, so sounds like it's a workaround for the core issue?
The comment above bundle is removed, but that was also for langcode and should be changed instead.
The sort is IMHO out of scope and should be a separate issue, I see the consistency argument but it's going to completely change any already exported file, making them impossible to diff.
Comment #8
herved commentedPartially, I identified 2 main issues:
1. Issues with BaseFieldOverrides #3005924: Follow-up for #2871591: Marking configurable fields as internal is impossible
2. We don't want to remove computed properties like
path, but\Drupal\Core\TypedData\DataDefinition::isReadOnly()first checks if it is computed, and if so returns TRUE. Not exactly sure why...Agreed.
Comment #9
herved commentedNew patch without fields sorting and updated tests which should pass.
Comment #10
herved commentedI noticed #3274419: Make BaseFieldOverride inherit internal property from the base field fixed
isInternal()forBaseFieldOverridein core 10.1.6I believe it should address point 1 but not point 2 (from #9).
Is my assumption correct in the first place? that we want to skip internal and read-only fields but not computed
Comment #11
berdirremoving the revision affected field seems sensible, that is calculated on save anyway. Should be updated to a merge request to verify that tests still pass.
Comment #13
herved commentedThe IS is no longer accurate since #3274419: Make BaseFieldOverride inherit internal property from the base field fixed
isInternalforBaseFieldOverrideand path aliases are now exported.But maybe we could use this issue to prevent exporting fields that are explicitly marked as read-only?
I created a MR, anyway.
Doing this prevents field like these to be exported:
- revision_translation_affected
- metatag
- custom computed fields (marked as read-only but not internal)
Comment #14
alexpottThis is a great fix. Lots of hardcoded overrides removed and works a treat. I'm using this MR on a site with exported commerce products & variations and it cleans up a load of stuff.
Comment #16
berdirMerging. Might want to check how core handles this and if something there should be adjusted as well.