Updated: Comment #0
Problem/Motivation
Entity Reference module uses hook_field_info_alter() to change the default class of entity reference items from EntityReferenceItem
to ConfigurableEntityReferenceItem
. That means that as soon as you turn on the module things such as $node->uid
are an instance of the latter and not the former.
Because ConfigurableEntityReferenceItem
changes the schema, however, relative to EntityReferenceItem
(it adds the 'revision_id' column) that means that e.g. $node->uid
has a different schema depending on whether Entity Reference module is installed or not.
Currently the field item schema is unused which is why this hasn't cropped up. With #2183231: Make ContentEntityDatabaseStorage generate static database schemas for content entities, however, that situation would change and things could blow up massively depending on when you turn on Entity Reference module.
Proposed resolution
A: Do not unilaterally alter the field item class. Only use ConfigurableEntityReferenceItem
for configurable fields.
B: Move the 'revision_id' (probably as 'target_revision_id' ?!) schema column and the general ability to reference revisiosn into EntityReferenceItem
. Possibly add a corresponding setting to toggle that behavior.
Remaining tasks
User interface changes
-
API changes
Comment | File | Size | Author |
---|---|---|---|
#44 | 2209981-44-entity-reference-item-revision.patch | 26.56 KB | tstoeckler |
Comments
Comment #1
tstoecklerI would personally vote for B, as I don't see any reason why the non-configurable one should not have a feature that is being used by the configurable one.
Comment #2
andypost+1 to B
Comment #3
amateescu CreditAttribution: amateescu commentedThis is being fixed in #2152571: ER: List of things you can sort by on a default reference type method is incomplete by adding the revision_id schema property only if the field is 'configurable'. See the first hunk of that patch.
Comment #4
tstoecklerHmm... is there any reason we shouldn't move that to the non-configurable class, though? In that case we could maybe tackle that here. Otherwise suppose we could close this issue.
Comment #5
amateescu CreditAttribution: amateescu commentedIf we want to move it to the non-configurable class, we'd need to also move the code that handles revision_id support and it would muddy the waters even more about what the base class supports vs. the configurable class. Maybe in the (near?) future we should try to unify those classes after all, but that's a different battle :)
So yeah, I think this issue can be closed.
Comment #6
tstoecklerHmmm... #2225739: Remove usage of field_info_instances(), use EntityManager::getFieldDefinitions() instead which is the issue that marked the one linked in #3 as duplicate is only mildly related. We should really fix this proper.
Let's see how much this breaks.
This introduces a 'target_revision' (boolean) setting (better naming suggestions welcome!) in EntityReferenceItem to toggle the revision ID behavior of ConfigurableEntityReferenceItem and removes all the related code from there.
Comment #7
tstoecklerOh, what I forgot to mention. I didn't see the 'revision_id' (which should really be target_revision_id) being used in core at all. If you add an entity reference field, the revision ID will not be stored, it will just be added to the schema.
Comment #9
tstoecklerA bunch of entities are using setSettings(), when they really should be using setSetting(). The problem is that setSettings() overrides *all* settings, including the defaults provided by the field type.
Comment #10
amateescu CreditAttribution: amateescu commentedMismatch between the property name and the schema column.
I agree with the reasoning in #7 that we should move to 'target_revision_id'.
Shouldn't we fix the root cause instead?
Comment #11
tstoecklerRight, thanks for the review, will fix that.
Yay :-)
The problem is that there are use-cases of overriding all settings. Over in #2235125: Use DataDefinition::addConstraint() instead of ::setConstraints(), which is very similar to this problem, I simply added a not to setConstraints() that you generally don't want that one. I think we could do the same here. Will do that in a separate issue, though. This was just required to make tests pass.
Comment #12
tstoecklerHere we go. Let's see if I missed anything.
Comment #13
amateescu CreditAttribution: amateescu commentedYup, it's indeed a similar problem. In our case, what was the value of the 'target_revision' setting for field definitions that were using
setSettings()
?Comment #14
tstoecklerRe-reading #11 again, I realize there is a difference to the addConstraint() issue:
In theory we could drop the setSettings() method entirely and just force people to do
<?ph
$settings = $field_definition->getSettings();
// Do something with settings.
foreach ($settings as $name => $settings) {
$field_definition->setSetting($name, $setting);
}
?>
if they want to set *all* settings. This would be a little more code for those use-cases, but those are mostly internal and would make for an API which makes it less easy to shoot yourself in the foot.
Again, separate issue (IMO), but wanted to point that out, since I brought up the other issue above.
Comment #15
tstoecklerRe #13: Well it was not set at all, thus generating notices. The defaults get set initially, but then if you do a setSettings(array()), they're all gone, and then if you try to access the settings, it blows up.
Comment #16
amateescu CreditAttribution: amateescu commentedI think that's one of the reasons why we don't have the setSettings() method on
FieldDefinitionInterface
.What troubles me here is that this can only happen for base fields, because configurable fields have some code in
FieldInstanceConfig
andFieldConfig
specifically for preventing this scenario and returning the default value.Maybe the default behavior of setSettings() needs to be changed.. not sure what to do :/
Edit: What I am sure, though, is that this problem has already been discussed elsewhere, we just need to find it.
Comment #17
tstoecklerOk, I opened #2236903: setSettings() on field definitions can unintentionally clear default values to discuss that.
Are we ready to go here?
Comment #18
tstoecklerComment #19
amateescu CreditAttribution: amateescu commentedYup, I guess we are.
Comment #21
tstoeckler#2225739: Remove usage of field_info_instances(), use EntityManager::getFieldDefinitions() instead broke this. This still think it makes sense, though, IMO even though it is no longer a blocker for #2183231: Make ContentEntityDatabaseStorage generate static database schemas for content entities (I think).
Here's a re-roll.
The merge conflict was only due to the code that is being removed having changed due to abovementioned issue, so setting straight to RTBC. Hope that's OK.
Comment #23
Jalandhar CreditAttribution: Jalandhar commentedUpdating the patch with reroll. Please review.
Comment #24
tstoecklerAwesome, thanks a lot. Back to RTBC per #19.
Comment #25
plach+1 on this, looks like a way cleaner solution :)
Comment #26
alexpottI can not determine if we have code or configuration that is ver going to set
target_revision
to TRUE. Is this untested? Setting to needs review to get this question answered.Comment #27
amateescu CreditAttribution: amateescu commentedThat's true, we don't have any UI configuration for that. Currently, it can only be set in default config or when you create the field programatically.
Comment #28
tstoecklerYes, it's currently untested. #2183231: Make ContentEntityDatabaseStorage generate static database schemas for content entities will (implicitly) provide integration-level test coverage for this. Attached patch includes a unit test. The verbosity/ugliness of that test is why I didn't include a test right from the start. Field items are currently not very unit testable. Also note that this is the first test for a class in \Drupal\Core\Field.
In current 8.x the schema column is always created (if Entity Reference module is installed) but as #27 mentions it is not possible to actually put values in it, except programmatically in some way. With the patch you can control whether that column will be created or not. That you can't put values in it isn't changed by the patch but I hope we can explore that in a follow-up instead of fixing that here.
Comment #30
tstoecklerD'uh, forgot to add getInfo() to the unit test... Drupal-- (but also tstoeckler's memory--).
Also removed the needless storing of the module handler in the test as it's only being used in one test method.
Comment #31
amateescu CreditAttribution: amateescu commentedUgh, I agree with @tstoeckler, the unit test added in #28 is quite an overkill. If we really want to test this, I think it would be a lot easier to do it in
\Drupal\entity_reference\Tests\EntityReferenceFieldTest
.Comment #32
tstoecklerOK, here we go. This comes with a little addition to
EntityReferenceFieldTest
.Comment #34
tstoecklerComment #37
amateescu CreditAttribution: amateescu commentedShouldn't we put the interfaces here?
This comment is not really needed :)
We can now do FieldConfig::create() instead.
Comment #38
tstoecklerBringing this back from the dead. Sorry, but couldn't be bothered to generate an interdiff for this due to merge conflicts.
Comment #40
tstoecklerLet's see if this is green. Should at least give some sensible test results.
Comment #42
tstoecklerLet's see if this is green. There's no way you're going to get me to run ConfigImportAllTest locally, so this is going to be a trial-and-error. :-)
Comment #44
tstoecklerConfig schema error messages are very strange.
Comment #45
yched CreditAttribution: yched commentedFully agreed that ConfigurableEntityReferenceItem needs to stop switching a different feild schema.
However, this "reference specific revision" feature feels experimental still. AFAIK :
- Core has no use for it
- No widget supports referencing a specific revision anyway
- This really doesn't feel like a "80%" (nor 95%, for that matter) use case
- The behavior (field display - if the user doesn't have access to previous/unpulblished revisions, or views relationships), feel a bit unclear / TBD ?
Looks like a feature that would belong to a separate field type in contrib, and has no reason for being in core ?
Comment #46
amateescu CreditAttribution: amateescu commentedWhile I agree that the revision support looks a bit experimental (it was added out of the blue in #1801304-111: Add Entity reference field, after @bojanz said in comment #73 that we should open a followup for it), now that it's already in it feels a bit weird to remove it because I know for sure that Drupal Commerce 2.x will use it *a lot*.
And experimenting with new widgets/formatters in contrib is a whole lot easier than experimenting with an entirely different field type (for example, Commerce would have to depend on that field type instead of the one we have now in core).
Comment #47
yched CreditAttribution: yched commented@amateescu: thanks for the background - but honestly, i read #46 as "this feature never received actual thought and should have never gotten into Core as is" :-/.
We are now way past the time where we should be polishing experimental features that are still half-baked at this point - and I don't even know how we could get that feature "right" in Core, given that we have no use case.
If Drupal Commerce has one, fine, IMO they'll be better off relying on a solid, dedicated field type that suits their needs in contrib.
This "reference specific revision" feature has "contrib" written all over it in red caps, as far as I'm concerned...
Comment #48
yched CreditAttribution: yched commentedTo be more specific:
I agree with @tstoeckler that we cannot keep the status quo - entity_reference.module cannot come and swap different field schemas for base entity_ref fields.
So either 1) we promote it up into the native EntityRefItem used for all ref fields, or 2) we remove it and let contrib handle it.
Given that:
- this is highly edge-case in terms of "data modelling toolbox"
- this is still atm a half-baked feature that no-one really thought about
- it was never actually battle tested since it wasn't part of entityreference D7,
- I see no technical reason for which it *has* to be done in core native's reference field type,
I'd be fairly strongly in favor of 2) ...
Comment #49
amateescu CreditAttribution: amateescu commented@yched, you read the *first part* of #46 correctly, that's exactly how I felt when it was added.
Now, however, I don't think it really hurts anyone by being there and letting contrib experiment around it. I'd prefer option 1) (what the current patch does) but you seem to hate it though and I guess I don't feel as strongly about it as you do :)
Comment #50
tstoecklerOne the one hand I agree with @amateescu that I think it doesn't really hurt to leave this in now that we have it, since I think the maintenance value should not be large (once this issue gets in). The use-cases in contrib will be ample. I recently talked to @larowlan, for example, about Workbench Moderation in D8 and that will certainly need that capability. In general, since revisionability is so (!!!) much easier to implement in D7 a lot of contrib entity types will be revisionable and I think this will be a common need for many sites.
On the other hand it always gives a little bit of unease when we modify the schema based on settings. And it wouldn't really be a problem for contrib to implement this in a lightweight base module which other modules could depend on.
I talked to @Xano about this problem space in Amsterdam and we should really have a clear separation of mutable and immutable settings (or better named versions thereof), where the
target_revision
setting would fall in the latter category. Currently one can change any schema-affecting setting at any point in time which will lead to inconsistent results in code that interacts with the field schema in the worst case leading to SQL errors. But the fact that we lack this distinction in our API is not the fault ofEntityReferenceItem
and we have other instances of settings affecting the schema in core (albeit not actually adding or removing columns, as far as I'm aware).Lastly, though, I would really like to move forward with this as the current status is really, really strange and can lead to incredibly hard-to-debug issues.
So I would propose to move forward with this patch as is, as it can basically go in as is, as far as I'm concerned, and discuss whether or not to remove the feature in a follow-up.
Thoughts?
Comment #51
tstoecklerOne the one hand I agree with @amateescu that I think it doesn't really hurt to leave this in now that we have it, since I think the maintenance value should not be large (once this issue gets in). The use-cases in contrib will be ample. I recently talked to @larowlan, for example, about Workbench Moderation in D8 and that will certainly need that capability. In general, since revisionability is so (!!!) much easier to implement in D7 a lot of contrib entity types will be revisionable and I think this will be a common need for many sites.
On the other hand it always gives a little bit of unease when we modify the schema based on settings. And it wouldn't really be a problem for contrib to implement this in a lightweight base module which other modules could depend on.
I talked to @Xano about this problem space in Amsterdam and we should really have a clear separation of mutable and immutable settings (or better named versions thereof), where the
target_revision
setting would fall in the latter category. Currently one can change any schema-affecting setting at any point in time which will lead to inconsistent results in code that interacts with the field schema in the worst case leading to SQL errors. But the fact that we lack this distinction in our API is not the fault ofEntityReferenceItem
and we have other instances of settings affecting the schema in core (albeit not actually adding or removing columns, as far as I'm aware).Lastly, though, I would really like to move forward with this as the current status is really, really strange and can lead to incredibly hard-to-debug issues.
So I would propose to move forward with this patch as is, as it can basically go in as is, as far as I'm concerned, and discuss whether or not to remove the feature in a follow-up.
Thoughts?
Comment #52
amateescu CreditAttribution: amateescu commentedAs already said above, +1 for moving forward with the current patch and discuss the removal of this feature in a different issue.
Comment #53
yched CreditAttribution: yched commentedFine, opened #2356609: Remove support for "reference a specific revision"
Comment #54
tstoecklerDoes someone want to RTBC this one, then? ;-)
Comment #55
amateescu CreditAttribution: amateescu commentedSure :)
Comment #56
alexpottI don't think I see the point of making this change if we going to rip it out. @DamZ has already +1 the decision in #2356609: Remove support for "reference a specific revision"
Bascially, it feels silly to commit this and then rip it out.
Comment #57
alexpottIf someone other than me commits this the three use statements above need removing on commit. The first two because they are not used and
Drupal\field\Entity\FieldInstanceConfig
because it does not exist.Comment #58
amateescu CreditAttribution: amateescu commentedI agree that it feels silly, I RTBC'd it because @tstoeckler put a lot of work into the patch and I didn't want to dismiss it.
Comment #59
tstoecklerMeh :-/
Let's leave this postponed for now. If the other issue is fixed, this can be closed directly but if that issue does get bikeshedded or closed or whatever we still need to fix this.
Comment #60
tstoecklerActually we should still fix some of the bugs and general cleanup found here to get the tests to pass, so postponed is in fact correct. Also we should not lose the actual test coverage that was introduced here.
Comment #61
alexpottClosing since I've committed #2356609: Remove support for "reference a specific revision"
Comment #62
tstoecklerPer #60.
Needs a re - titling now.
Comment #63
plachComment #64
yched CreditAttribution: yched commentedThe schema() override in ConfigurableEntityReferenceItem was removed in #2356609: Remove support for "reference a specific revision"
Not sure there's anything left to fix here ?
Comment #65
jeroen.b CreditAttribution: jeroen.b commentedCan I vote for re-adding the target_revision_id to the field schema?
See some discussion about it here: https://www.drupal.org/node/2330089#comment-9537355
I know core does not really have a use for it now, but a lot of contrib modules in the future will.
I'd really like to prevent having to make a separate contrib module that only implements a field type to add "target_revision_id".