ConfigurableEntityReferenceItem should not mess with the field schema

+1 to B

This 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.

If 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.

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
@@ -64,6 +65,12 @@ public static function propertyDefinitions(FieldDefinitionInterface $field_defin
+        $properties['target_revision_id'] = DataDefinition::create('integer')

@@ -108,6 +115,15 @@ public static function schema(FieldDefinitionInterface $field_definition) {
+        $columns['revision_id'] = array(

Mismatch between the property name and the schema column.

I agree with the reasoning in #7 that we should move to 'target_revision_id'.

The problem is that setSettings() overrides *all* settings, including the defaults provided by the field type.

Shouldn't we fix the root cause instead?

Yup, 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()?

In theory we could drop the setSettings() method entirely...

I 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 and FieldConfig 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.

Yup, I guess we are.

Updating the patch with reroll. Please review.

+1 on this, looks like a way cleaner solution :)

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
@@ -39,6 +39,7 @@ public static function defaultSettings() {
+      'target_revision' => FALSE,

@@ -64,6 +65,12 @@ public static function propertyDefinitions(FieldDefinitionInterface $field_defin
+      if ($settings['target_revision'] && $target_type_info->getKey('revision')) {

@@ -108,6 +115,15 @@ public static function schema(FieldDefinitionInterface $field_definition) {
+      if ($field_definition->getSetting('target_revision') && $target_type_info->getKey('revision')) {

I 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.

That'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.

Ugh, 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.

1. +++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Tests/EntityReferenceFieldTest.php
   @@ -47,14 +48,14 @@ class EntityReferenceFieldTest extends EntityUnitTestBase {
   -   * @var array
   +   * @var \Drupal\field\Entity\FieldConfig
   ...
   -   * @var array
   +   * @var \Drupal\field\Entity\FieldInstanceConfig
   

   Shouldn't we put the interfaces here?

2. +++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Tests/EntityReferenceFieldTest.php
   @@ -116,4 +117,49 @@ public function testEntityReferenceFieldValidation() {
   +    // Setup a field and instance.
   

   This comment is not really needed :)

3. +++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Tests/EntityReferenceFieldTest.php
   @@ -116,4 +117,49 @@ public function testEntityReferenceFieldValidation() {
   +    /** @var \Drupal\field\FieldConfigInterface $field */
   +    $field = entity_create('field_config', array(
   

   We can now do FieldConfig::create() instead.

Fully 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 ?

While 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).

@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...

To 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) ...

@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 :)

As already said above, +1 for moving forward with the current patch and discuss the removal of this feature in a different issue.

Fine, opened #2356609: Remove support for "reference a specific revision"

Sure :)

I 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.

diff --git a/core/modules/entity_reference/src/Tests/EntityReferenceFieldTest.php b/core/modules/entity_reference/src/Tests/EntityReferenceFieldTest.php
index 303f490..12370ca 100644
--- a/core/modules/entity_reference/src/Tests/EntityReferenceFieldTest.php
+++ b/core/modules/entity_reference/src/Tests/EntityReferenceFieldTest.php
@@ -7,9 +7,6 @@

 namespace Drupal\entity_reference\Tests;

-use Drupal\Core\Field\FieldDefinitionInterface;
-use Drupal\field\Entity\FieldConfig;
-use Drupal\field\Entity\FieldInstanceConfig;
 use Drupal\field\Entity\FieldStorageConfig;
 use Drupal\system\Tests\Entity\EntityUnitTestBase;
 use Drupal\Core\Field\FieldStorageDefinitionInterface;

If 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.

I 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.

Closing since I've committed #2356609: Remove support for "reference a specific revision"

The schema() override in ConfigurableEntityReferenceItem was removed in #2356609: Remove support for "reference a specific revision"

Not sure there's anything left to fix here ?

Can 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".