Deprecate entity_reference.module and move its functionality to core

Entity Reference module
- Amitai Burstein 'Amitaibu' http://drupal.org/user/57511
- Andrei Mateescu 'amateescu' http://drupal.org/user/729614

We can remove this too from MAINTAINERS.txt. :P

OMG so much yay.

As a side note, though - ER is a topic in itself, it would be nice to keep an "entity reference field" or something in MAINTAINERS.txt, rather than simply merging it in the "field system" component.

Opened #2438267: Create an "EntityReference system" component

But...but... the old title was awesome ;)

1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
   @@ -91,8 +98,11 @@ public static function propertyDefinitions(FieldStorageDefinitionInterface $fiel
   -    if (isset($settings['target_bundle'])) {
   -      $properties['entity']->getTargetDefinition()->addConstraint('Bundle', $settings['target_bundle']);
   +    // @todo This is a problem, the target bundles is now a field setting
   +    // instead of a field storage setting. Clarify/fix this in
   +    // https://www.drupal.org/node/2064191
   +    if (isset($settings['handler_settings']['target_bundles'])) {
   +      $properties['entity']->getTargetDefinition()->addConstraint('Bundle', $settings['handler_settings']['target_bundles']);
        }
   

   See also the recent discussion in the remove taxonomy term reference field issue on field vs. storage setting.

2. +++ b/core/modules/system/system.module
   @@ -1382,3 +1386,93 @@ function system_path_insert() {
   +
   +/**
   + * Implements hook_ENTITY_TYPE_update() for 'field_storage_config'.
   + *
   + * Reset the instance handler settings, when the target type is changed.
   + */
   +function system_field_storage_config_update(FieldStorageConfigInterface $field_storage) {
   +  if ($field_storage->getType() != 'entity_reference') {
   +    // Only act on entity reference fields.
   

   I'd argue this belongs in field.module, not system?

   The config entities are provided by field.module, not core.

3. +++ b/core/modules/system/system.module
   @@ -1382,3 +1386,93 @@ function system_path_insert() {
   +function entity_reference_create_field($entity_type, $bundle, $field_name, $field_label, $target_entity_type, $selection_handler = 'default', $selection_handler_settings = array(), $cardinality = 1) {
   

   This is exclusively used in tests. I don't think there's going to be enough code to dynamically create ER fields to keep this as a public API. Test trait?

4. +++ b/core/modules/system/system.views.inc
   @@ -10,10 +10,19 @@
   -function entity_reference_field_views_data(FieldStorageConfigInterface $field_storage) {
   +function core_field_views_data(FieldStorageConfigInterface $field_storage) {
   

   That's.. weird ;) But same, why not move this to views.module?

for 10.3 trait is better like #2357199: Consider CommentManagerInterface::addDefaultField() as deprecated and remove in favour of CommentTestTrait

+++ b/core/modules/system/system.module
@@ -1382,3 +1386,93 @@ function system_path_insert() {
+/**
+ * Implements hook_ENTITY_TYPE_update() for 'field_storage_config'.
+ *
+ * Reset the instance handler settings, when the target type is changed.
+ */
+function system_field_storage_config_update(FieldStorageConfigInterface $field_storage) {
+  if ($field_storage->getType() != 'entity_reference') {
+    // Only act on entity reference fields.
+    return;
+  }
+
+  if ($field_storage->isSyncing()) {
+    // Don't change anything during a configuration sync.
+    return;
+  }
+
+  if ($field_storage->getSetting('target_type') == $field_storage->original->getSetting('target_type')) {
+    // Target type didn't change.
+    return;
+  }
+
+  foreach ($field_storage->getBundles() as $bundle) {
+    $field = FieldConfig::loadByName($field_storage->getTargetEntityTypeId(), $bundle, $field_storage->getName());
+    $field->settings['handler_settings'] = array();
+    $field->save();
+  }
+}

Is this code used? You can't change the the target_type through the UI. Shouldn't we just prevent changing it?

The blocking issue has landed.

10.4 yeah, I guess so. surprised that we have no other example yet with all the stuff we moved into core, but I guess all those field types didn't have special views integration. The only other option I see is to put it right into views_field_default_views_data(), which is always called. That might actually not be such a stupid idea?

I guess the difference of that is that it would automatically also run for taxonomy, file/image and other references, that are not explicitly type entity_reference.

There's already an entity_reference_revisions module in contrib, which would then also benefit from it, although it would probably have to customize the relationship a bit.

Oddities like core_field_views_data() are artifacts of the current shape of the "views integration for field types" API, which is still very much the CCK D6 version :-/. In an ideal world, it would move to an OO / handler shape, like was done for "views integration for entity types".
Separate issue, though ;-)

Let's see a simple reroll.

Sorry, forgot the new trait.

Narrowing the test failures.

Not ready. Still needs tests fixing.

Assigning this to @alexpott, CC @catch for a direction with this issue.

Few days ago, I discussed on IRC with @Berdir on this issue and he recommended me to ask you (@alexpott & @catch) if this can go in 8.0.0. There's some work to do here, not a simple issue, and I want to be sure that we don't waste effort on a task that has no chance to catch 8.0.0.

Overall, I feel that entity reference needs some love before the first 8 stable and I'm willing to help here. There are other unfinished tasks/bugs related to ER open.

nice try.

This can go in the framework manager review queue.

This has to be green.

Entity reference field (and descendants, like image, file) they all need an update path to remove the target_bundle setting from field storage config and possibly append it to field config handler settings, into target_bundles array. That's why I've implemented a new update hook, field_update_8001(), and added test to test this update (\Drupal\system\Tests\Field\Update\FieldSettingsTest).

Note also that Entity Reference module is still kept in code base but empty, with no functionality and we have to remove it in a separate issue. I will open a new issue for that.

Opened #2552139: Remove entity_reference module from codebase to remove entity_reference module from codebase in 8.1.x.

Can someone indicate in the beta evaluation which prioritized change type this is?:

https://www.drupal.org/core/beta-changes#prioritized

Wondering about field_update_8001() :

1. given that this patch is moving stuff from e_r.module to Core, doesn't it belong to system updates rather than field.module, who is not really involved here ?
2. Do we really need to bother carrying the former storage 'target_bundle' over to the field handler_settings ? IIRC nothing was actually using that setting anyway for configurable fields, so whatever was in there was not really important ? (if the field was working as expected, the field handler_settings already has the correct target_bundles to make the field work ?)

3. +++ b/core/modules/field/field.install
   @@ -0,0 +1,69 @@
   +  // Iterate on all fields storage to find fields having 'target_setting'.
   

   should be 'target_bundle' ?

4. +++ b/core/modules/field/field.install
   @@ -0,0 +1,69 @@
   +          $handler_settings = $handler_settings ?: [];
   

   That line shouldn't be needed, the !empty() in the next line should account for the case $handler_settings is empty / NULL (empty($null['foo']) is TRUE)

5. +++ b/core/modules/field/field.install
   @@ -0,0 +1,69 @@
   +          else {
   +            continue;
   +          }
   

   If the value is neither an array nor a string, then we'll be looping an all fields just to do "continue" on each one. This check (if needed at all ? can it really happen that it's not empty and neither an array nor a string ?) could move to the if() wrapping the foreach() ?

6. +++ b/core/modules/field/field.install
   @@ -0,0 +1,69 @@
   +  // Disable 'entity_reference' module.
   +  $module_installer->uninstall(['entity_reference']);
   

   Not sure how we handle modules disappearing from the codebase, but AFAICT by looking at ModuleInstaller::uninstall(), this will do nothing if the module cannot be found in the codebase (first if() in the method)

   Shouldn't we just directly remove the entry from the core.extension config ?

@yched,

Not sure how we handle modules disappearing from the codebase, but AFAICT by looking at ModuleInstaller::uninstall(), this will do nothing if the module cannot be found in the codebase (first if() in the method)

I don't think we can remove the module from codebase right now because there are update tests that have the module in the list and enabled (from D8 dumps) and those tests will crash because are not able to find the code in the code base. The patch is not removing the module, is only leaving an empty module with no functionality and disable it. Anyway, I'm not very sure that removing from core.extension will not work.

Sure, but that's only because the #2552139: Remove entity_reference module from codebase part is temporarliy deferred to a later step ? Once that is done, sites going from beta14 to beta15 (or whatever depending on when this gets in) will run the update while e_r.module is not in the codebase anymore, and the code in the update as it currently stands will do nothing ?

The problem is this:

Let's assume this scenario:

1. We commit this now including this upgrade path.
2. You run the updates to the next beta, the module is uninstalled and everything works.
3. Later on, lets say in 8.1, we remove the module left-overs from the system.

Now, someone, for some reason, updates a site directly from beta13 to 8.1. Then he will run into the same problem as we have right now.

That's the main reason for why update functions are so complex. You never know from which version someone is updated to what, they need to work for all possible cases. And the best way to do that is to work with as low-level API's as possible and avoid any hooks/events that you can't control.

That said, we actually discussed on the d8 criticals hangout last friday about exactly such a scenario: Whether we want to special case beta updates and not official support a direct update from betaX to 8.0+. This would be a good reason for that. At the same time, we're going to survive carrying around a hidden, empty entity_reference.info.yml file.

#43.1: Moved as system_update_8004().

#43.2-5: Removed the part that copies target_bundle over handler settings target_bundles.

#43.6, #45, #46: Using config API to uninstall.

Also in system_update_8004() I'm processing now module dependencies and type provider values.

These tests are passing locally. I don't understand why are failing here.

Reroll.

Rerolled.

Fixed the new test location.

The reroll is not straight. #66 doesn't take into account changes from 7a25f513f5b74d7fbc29b95b7de2fc88334533f4.

Tagging.

Thanks @claudiu.cristea I should have noticed that from the size of the patch.

Fix tag.

Reopened #2064191: Fix the 'target_bundle' field setting - 'target_bundles' are not validated when entity_reference is enabled and posted a patch over there, to try to get that part cleaned up.

As I noted there, this part of the patch here is incorrect :

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
@@ -102,14 +108,11 @@ public static function propertyDefinitions(FieldStorageDefinitionInterface $fiel
-    if (isset($settings['target_bundle'])) {
-      $properties['entity']->addConstraint('Bundle', $settings['target_bundle']);
-      // Set any further bundle constraints on the target definition as well,
-      // such that it can derive more special data types if possible. For
-      // example, "entity:node:page" instead of "entity:node".
-      $properties['entity']->getTargetDefinition()
-        ->addConstraint('Bundle', $settings['target_bundle']);
+    if (!empty($settings['handler_settings']['target_bundles'])) {
+      $properties['entity']
+        ->addConstraint('Bundle', $settings['handler_settings']['target_bundles']);
     }

'handler_settings' is a Field-level setting, we can't access it in propertyDefinitions(), where all we have is a FieldStorageDefinition.

That would be *very* nice to get this in before RC :-)
Now that #2064191: Fix the 'target_bundle' field setting - 'target_bundles' are not validated when entity_reference is enabled is in, that should be mostly about moving code around with no logic change, right ?

@yched, I'll try to rework this tomorrow. It would be nice if we can have a positive sign on this from the core committers.

You still need a (actual) beta evaluation in order to get said sign-off.

here we go. Thank you @yched for hints.

Rerolled.

Fixed IS. Added Beta Evaluation.

Thanks for the awesome work, @claudiu.cristea !

1st pass at reviewing, I only looked at the "peripherals" for now (not the actual e_r code being moved around)

1. +++ b/core/lib/Drupal/Core/Entity/Plugin/EntityReferenceSelection/SelectionBase.php
   @@ -132,7 +133,7 @@ public function buildConfigurationForm(array $form, FormStateInterface $form_sta
   -        '#element_validate' => array('_entity_reference_element_validate_filter'),
   +        '#element_validate' => array(array(get_class($this), 'elementValidateFilter')),
   
   @@ -179,7 +180,7 @@ public function buildConfigurationForm(array $form, FormStateInterface $form_sta
   -        '#process' => array('_entity_reference_form_process_merge_parent'),
   +        '#process' => array(array('\Drupal\Core\Field\Plugin\Field\FieldType\EntityReferenceItem', 'formProcessMergeParent')),
   

   Nit : move to [ ... ] short syntax ?

2. +++ b/core/lib/Drupal/Core/Entity/Plugin/EntityReferenceSelection/SelectionBase.php
   @@ -215,6 +216,16 @@ public function validateConfigurationForm(array &$form, FormStateInterface $form
   +   * @todo Move some form of this to Form API itself?
   

   We shouldn't be adding orphaned @todos, we should either remove it or open an issue and link to it.

3. +++ b/core/lib/Drupal/Core/Entity/Plugin/EntityReferenceSelection/SelectionBase.php
   @@ -338,8 +349,8 @@ protected function buildEntityQuery($match = NULL, $match_operator = 'CONTAINS')
   -    // entity_reference_query_entity_reference_alter().
   -    $query->addTag('entity_reference');
   +    // system_query_entity_reference_selection_alter().
   +    $query->addTag('entity_reference_selection');
   

   Why is this change needed ? That seems like an API break ?

4. +++ b/core/modules/migrate/src/Entity/MigrationInterface.php
   @@ -252,6 +252,8 @@ public function setProcess(array $process);
   +   *
   +   * @see Drupal\migrate_drupal\Plugin\migrate\load\LoadEntity::processLinkField().
   

   That looks unrelated ?

5. +++ b/core/modules/system/src/Tests/Field/Update/FieldSettingsTest.php
   @@ -0,0 +1,101 @@
   +class FieldSettingsTest extends UpdatePathTestBase {
   

   Class name is a bit vague :-)

6. +++ b/core/modules/system/src/Tests/Field/Update/FieldSettingsTest.php
   @@ -0,0 +1,101 @@
   +    // Create an entity reference field.
   +    $this->createEntityReferenceField('node', 'article', 'foo', $this->randomString(), 'node');
   +
   +    // Use the Config API in order to bypass validations and write directly to
   +    // the backend.
   +    $field_storage = $this->configFactory->getEditable('field.storage.node.foo');
   +    // Add a deprecated 'target_bundle' storage settings.
   +    $field_storage->set('settings.target_bundle', NULL);
   +    // Add a dependency to the 'entity_reference' module to check if it will get
   +    // removed.
   +    $dependencies = $field_storage->get('dependencies');
   +    $dependencies['module'][] = 'entity_reference';
   +    $field_storage->set('dependencies', $dependencies);
   +    // Set the provider to 'entity_reference' to see if it will get replaced
   +    // by 'core'.
   +    $field_storage->set('module', 'entity_reference');
   +    $field_storage->save(TRUE);
   

   As mentioned by @dawehner in #2064191-55: Fix the 'target_bundle' field setting - 'target_bundles' are not validated when entity_reference is enabled, the test shouldn't use the APIs to adjust the content of the config before running the updgrades.

   We should either
   - put our test fixtures in an additional dump file (like for example LocalActionsAndTasksConvertedIntoBlocksUpdateTest does),
   - or simply base our test on the content of drupal-8.bare.standard.php.gz (frozen dump based on beta12)

   The latter is way simpler and largely good enough IMO, that dump contains configurable e_r fields we can test (node.article.field_tags for example)

   See for example EntityReferenceTargetBundleUpdateTest

7. +++ b/core/modules/system/src/Tests/Field/Update/FieldSettingsTest.php
   @@ -0,0 +1,101 @@
   +    // The legacy 'target_bundle' storage setting exists and is NULL.
   +    $this->assertTrue(array_key_exists('target_bundle', $settings));
   +    $this->assertNull($settings['target_bundle']);
   ...
   +    // Setting 'target_bundle' has been removed.
   +    $this->assertFalse(array_key_exists('target_bundle', $settings));
   

   After #2064191: Fix the 'target_bundle' field setting - 'target_bundles' are not validated when entity_reference is enabled, those parts are handled in a different update, and tested by a different test.

8. +++ b/core/modules/system/system.install
   @@ -1808,5 +1808,44 @@ function system_update_8011() {
   +function system_update_8012() {
   

   Arguably this would belong to field.module :
   - e_r module was about configurable fields and required field.module
   - the update of the field config only makes sense if field.module is enabled

9. +++ b/core/modules/system/system.module
   similarity index 83%
   rename from core/modules/entity_reference/entity_reference.views.inc
   
   rename from core/modules/entity_reference/entity_reference.views.inc
   rename to core/modules/system/system.views.inc
   

   Ideally that should be shipped in views.module instead of system.

10. +++ b/core/modules/taxonomy/taxonomy.module
    @@ -501,9 +501,12 @@ function taxonomy_build_node_index($node) {
    +      $is_entity_reference_class = $class == $entity_reference_class || is_subclass_of($class, $entity_reference_class);
    

    Nit : would need parenthesis around the == check.
    (also, that would be best as === IMO)

11. +++ b/core/modules/entity_reference/entity_reference.module
    @@ -2,212 +2,7 @@
    -      $output .= '<dt>' . t('Configuring form displays') . '</dt>';
    -      $output .= '<dd>' . t('Reference fields have several widgets available on the <em>Manage form display</em> page:');
    -      $output .= '<ul>';
    -      $output .= '<li>' . t('The <em>Check boxes/radio buttons</em> widget displays the existing entities for the entity type as check boxes or radio buttons based on the <em>Allowed number of values</em> set for the field.') . '</li>';
    -      $output .= '<li>' . t('The <em>Select list</em> widget displays the existing entities in a drop-down list or scrolling list box based on the <em>Allowed number of values</em> setting for the field.') . '</li>';
    -      $output .= '<li>' . t('The <em>Autocomplete</em> widget displays text fields in which users can type entity labels based on the <em>Allowed number of values</em>. The widget can be configured to display all entities that contain the typed characters or restricted to those starting with those characters.') . '</li>';
    -      $output .= '<li>' . t('The <em>Autocomplete (Tags style)</em> widget displays a multi-text field in which users can type in a comma-separated list of entity labels.') . '</li>';
    

    Those are not reflected in the help text added to field_help() ?

12. +++ b/core/modules/field/field.module
    @@ -205,6 +218,67 @@ function field_entity_bundle_delete($entity_type, $bundle) {
    +function field_form_field_ui_field_storage_add_form_alter(array &$form) {
    

    Nit : would rather belong to field_ui.module ?

Almost done, but need to run, here's what I have for a 2nd part :

1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
   @@ -164,6 +171,13 @@ public function getConstraints() {
   +    // Remove the 'AllowedValuesConstraint' validation constraint because entity
   +    // reference fields already use the 'ValidReference' constraint.
   +    foreach ($constraints as $key => $constraint) {
   +      if ($constraint instanceof AllowedValuesConstraint) {
   +        unset($constraints[$key]);
   +      }
   +    }
   

   Minor : this would logically go above the code block where we add another constraint (no need to loop over that one)

2. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
   @@ -283,6 +297,107 @@ public static function generateSampleValue(FieldDefinitionInterface $field_defin
   +  public static function fieldSettingsFormValidate(array $form, FormStateInterface $form_state) {
   +    if ($form_state->hasValue('field')) {
   +      $form_state->unsetValue(array('field', 'settings', 'handler_submit'));
   +      $form_state->get('field')->settings = $form_state->getValue(['field', 'settings']);
   +
   +      $handler = \Drupal::service('plugin.manager.entity_reference_selection')->getSelectionHandler($form_state->get('field'));
   +      $handler->validateConfigurationForm($form, $form_state);
   +    }
   +  }
   

   This method is faitly different from what it used to be in ConfigurableEntityReferenceItem, wondering why ?

3. +++ b/core/modules/field/src/Tests/EntityReference/EntityReferenceTestTrait.php
   @@ -39,7 +39,7 @@
   -  protected function createEntityReferenceField($entity_type, $bundle, $field_name, $field_label, $target_entity_type, $selection_handler = 'default', $selection_handler_settings = array(), $cardinality = 1) {
   +  public function createEntityReferenceField($entity_type, $bundle, $field_name, $field_label, $target_entity_type, $selection_handler = 'default', $selection_handler_settings = array(), $cardinality = 1) {
   

   Why the visibility change ?

4. +++ b/core/modules/system/system.views.inc
   @@ -10,9 +10,19 @@
   +  // The code below only deals with the Entity reference field.
   

   Nitpick : s/field/field type

• #83.1, 2: OK.
• #83.3: I don't know what is the reason for that change. We should ask @amateescu, he worked the patch in the early stages. I switched back for now.
• #83.4: Removed.
• #83.5: I moved the test together with the one for filed_update_8001. I renamed the class to a generic FieldUpdateTest now we can add there future tests for field -- field_update_N()
• #83.6: Ok, I used field_tags for testing. It works.
• #83.7: Yep! I forgot that.
• #83.8: Great to escape from system.install hell :)
• #83.9: Moved in views.views.inc.
• #83.10: Ok.
• #83.11: Added.
• #83.12: Moved to field ui.
• #84.1: Ok.
• #83.2: I did some tests around this. It seems that is an attempt clear non-JS submit button that is stored in settings. But as far as I can see that if () {} is never triggered and the settings are saved correctly. Reverted to ConfigurableEntityReferenceItem version because that is in place right now and works as expected. Maybe @amateescu knows more on this.
• #84.3: That is a mistake. Reverted.
• #84.4: Ok.

@claudiu.cristea : Thanks !

Meanwhile, I finished reviewing #79, no remarks left :-)

Remarks below are about the interdiff in #85

1. +++ b/core/modules/views/views.views.inc
   @@ -652,3 +652,76 @@ function views_field_default_views_data(FieldStorageConfigInterface $field_stora
   +function views_field_views_data(FieldStorageConfigInterface $field_storage) {
   

   It still needs to be called core_field_default_views_data(), since it's a "pseudo-hook / magic callback" (probably one of the last in core), called on the provider of the field type - in this case, 'core'.

   Still makes sense IMO that views.module's views.views.inc provides integration code for the field types provided by core.

   That probably explains the fails.

2. +++ b/core/modules/field/src/Tests/Update/FieldUpdateTest.php
   @@ -0,0 +1,103 @@
   +    // The type provider for the field_tags field is entity_reference module.
   ...
   +    // The entity_reference module is a dependency of field_tags config entity.
   ...
   +    // Check the 'node.article.field_tags' field config.
   ...
   +    // The type provider is now 'core'.
   ...
   +    // The module dependency on 'entity_reference' has been removed.
   ...
   +    // The entity_reference module has been uninstalled.
   

   Clarity nitpick : can we start those comments with "Check that [foo is bar]" ? Helps a lot when visually parsing what the test does :-)

Then I guess the last question is : at this point, what prevents us from removing entity_ref.module completely ? :-)

Also, @amateescu :
The review points in #83.3 and #84.2 seem to be about your earlier versions of the patch. @claudiu.cristea reverted them in #85, but maybe we missed something ?

Expanded the motivations in the IS a bit.

Great, I love the idea that we can get rid of entityreference module!

+++ /dev/null
index 332de7b..0000000
--- a/core/modules/entity_reference/src/ConfigurableEntityReferenceItem.php

--- a/core/modules/entity_reference/src/ConfigurableEntityReferenceItem.php
+++ /dev/null

+++ /dev/null
+++ /dev/null
@@ -1,224 +0,0 @@

@@ -1,224 +0,0 @@
-<?php

Since we are in API freeze we cannot just remove classes like this, since that would break contrib modules. This class and all others from entityreference module need to stay as empty classes that just extend the one from Core now. Except for test classes.

And even in 8.1.x we cannot remove those classes, so the entire removal of the module must be postponed to Drupal 9. Otherwise we make contrib developers very angry :-)

@klausi that is why it has Needs framework manager review tag :). I think @Berdir has a script which renames the module so that can be helpful here but none the less good points.

As DER maintainer this is disruptive but I only have to change some use statements and it'll be good as new :D.

I'm strongly +1 for this issue.

@klausi : thing is, we cannot keep the class for BC without also keeping an entity_reference.module that can be enabled (for the class to be found by the autoloader)

Since we wouldn't want people to see it in the module admin page and wonder what it does (it does nothing), that would mean making it hidden *and* required.

• #87.1: Right. I was confused by the docblock: "Implements hook_..." and treated as a hook, but it's a magic call. Expanding a little the docblock to explain this.
• #87.2: Ok.

@yched,

Then I guess the last question is : at this point, what prevents us from removing entity_ref.module completely ? :-)

I tried this once. All the update tests will fail because you import a DB snapshot of beta12 and inside that a Drupal instance where the entity_reference module is enabled. But in the same time the module is missing from the codebase. That shows a very angry drupal_get_filename() :)

trigger_error(SafeMarkup::format('The following @type is missing from the file system: @name', array('@type' => $type, '@name' => $name)), E_USER_WARNING);

The old bot :(

Yes, removing the module is painful for existing sites.

I don't think we need to make the module required, just keep it around as hidden so that existing sites don't break.

You can see the failures here https://www.drupal.org/node/2274167?page=1#comment-10405883

I don't think we need to make the module required, just keep it around as hidden so that existing sites don't break.

That was if we wanted to keep the Item class for BC (if we keep the module anyway, we might as well do that ?). If a contrib module uses the class (and thus requires the module), but the module is hidden, you're in a deadlock ?

Thus I was thinking hidden + required (always enabled, problem disappears) ?

If it's hidden and a dependency I'd expect core to enable it? If that doesn't work would be a bug.

Only thing you can't easily do when it's hidden is uninstall.

Indeed, my bad, you can enable a module that depends on a hidden module, both through admin/modules and drush en.

So I guess that means we should just :
- leave reference/src/ConfigurableEntityReferenceItem.php as an empty @deprecated class extending EntityReferenceItem,
- remove the part of the update (and test) that uninstalls entity_reference.module on existing sites
- Adjust the 'description' in entity_reference.info.yml to something like "Deprecated, all functionality has been moved to Core' (because the current description, 'Provides a field that can reference other entities', would be a lie)
- (Not sure we really need the .module file ?)

And then we can RTBC, commit, and break no BC at all ? :-)

@yched, sounds like plan. But still keep it as hidden: true, right? Anyway, if a site admin will disable the module we assume that he knows what he is doing, meaning he has no class extending ConfigurableEntityReferenceItem. Fair enough.

OK. That's it.

Thanks @claudiu.cristea. Let's do this !

1. +++ b/core/MAINTAINERS.txt
   @@ -323,10 +323,6 @@ Email module
   -- Amitai Burstein 'Amitaibu' https://www.drupal.org/u/amitaibu
   -- Andrei Mateescu 'amateescu' https://www.drupal.org/u/amateescu
   

   I don't think we should remove amitaibu and amateescu, they should be moved either to the Entity API section or a new section "Entity Reference API".

2. +++ b/core/lib/Drupal/Core/Entity/Plugin/EntityReferenceSelection/SelectionBase.php
   @@ -179,7 +180,7 @@ public function buildConfigurationForm(array $form, FormStateInterface $form_sta
   +        '#process' => [['\Drupal\Core\Field\Plugin\Field\FieldType\EntityReferenceItem', 'formProcessMergeParent']],
   

   since we depend on PHP 5.5 now we should use ::class here instead of the class name in a string.

3. +++ /dev/null
   index 332de7b..0000000
   --- a/core/modules/entity_reference/src/ConfigurableEntityReferenceItem.php
   +++ /dev/null
   

   so this class should stay as empty BC class?

4. +++ b/core/modules/field/field.install
   @@ -26,3 +26,43 @@ function field_update_8001() {
   +  $item_class = 'Drupal\Core\Field\Plugin\Field\FieldType\EntityReferenceItem';
   

   can use ::class

So the patch still removes ConfigurableEntityReferenceItem, which we should not do. Let's make it @deprecated extending the Core one and otherwise empty.

I think we should also keep the plugin classes in src/Plugin for the Views integration stuff, with the same @deprecated pattern. We need to stop removing classes and leave them as empty @depreacted wrappers to keep the promise of not breaking contrib code.

So the patch still removes ConfigurableEntityReferenceItem, which we should not do. Let's make it @deprecated extending the Core one and otherwise empty.

Ouch! That was a mistake on my side when I built the patch. Sorry.

related to critical #2520526: Calculate configuration entity dependencies on install

there is a beta evaluation, so removing that tag

@claudiu.cristea does that mean you are working on it? if not, I can. (I'm pingable in IRC.)

adding
Needs framework manager review: http://cgit.drupalcode.org/drupal/tree/core/MAINTAINERS.txt means getting feedback from effulgentsia, catch, or alexpott
to the remaining tasks in the issue summary, for clarity.

@klausi :

#107.1 : MAINTAINERS.txt is a tough issue :-) Most of the entity_ref feature is already provided in Core/Field anyway, and Field is the only "component". There is no 'EntityReference" component (not technically in Core, and not in the list of "compnents" in issues).

Like with all the previous field types that were moved from modules to Core/File, this has the drawback of munging the issues under the "Field system" compnent, and making Field API maintainers the only apparent maintainers for them (which, personnaly, doesn't rejoice me ;-)).

But that is an existing issue (there's an issue open for this, can't find it atm), and given the timeframe, I'd rather not derail the patch on this. Also, in this case, it so happens that @amateescu is already a Field API maintainer, and that AFAIK @amitaibu has not really been involved with e_r after the initial patch :-)

So maybe can we punt on that ?

#107.2 .4: Wasn't aware of ::class. So those would become
'#process' => [[EntityReferenceItem::class, 'formProcessMergeParent']]
$item_class = EntityReferenceItem::class;
?
Why not, more readable indeed :-) Let's not block too much on those, though, there are many similar instances in core, and we can mass-simplify them later with a full search on '\Drupal\.*'

#107.3 : ConfigurableEntityReferenceItem - Oops, I was too quick when reviewing the interdiff #105 :-(

Removed the "Remaining task : remove the empty entity_reference.module" item from the IS

And agreed with @klausi #107 : "keep the plugin classes in src/Plugin for the Views integration stuff, with the same @deprecated pattern". Sucks, but that's the deal.

Working on it.

@yched
@alexpott added the tag in #39, I was putting it in the summary so we didn't forget about it.

[edit:]
ahh, the third papraphragh in #111 is reinforcing that it is ok to remove the section in maintainers. I see.

@YesCT : sure, I didn't modify that :-)

Note on views plugins: I renamed the deprecated plugins annotation labels to mark the also as deprecated. But I admit that I don't understand what happens if two modules are providing the same plugin type and id. And this is the case here, where we want to use those from Views for new views.

We can't have two plugin classes with the same plugin ID annotation. The annotation should now be provided by the plugin class in Views only. Just keep the empty class around with the @deprecated note.

We have a change here which we cannot avoid: the Views style plugin class for the "entity_reference" plugin is changed. I think that is ok, modules should not hard-code anything to the specific class of a plugin. As long as we keep the old class around we keep compatibility with all modules that specifically extended that class or used it in other ways.

Almost there claudiu :-)

+1, the real plugin implementations seen by the system are the new ones, the old classes don't provide plugins anymore, they are just empty shells kept there for contrib modules that might be extending from them for their own plugins,

Also, grammar in the @deprecated comments is a bit off :
"Use the [New\Classname] instead for extending from the [thing]."
Maybe just "Use [new class] instead." ?

Addressed all of @klausi's feedback, with one exception:

I think we should also keep the plugin classes in src/Plugin for the Views integration stuff, with the same @deprecated pattern. We need to stop removing classes and leave them as empty @depreacted wrappers to keep the promise of not breaking contrib code.

I don't think we can keep those because the plugin IDs would conflict. I'll leave that to you guys to sort out.

EDIT: ugh, cross-posted :/. Dropped the patch I was uploading, since Claudiu also made additional changes. I caught the problem @klausi pointed out in #118 early as you can see above :)

EDIT: DOUBLE cross-post. Drupal.org, you're super annoying in how incredibly bad you are at resolving cross-posts automatically. Complaining about the file field even after I've already removed them. Gahhhhhhhh.

+++ b/core/modules/entity_reference/src/ConfigurableEntityReferenceItem.php
@@ -7,218 +7,14 @@
+ * @deprecated in Drupal 8.0.x and will be removed in Drupal 9.0.x. Use
+ *  \Drupal\Core\Field\Plugin\Field\FieldType\EntityReferenceItem instead.

Can this be fixed on commit? An indent space is missed on the 2nd line :)

Hmm... other fixes.

One more catch.

Thanks !

Nitpick, can be fixed on commit :

+++ b/core/modules/field/field.install
@@ -29,7 +29,7 @@ function field_update_8001() {
+/**
+ * Fixes the entity reference fields storage type provider.
+ */
+function field_update_8002() {

A bit cryptic (and it's "field type provider" rather than "field storage type provide", which is not really a thing).
Could be simply : "The 'entity_reference' field type is now provided by core." ?

Other than that, yay, back to RTBC if green ?

@yched, thank you for the great review and guidance.

Fixed the 8002 docblock.

so the bot runs. [edit: cross post]

HEAD has changed.

Got a ping from @YesCT about this:

> and that AFAIK @amitaibu has not really been involved with e_r after the initial patch :-)

Indeed. As much as I like the idea of having my name in maintainers.txt, I wasn't involved in recent couple of years in ER, so feel free to remove me from there. I'll take credit for the initial ER patches in other occasions ;) Thanks for your hard work folks!

Given there's a bc layer is this feasible for a minor version?

@catch, I don't know if we want to go in the stable version with this solution where parts of entity reference are all around.

Feel free to remove my name from the credit list. My only involvement was an attempt at a reroll at #66. My reroll was unsuccessful.

Great work guys!

Discussed with @alexpott. Thanks for your work on this change! We definitely see the benefits of this change (better developer and site builder experience, less code to maintain, etc.). The fact that the patch retains BC also means that we could make this change now or in a minor version.

However, there are a number of risks with this change:

• There is a lot of reshuffling of plugin and config dependencies needed for the change. Existing configuration on existing sites would have stale dependency references, and so would need an upgrade path.
• The plugin, config, and module dependency changes also each increase the risk of exposing sites to other problems or bugs with dependency management.
• The field_update_8002() in the patch addresses dependencies in fields, but not in views.

For these reasons, I think this change would need an additional upgrade path and upgrade path test, and manual testing of the upgrade path with various installed schema versions and ER-dependent content and configuration. Even then, there would still be a risk of introducing upgrade path bugs that caused problems between earlier betas, beta16, and RC1. Given how close we currently are to a release candidate and that we already have had numerous bugs related to both upgrade paths for these sorts of changes and for configuration dependencies in general, we think that the risk of making this change at this point in 8.0.x is unfortunately too high.

I'd suggest that we add some documentation to the codebase and UI about the state of the module for 8.0.x (could be an rc target issue). @alexpott and I agreed that we should then pursue this change in 8.1.x, once we've had more opportunity to discover and resolve issues related to these sorts of upgrade paths and also more time to write and test the upgrade path for this change in particular.

Also retitling, since the current title (while amusing and quite searchable) is not specific and a bit, um, incendiary.

Thanks everyone! And thanks @alexpott for helping review the changes in the patch and this comment.

@catch: +1 to what @claudiu.cristea said. Removing the arbitray separation (Core has a full fledged e_r field, but you need an optional module that swaps the implemenation to be able to use it in Field UI) asap would really help cleanup unnecessary config dependencies before they spread all over (see #2520526: Calculate configuration entity dependencies on install that fixes places where they currently miss. Not having to deal with them while we don't actually need that would definitely be simpler)

Also, the current situation is pretty misleading because you *can* actually have configurable e_r fields without e_r.module (through the API or a module's config/default - just you cant use Field UI). So even the simple "e_r.module is for configurable e_r fields" assessment is actually an oversimplifiication that can lead to wrong assumptions and buggy code.

In short : still really in favor of doing this for 8.0.0 :-)

Oops - #144 was a reply to @catch #140, crossposted with @xjm #143. Reading.

Aw. Really sad that we have to keep this confusion, for the reasons explained in #144 :-(

At any rate, if this doesn't get in 8.0.0, we really need to get #2578249: Some e_r fields get the wrong Selection handler committed (fixes a nasty example of the "you can have configurable e_r fields without e_r.module" confusion I mentioned)

Also : this argument from #143 is actually incorrect :

The field_update_8002() in the patch addresses dependencies in fields, but not in views

The update removes 'entity_ref' as a dependency in *all* config records (fields, views, entity displays, ... whatever else). That's easy, and cannot really lead to more bugs down the road : entity_ref.module is just en empty shell, no-one, ever, has any reason to ever depend on it. Thus IMO the other arguments (might break more things with dependencies down the road) are kind of invalid as well IMO ?

If the premises for the decision in #143 are incorrect, any chance we can reconsider ?

Temptatively setting back to RTBC and "Needs framework manager review". Sorry for the trouble, feel free to kick back if need be.

I agree 100% with @yched. I think framework manager should revisit the decision in the light of #147. @yched counter every point made in #143.

Thanks @amitaibu for all your work and once again thank you for your initial ER port you are awesome see you in OG issue queue fighting with ER modules shortcoming ;-)

+++ b/core/modules/field/field.install
@@ -20,9 +21,48 @@ function field_update_8001() {
+/**
+ * The 'entity_reference' field type is now provided by core.
+ */
+function field_update_8002() {
+  $config_factory = \Drupal::configFactory();
+  /** @var \Drupal\Core\Field\FieldTypePluginManager $field_type_manager */
+  $field_type_manager = \Drupal::service('plugin.manager.field.field_type');
+
+  // Iterate on all configuration entities.
+  foreach ($config_factory->listAll() as $id) {
+    $changed = FALSE;
+    $config = $config_factory->getEditable($id);
+
+    // Update field storage configurations.
+    if (strpos($id, 'field.storage.') === 0) {
+      $class = $field_type_manager->getPluginClass($config->get('type'));
+      // Deal only with entity reference fields and descendants.
+      if ($class == EntityReferenceItem::class || is_subclass_of($class, EntityReferenceItem::class)) {
+        // Fix the type provider.
+        $config->set('module', 'core');
+        $changed = TRUE;
+      }
+    }
+
+    // Remove entity_reference module dependency from any configuration entity.
+    if ($dependencies = $config->get('dependencies.module')) {
+      if (($delta = array_search('entity_reference', $dependencies)) !== FALSE) {
+        unset($dependencies[$delta]);
+        $config->set('dependencies.module', $dependencies);
+        $changed = TRUE;
+      }
+    }
+
+    if ($changed) {
+      $config->save(TRUE);
+    }
+  }
+}

Here is the full update hook, for reference.

Thus IMO the other arguments (might break more things with dependencies down the road) are kind of invalid as well IMO ?

I disagree; that's not what #143 said. The point is that this change is not risk-free for existing sites, and we have very little time to make the change before RC or to find unexpected repercussions without risk to the RC upgrade path. I'm not saying it's clear-cut -- just that both the benefit and risk are high, which is why @alexpott and I discussed erring on the side of caution.

The point about this causing other dependency issues is worth considering, but I'm not sure it outweighs the risk.

I still don't see how Views ER plugin configuration dependencies are going to get removed in #149. I see it removing module dependencies and updating field storage dependencies.

@xjm : the views plugins impacted are a display, row and style plugin, used by a very specific feature of (Core) e_r fields : the ViewsSelection handler, that lets you define referenceable entities by a view rather than by bundles.

Those plugin implementations are moved, but the plugin IDs remain unchanged. So I don't think anything in existing views config records need to be changed ? (other than, no longer needs e_r.module, which the update does)

I missed the part of the update where all configs which depend on entity reference are being updated.

+++ b/core/modules/field/field.install
@@ -20,9 +21,48 @@ function field_update_8001() {
+        unset($dependencies[$delta]);
+        $config->set('dependencies.module', $dependencies);

This needs to be $config->set('dependencies.module', array_values($dependencies)); otherwise some odd numbers are going to start appearing in config dependency keys.

Have we got test coverage of a view that depends on entity reference being updated... it would be great if this view also depended on user so we can test for the bug avoided by doing the array_values() above.

I still agree with @xjm that this is risky.

Rerolled and added #152.

Passing for review.

Ouch, DrupalCI is lying. That should have failed. Opened a critical #2580293: Patch having test with "PHP Fatal error" is marked as PASSED.

+++ b/core/modules/field_ui/src/Tests/ManageFieldsTest.php
@@ -10,7 +10,7 @@
+use Drupal\field\Tests\EntityReference\EntityReferenceTestTrait ;

There are a few instances of a space before the semi-colon.

I have found 8.

- Enriched testFieldUpdate8002() with tests for a view using the e_r plugins, checking that the "referenceable entities based on a view" feature still works
- Moved EntityReferenceSettingsTest, that got added ine_r.module meanwhile.

Meh. Didn't intend to leave testFieldUpdate8001() commented out :-/

Those fails beat me. The field.storage.node.field_ref_views_select_2429191.yml I provide as a fixture causes a schema error when the recently added field_post_update_save_custom_storage_property() tries to resave it -" Exception thrown while performing a schema update. in Drupal\Core\Entity\Sql\SqlContentEntityStorage->wrapSchemaException()"

Same error if I completely comment out the field_update_8002() update we're adding here, so it's not the fault of the code in there.

Have to run, hope @claudiu.cristea can find out more :-/

BTW, unrelated to the fails, but to leave clean dependencies, field_update_8002() should rather do

-        $config->set('dependencies.module', array_values($dependencies));
+        if ($dependencies) {
+          $config->set('dependencies.module', array_values($dependencies));
+        }
+        else {
+          $config->clear('dependencies.module');
+        }

We should roll that in the next patch.

Other than those weird fails, the asserts about the view still working do pass, btw. It's just the post_update that chokes on my test field...

It's something related to field_post_update_save_custom_storage_property()