Remove Taxonomy term reference field in favor of Entity reference

setting right status then

+    // @todo: We should pass the view-mode, however we get view-mode as "full" which doesn't
+    // exist.
+    $display = entity_get_display($element['#entity_type'], $element['#bundle'], $element['#view_mode'] != 'full' ? $element['#view_mode'] : 'default');
+    $options = $display->getComponent($field_name);
+
+    if ($options['type'] != 'entity_reference_label' || empty($options['settings']['link'])) {
+      // Formatter is not a label with link.
+      continue;
+    }

^ This looks bogus. I guess you are looking for the formatter type and settings, don't you have that in $element already? (You have the formatter type for sure in $element['#formatter']).

You get $element['#formatter'] = the plugin_id of the formatter.
You don't have the formatter settings in $elements, though.

Right now if you want to get the settings of the actual formatter object being used for render, you should do something like :

$formatter = entity_get_render_display($entity, $view_mode)->getFormatter($field_name);
$my_setting = $formatter->getSetting('my_setting');
// or
$settings = $formatter->getSettings();

[edit: obviously we could put more stuff in $element: the $display object, the $formatter plugin object... My only worry is serialization of all of those in case of render caching]

That hunk is in rdf_field_attach_view_alter() - hm, crap, currently #1875970: Pass EntityDisplay objects to the whole entity_view() callstack leaves out hook_field_attach_view_alter().
Can't remember right now if that was a deliberate omission, or whether it was just because I'd so wish to get rid of those hook_field_attach_*() hooks in favor of the the existing hook_entity_*() hooks - which is not that easy.

But yes, while it's there, hook_field_attach_view_alter() should probably receive the $display too. I'm not sure I'll be able to update the #1875970 patch before a couple days though. Meanwhile, the code in #22 should work.

Converting fields from one type to another - aw, no, I don't think that's something that was ever done in core. Doesn't ER D7 have some support code for converting old [node|user]_reference fields, though ?

I guess it will be fiddling directly with the schema and some field_update_field()

field_update_field() doesn't allow changing the field type.
But using it is not allowed within an update func anyway - direct writes to the {field_config*} tables instead.

For field values: well, you will only be able to handle the case of fields stored in field_sql_storage. Fields stored in contrib storage backends are out of reach...

Functions like field_read_fields(), field_update_field(), field_update_instance(), entity_get_display() and loading config entities are not supposed to be used in update functions, because they fire hooks.

- working on field and instance definitions has to be done by directly accessing the definition records (currently rows in the field_config* db tables, in the future raw CMI files). There are a couple helper _update_7000_field_*() functions already to that effect, but currently no function to update definitions though...
- working with $entity_display config entities can be done with _update_8000_entity_get_display() (update-friendly replacement for entity_get_display(), works on the underlying CMI data, not on the config entity). See user_update_8011() for an example.

Here's some progress.

And with even more fixes and rollbacks of unnecessary changes :)

The autocompletion changes look suspect. Looks like we're dropping a bunch of tests related to autocomplete, but not adding any tests for them? Are there already existing tests for the path entity_reference/autocomplete/tags' . $tag_field_name . '/' . $tag_field_instance['entity_type'] . '/' . $tag_field_instance['bundle']? This looks like it doesn't even match the entity_reference_menu() entry since there is no backslash in-between 'tags' and $tag_field_name.

A few more quick things just based off patch read:

A lot of the tests in TermFieldTest seem like they could still be valuable, especially testTaxonomyTermFieldChangeMachineName()

Am I able to create an entity reference field that references not taxonomy terms and still select the entity_reference_autocomplete_tags widget? Should I actually be able to do that since my gut says no I shouldn't since it's very taxonomy term specific.

That's exactly what I said in IRC a couple of minutes ago, that we might've dropped too many autocomplete tests :/ I'll look into the changes prior to #43 tomorrow.

Am I able to create an entity reference field that references not taxonomy terms and still select the entity_reference_autocomplete_tags widget? Should I actually be able to do that since my gut says no I shouldn't since it's very taxonomy term specific.

The autocomplete tags widget is not taxonomy term specific at all, just the auto creation of terms part of it, and that has been moved already to the taxonomy implementation of the entity reference Selection plugin.

It looks like entity_reference module should actually support hook_field_attach_rename_bundle() to automatically fix any bundle renames for all entity types in all entity reference field configurations. That code shouldn't be living in taxonomy module anymore. Should also move testTaxonomyTermFieldChangeMachineName to a generic test in entity_reference.

Ok, i generated 10 nodes, added a few terms on them, applied patch run update.php.
When i visit /node teasers appear fine, that means that data where succesfully transferred to the entity reference field.

But when i try to edit a node:

Warning: array_key_exists() expects parameter 2 to be array, null given in Drupal\field\Plugin\PluginSettingsBase->getSetting() (line 50 of core/modules/field/lib/Drupal/field/Plugin/PluginSettingsBase.php).
Fatal error: Unsupported operand types in /var/www/d8/core/modules/field/lib/Drupal/field/Plugin/PluginSettingsBase.php on line 60

This also happens when i try to add a new node that has a taxonomy er field attached.

Field UI on the article content type again:
Notice: Undefined index: weight in Drupal\field_ui\FieldOverview->form() (line 112 of core/modules/field_ui/lib/Drupal/field_ui/FieldOverview.php).
Notice: Undefined index: weight in field_info_max_weight() (line 519 of core/modules/field/field.info.inc).

didnt have time to dive through the code, but a quick scan i spotted this..i know very minor, compared to upgrade path

+++ b/core/modules/taxonomy/taxonomy.installundefined
@@ -337,3 +338,127 @@ function taxonomy_update_8006() {
+  $module_handler = drupal_container()->get('module_handler');
...
+  drupal_container()->get('kernel')->updateModules($module_filenames, $module_filenames);

i suppose you could store container in a variable the first time

Note that the autocomplete logic has an RTBC patch to turn it into a pure route: #1801356: Entity reference autocomplete using routes. That will likely land first, which will impact this patch.

Yep, and the impact is that we'll need to basically undo that patch and implement it again for entity reference :(

Since this patch would render that one redundant, what are the odds of this one actually landing? If they're high, I'm happy to postpone that one and let this issue steal its model and do it for ERs now, in this issue.

i am pretty sure, this was one of the main reasons to get entity reference into core in the first place, so i would say confidently that this goes in

Talked to catch in IRC. Marking #1801356: Entity reference autocomplete using routes as postponed on this one, on the assumption this issue will take care of such conversions.

To be honest, I think I'd prefer to just repurpose/re-title that one to 'Entity reference autocomplete using routes", because we already have a >100K patch in here, and, even more important, these tasks can be done in parralel, no need for anyone to wait on each other.

Ok, fixed all the problems mentioned by @rootatwc and @Dave Reid:

d7200fa - Fix upgrade path and View UI wizard.
9af70f2 - Added an upgrade path test.
097aa82 - Implemented hook_field_attach_rename_bundle() in ER, simplifying the Vocabulary storage controller.
ae9956b - Implemented hook_field_attach_delete_bundle() in ER, simplifying the Vocabulary storage controller even more.
f36ad4b - Revert some unnecessary (and also incorrect) changes.
ec36f2e - Add back some tests from the taxonomy term reference field.

Also reviewed and corrected everything that seemed suspicious from the changes prior to #43. The relevant tests were added back in EntityReferenceFieldTest and EntityReferenceAutocompleteTest.

*sigh*.. random failure from the new ckeditor module.

Upgrade path looks good, all my term references are still there, even multiple tags..
only problem i can find is when i try to create a new article, i cant specify more than one tag..second tag always overwrite the first:/

only problem i can find is when i try to create a new article, i cant specify more than one tag..second tag always overwrite the first:/

That's not related to this patch, I opened a bug report for it here: #1914180: Unbreak and cleanup the autocomplete widgets

So.. the patch from #58 is green and in need of reviews :)

#1914180: Unbreak and cleanup the autocomplete widgets fixed it indeed! that was a quick commit;)
had a couple of minutes and took a look again..i am sorry, i cant find time, esp this week:/
UI and update looks good.next time this gets rerolled:

+++ b/core/modules/entity_reference/entity_reference.moduleundefined
@@ -420,6 +420,76 @@ function entity_reference_query_entity_reference_alter(AlterableInterface $query
+ * Implements hook_field_attach_rename_bundle().

it implements field_attach_delete though

Yeah, that was already fixed in the sandbox :) Here's a new patch with more code removal (yay!).

8dfc0e8 - Remove taxonomy term reference views integration. This will be added to ER in #1906806: Provide a relationship for each entity reference field.
7f347c4 - Fix typo.
2a8ec81 - Remove obsolete TaxonomyTermReferenceItem class.

berdir suggested this instead of #1120144: Term Reference Autocomplete widget - add option to match only start of term and expose settings (size, match)

+++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Tests/EntityReferenceFieldTest.phpundefined
@@ -0,0 +1,141 @@
+
+/**
+ * @file
+ * Definition of Drupal\entity_reference\Tests\EntityReferenceFieldTest.

Usual missing \ in various files and should be Contains.

+++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Tests/EntityReferenceFieldTest.phpundefined
@@ -0,0 +1,141 @@
+
+/**
+ * Tests for taxonomy term field and formatter.
+ */
+class EntityReferenceFieldTest extends DrupalUnitTestBase {
...
+
+  /**
+   * Test term field validation.
+   */
+  function testTaxonomyTermFieldValidation() {

That test class is a bit strange, comment/method is about taxonomy but class name isn't, description says it's about creating reference fields and it doesn't even seem to depend on taxonomy?

Is this a moved taxonomy term reference test?

I also have an issue that introduced a entity test base class that would simplify the dependency handling here.

+++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Tests/EntityReferenceFieldTest.phpundefined
@@ -0,0 +1,141 @@
+    $this->installSchema('system', 'variable');
+    $this->enableModules(array('field', 'field_sql_storage', 'field_test'));

enableModules() will soon no longer install the tables, you will have to do that yourself.

+++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Tests/EntityReferenceFieldTest.phpundefined
@@ -0,0 +1,141 @@
+      'bundle' => 'test_bundle',
+      'entity_type' => 'test_entity',

I'm in the process of replacing test_entity with entity_test, which is already NG. Can we change these tests to use that already?

+++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Tests/Upgrade/EntityReferenceUpgradePathTest.phpundefined
@@ -0,0 +1,63 @@
+    $this->assertNoLink('Term reference', 'There are no Term reference fields.');

Given that we will delete the code field type, this seems like a useless assertions, should probably check the field instance information instead.

+++ b/core/modules/taxonomy/taxonomy.installundefined
@@ -337,3 +338,115 @@ function taxonomy_update_8006() {
+  if (!$fields = field_read_fields(array('type' => 'taxonomy_term_reference'), array('include_inactive' => 1))) {

Needs to use the update save field API functions.

Fixed all the issues raised in the review above. Yes, that test was indeed a moved taxonomy term reference test :)

I spoke to @Berdir on IRC and he agreed to leave EntityReferenceFieldTest as is here and clean it up along with all the other tests that use test_entity in #1822000: Remove Drupal\field_test\Plugin\Entity\Type\TestEntity in favor of EntityTest.

P.S. I couldn't merge latest HEAD in the branch that was previosuly used for this issue, so I pushed everything to a new one, starting with the patch from #65.

Reverted the change that broke all those upgrade tests.

Merged HEAD.

This should do it.

+++ b/core/modules/field/field.installundefined
@@ -228,13 +233,34 @@ function _update_7000_field_create_field(&$field) {
+    // We don't use drupal_write_record() here because it depends on the schema.

Same applies for the update case.

Also the introduced dependency of er on taxonomy module should be removed

You're right, I don't know what I was thinking when I added that dependecy :/

awesome, thanks!
I run one more manual upgrade, check, everything works as should and cant find anything to complain about the patch
good job!

Merged HEAD.

The first testbot result was a fluke, we're good to go here :)

This patch contains the fix for #1942106: Entity reference and revisions , so i am bumping this to major

Re-rolled, hope @amateescu wasn't already on it as well.

Not 100% sure about some parts e.g. in rdf.module. Let's see what the tests say.

Nope, I wasn't :) I was planning to reroll tomorrow, but thanks for taking it on. Can you please use the branch for this? http://drupalcode.org/sandbox/yched/1736366.git/shortlog/refs/heads/1847...

Let's see if that fixes the tests.

Weird that insert and update in forum.module are implemented completely different, wondering if there's a reason for that.

@amateescu: Oh, wasn't aware of that branch and did it using rebase, so can't push to that anymore. Pushed it to http://drupalcode.org/sandbox/yched/1736366.git/shortlog/refs/heads/1847... now.

I started this earlier today, and since then some patches have been posted. So some of this might be addressed already.

+++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Tests/EntityReferenceFieldTest.phpundefined
@@ -0,0 +1,145 @@
+  function setUp() {

should be public function
http://drupal.org/node/325974

+++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Tests/EntityReferenceFieldTest.phpundefined
@@ -0,0 +1,145 @@
+   * Test reference field validation.
+   */
+  function testEntityReferenceFieldValidation() {
...
+   * Tests that vocabulary machine name changes are mirrored in field
+   * definitions.
+   */
+  function testEntityReferenceFieldChangeMachineName() {

should be one line,
and consistant in terms of Test ... or Tests.
See: http://drupal.org/node/1869794#comment-7173722
Tests ... would be consistant with 1354.

+++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Tests/Upgrade/EntityReferenceUpgradePathTest.phpundefined
@@ -0,0 +1,66 @@
+ * Performs major version release upgrade tests on a populated database.
+ *
...
+ */
+class EntityReferenceUpgradePathTest extends UpgradePathTestBase {

I'm not sure what the pattern is for this one line. http://drupal.org/node/1869794#comment-7173778

+++ b/core/modules/field/field.installundefined
@@ -347,9 +373,16 @@ function _update_7000_field_read_fields(array $conditions = array(), $key = 'id'
+ *   Optional; Indicate if we need to create a new instance or update an existing one. Defaults to FALSE.

should be (optional)
http://drupal.org/node/1354#param
also needs to wrap at 80 chars.

Pushed a number of fixes for the tests to my branch, TermTest still partially fails, something to do with preview.

Ok, so I had to add a separate item class for entity_reference fields as the core doesn't provide everything that it needs: revision_id, label (for auto-create) and access.

No interdiff because I merged in 8.x instead of rebase but you can see the comments here: http://drupalcode.org/sandbox/yched/1736366.git/shortlog/refs/heads/1847...

Reviewed the changes and although I was a bit sad that we have to extend the core ER item class, I got over it quickly for the sake of progress :)

This was RTBC before the Node NG conversion and subsequent patch were mostly rerolls.

This was already rtbc, and I would have not mentioned the nit, if I had not found something that needed to be fixed otherwise.
I was looking to see if the things I mentioned before were addresses, not reading the code to see if it makes sense.

==============

1. (nit) 80 char limit on comment (not blocking)

+++ b/core/modules/rdf/rdf.moduleundefined
@@ -783,22 +783,37 @@ function rdf_field_attach_view_alter(&$output, $context) {
+    // @todo: We should pass the view-mode, however we get view-mode as "full" which doesn't
+    // exist.

wrap at 80 chars.

"Lines containing comments (including docblocks) must wrap as close to 80 characters as possible without going over, "
http://drupal.org/node/1354#drupal

2. this is blocking commit I think

+++ b/core/modules/taxonomy/taxonomy.moduleundefined
@@ -1348,6 +1018,7 @@ function taxonomy_rdf_mapping() {
 /**
+<<<<<<< HEAD
  * Implements hook_field_presave().
  *
  * Create any new terms defined in a freetagging vocabulary.

@@ -1363,6 +1034,8 @@ function taxonomy_field_presave(EntityInterface $entity, $field, $instance, $lan
 /**
+=======
+>>>>>>> applied patch

left over merge conflict notes.

3. question about @see

+++ b/core/modules/views/includes/ajax.incundefined
@@ -148,7 +148,7 @@ function views_ajax_form_wrapper($form_id, &$form_state) {
+ * @see entity_reference_autocomplete_tags()

I looked for this function, but didn't find it.

I looked in this patch, and also in a recent pull on core: $ grep -R " entity_reference_autocomplete_tags(" *

Where is it? Maybe it's referring to a plugin definition in a comment (annotation?) or a method which is camelCase now?

4. css changes. (just a question, not blocking anything)

+++ b/core/themes/bartik/css/style.cssundefined
@@ -117,7 +117,7 @@ ul.contextual-links,
+div.field-type-entity-reference,

the css changes (there are a bunch), look like the css was just for taxonomy term references, but now will effect all entity references.

Is there a case we can look at to see what a reference looked like before that was not a taxonomy term reference, and what it looks like now?

Is there a way to refer to just a taxonomy kind of entity reference in the css?

Do we want these to be general or just taxonomy entity references?

5. Issue summary

Updating the issue summary with the issue summary template might make committing this easier.
I'm thinking the sections on API changes and UI changes (might just be clarifying that there are none) would be helpful.
Also, maybe a commit credit recommendation since it looks like some work was done in a sandbox.?

========
patch coming right away.

+++ /dev/nullundefined
@@ -1,108 +0,0 @@
- * @Plugin(
- *   id = "taxonomy_autocomplete",

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/TermTest.phpundefined
@@ -143,7 +147,7 @@ function testNodeTermCreationAndDeletion() {
-      'type' => 'taxonomy_autocomplete',
+      'type' => 'entity_reference_autocomplete_tags',

maybe I look for a plugin like..
* @Plugin(
* id = "entity_reference_autocomplete_tags",

I'm going to have to learn how to do multi line grep like things.

$ grep -R 'id = "entity_reference_autocomplete_tags",' *
core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/field/widget/AutocompleteTagsWidget.php: * id = "entity_reference_autocomplete_tags",

So, how does the api autogeneration of docs work with plugin definitions? Is @see whatever() still the right syntax?

This addresses 1, 2, 3 from #95

I looked at http://drupal.org/node/1354#see

Follow the @see tag by a space, and then the item or URL you want to reference. An item can be a function name, class name, method name (with the class), constant name, file name, group identifier, or a URL.

Since it didn't say anything about a plugin id, I went with the class name.

And rootatwc and swentel helped me sort out the @see part.

Thanks for the re-roll. The taxonomy_field_presave function should be completely removed, there should be no remaining taxonomy_field_* functions.

@Berdir is that removal of that function something I should do right now? or separate follow-up issue? [edit: doing it. I think it should be done here.]

(I canceled the testbot, because I found a small whitespace thing in the patch. new one coming right away)

Now, removing them is the point of this issue :) Not sure what happened there with my re-roll.

Also, if we're being nitpicky, i think the second line of the @todo that you changed should be indented 2 spaces.

Thanks!

yep. :)

This fixes the indent of the @todo lines this patch changes (which was a nit, we could have let slide) and removes the function (which needed to be done).

@Berdir what about these:

$ grep -R "function taxonomy_field_" * | grep -v patch | grep -v "orig:" | grep -v interdiff
core/modules/taxonomy/taxonomy.install:function taxonomy_field_schema($field) {
core/modules/taxonomy/taxonomy.module:function taxonomy_field_extra_fields() {

[edit: I'm investigating]

I got some help from @dawehner and @yched in irc.
Not 100% sure. But here is my thinking.

There are taxonomy terms. Those have fields. This issue is not about removing fields on terms.
core/modules/taxonomy/taxonomy.module:function taxonomy_field_extra_fields() {
is about the fields on the terms, so that is staying in. [edit: corrected typo]

core/modules/taxonomy/taxonomy.install:function taxonomy_field_schema($field) {
is about adding a field to other things to reference the tid of terms, that is coming out.

good catch, hook_field_schema implementation should go

the css changes (there are a bunch), look like the css was just for taxonomy term references, but now will effect all entity references.
.....
Is there a way to refer to just a taxonomy kind of entity reference in the css?

Those look like good questions. Seems like this patch will make #1218640: Bartik displays all taxonomy term fields in a smaller font than other fields worse (will make it occur for all entity reference fields) and also will result in taxonomy terms not getting any special classes in the CSS which allow themers to distinguish them from other entity reference fields?

***

Also, has there been any discussion of the user interface changes here? It got hard enough to add simple taxonomy fields to content types going from Drupal 6 to 7 :) But now with this patch, in Drupal 8 you won't even be able to select "term reference" in the dropdown on the Manage Fields page in the Field UI (but rather will need to figure out to select something labeled "entity reference" instead and configure it further from there)?

The original issue was reviewed by the UX team and there is also #1847590: Fix UX of entity reference field type selection.

I looked a bit into the css class issue and I found that the field subtype concept (basically, plugin derivatives after fields are converted to plugins) would help a lot for this as well.

In the meantime, I think the best we can do is something like this interdiff.

from looking at the code, the css changes will now keep only effecting taxonomy terms.

if we need a follow-up, we can make one, to discuss if other references should be similarly styled and if taxonomy terms should also have a common entity reference class in common with the other types of reference.

Has anyone manually tested this? tagging for that.
I'm guessing that we want before and after screenshots to show no visual difference.

@amateescu is there an issue regarding field subtype concept? I did a search but didn't see one.

@YesCT, I'm pretty sure @rootatwc did a lot of manual testing on this, but I'll let him confirm. And for the subtype issue, it's the one linked by @Berdir in #108.

Yeah i have manually tested twice..seems css changes have some problems, see screenshots
Before

After

Yeah, I forgot to add 'field-type-' in front the class name, my bad :/ Also fixed those exceptions.

exactly the same now, thanks!

Thanks @rootatwc @Amitaibu et al. We tested this patch live this morning with @Dries, @webchick, and a couple others.

We have a few concerns about the usability. Most of these things aren't specific to the taxonomy implementation; they're existing patterns in entity reference or the Field UI. However, this patch would take the new entity reference interactions from being a "10%" site building task to a "90%" site building task--most site builders are going to have to go through this interaction at some point. (I am the taxonomy component maintainer and it still confused me.) ;) So, tagging for usability review. We should be able to create some separate issues to fix some of the entity reference usability issues, but we need to be careful about adding this change.

I'll also follow up with some screenshots and specific things we found confusing, and also review the patch some more. Thanks for keeping this issue going! It will be great to standardize on a single API.

Alright, here's some screenshots illustrating the UX issues we encountered when testing this. To reiterate, we realize these issues are mostly a result of the default behaviors of entity reference fields or the field API in general, but this patch still represents a usability regression from D7 taxonomy, which is why we need to solve at least some of this if we're going to replace the legacy term reference field type.

Note: This is written from the perspective of a reasonably experienced Drupal site builder, but a lot of these things actually confused me (the component maintainer for taxonomy!) as well as the two core maintainers who were watching me test it. ;)

1. I've already created my vocabulary and added terms to it, because that's what I've done first ever since Drupal 4.7. Now I want to add it to my posts.

   

   We really shouldn't be exposing the word "entity" to users, the same way we don't expose the word "node".

2. Even once I figure out to select entity reference (spoiler alert: I read the issue title), it's still not obvious how to make it a term reference field. The maximum value option is first, and most of the time I really don't care what that is. It turns out the "Target type" field is what I actually want, but I have to click on the dropdown to figure that out, and it's the last thing on the form even though it's the most important.

   

3. The options for maximum value are weird.

   

4. I try "more" to see WTF it does, and wonder why we couldn't just use this widget in the first place. Skip the dropdown, let me just type in a number, and instead have a toggle for "unlimited values" that hides the number widget.

   

5. Only after all that, when I click on "Target type" to see what it is, do I finally get the option to make this a taxonomy reference field. (Also, why aren't the options in alphabetical order?)

   

6. The next screen is overwhelming (which has always been a problem with the field UI), but it used to be that I could just scroll past it all and click save.

   

7. When I scroll past everything and click save like I'm used to doing, something funny happens. Oops!

   

8. So I go back to editing the field and realize my mistake. All the vocabularies were checked by default under that "Reference whatever" fieldset I didn't know what to do with. That seems like an unusual default--usually, I just want to create a field to reference one vocabulary, or one kind of node, or whatever. Referencing multiple bundles is a nice feature, but definitely not the 80% usecase.

   

9. Also, when I tried to add an autocomplete widget, I was confronted by these two unfamiliar options. I had no idea what the difference between "Autocomplete (tags style)" and "Autocomplete" was. (Me, IRL, and I've even used the D7 entity reference contributed module. At first I thought it might have to do with whether terms were autocreated, because that's what "tagging" meant for vocabularies in D6, and I never did figure out what it actually meant until moshe weitzman and effulgentsia explained it to me like ten minutes later.)

   

1) We could rename to just "Reference" mayhaps?

2) I think I tried to put the 'Target type' on top but I couldn't because of a Field API limitation IIRC.

3) and 4) are totally unrelated, I don't see why they were included in this review?

For 5) we could expose a radio widget instead of that select.

For all the others, suggestions are always welcome :)

Could we for ease of use, continue to have a separate field type 'term_reference' that is owned by taxonomy module, but extends entityreference in the same way that image field 'extends' the file field? That way it's more clear to site builders. And we can technically do some different UI things to make this process easier since they are separate fields, but not really?

Re #121: I think that would be a huge step in the wrong direction. Why would only taxonomy entities receive special treatement and UI improvements and not all referencable entity types?

Ok, so just not taxonomy terms, but if field types are or will be plugins, they should have automatic derivative plugins for each referencable entity type... It would be nice to never even see the word 'Entity reference' at all. Because entity reference fields cannot reference more than one entity type in it's configuration, why don't we just list every referencable type as a field type? I would make a content reference field instead of entity reference. That also make way more sense to me in a site builder role.

That's why we have this issue open :) #1847590: Fix UX of entity reference field type selection

1) We could rename to just "Reference" mayhaps?

I think that's a good first step until #1847590: Fix UX of entity reference field type selection is resolved. (That issue could use a clearer summary and/or title.)

3) and 4) are totally unrelated, I don't see why they were included in this review?

Because I just made a list of everything that came up during our testing, as a hypothetical site builder used to D7, and it was part of the overall WTF. :) As I stated, we acknowledge that's a problem in the field API itself. @swentel found #680546: Add a custom 'number of values' for 'multiple / multi-value' field. (e.g. nodereference, imagefield) for me and we can address that there.

For 5) we could expose a radio widget instead of that select.

Our current interface guidelines always use single-select boxes instead of radios. I'd prefer to give the field a more meaninful label instead, and eventually move this option up a step in #1847590: Fix UX of entity reference field type selection.

7,8) would be easy to fix..dont have anything checked as default, then checkboxes are required and if user misses it, form wont submit

Oh, one other thing:

2) I think I tried to put the 'Target type' on top but I couldn't because of a Field API limitation IIRC.

Worst case, we form alter the darn thing until #1847590: Fix UX of entity reference field type selection is resolved?

7,8) would be easy to fix..dont have anything checked as default, then checkboxes are required and if user misses it, form wont submit

Mmm, in general, that's not good UX. What should actually happen (ignoring current field API and entity reference limitations for the moment) is that the bundle options only should be at the very top of the form, after the label, or even on the previous screen. (Yes, I know it's on this screen b/c of field settings vs. instance settings.) :)

Edit: See #1880168: Introduce top-level sections for all forms.

Re #126, and #128:

I'd prefer to give the field a more meaninful label instead, and eventually move this option up a step in #1847590: Fix UX of entity reference field type selection

That issue is only about discussing subfields/field derivates/whatever, I don't think fixing Field UI to allow form elements to sit above the maximum value option would be in scope there.

Re #129:
That issue you linked there states this as the proposal: "Simplify forms used in contextual administration". I don't see how the field instance form is 'contextual administration'..

That being said, I guess @rootatwc's proposal is the easiest way out for now. We don't like that all bundles are checked by default and we don't want users to skip that very important form element? Make it required :)

I'm really sad that Entity reference has become the dumping ground of everyone's frustrations with Field UI. I've seen a lot of reviews and comments that actually bash Field UI, but not *one* issue opened. Why is that? We have a field_ui component afaik, can we please try to fix it there instead of putting the burden on ER?

One possible track for breaking out of the constraints of the current shape of the "select field type" dropdown in Field UI's "add new field" UI could be to remove those "add new field" / "add existing field" as rows in the "Manage Fields" table, and move them to action buttons at the top of the page
Those would lead to separate form steps where the selection of the field type would have more screen real estate available to explore more suitable UX formulations.

Note: 3. is being handled in #680546: Add a custom 'number of values' for 'multiple / multi-value' field. (e.g. nodereference, imagefield) and does not overlap at all with this issue, it's the same behavior for all field types. If that is important to be fixed, raise the priority there :) This does not in any way block this issue, it's the exact same behavior for all field types.

Edit: Just noticed that this was already in #126. I think what we need at this point is an updated summarized response to the points raised in the in the review so that we know what already was addressed, what will be addressed where and possibly some additional follow-up issues. I don't really see any remaining commit blockers..

@yched: Yes, I think that would make sense and there are issues for that I think. There are also accessibility problems with the current solution.

tagging for issue summary, especially a remaining tasks section, and updating the follow-up issue section.

+++ b/core/modules/entity_reference/entity_reference.moduleundefined
@@ -29,12 +29,27 @@ function entity_reference_field_info() {
+function entity_reference_data_type_info() {
+  return array(
+    'entity_reference_configurable_field' => array(
+      'label' => t('Configurable entity reference field item'),
+      'description' => t('An entity field containing a configurable entity reference.'),
+      'class' => '\Drupal\entity_reference\Type\ConfigurableEntityReferenceItem',
+      'list class' => '\Drupal\Core\Entity\Field\Type\Field',
+    ),
+  );

I've noticed in another issue that this wouldn't be necessary if we also fix field_data_type_info() to use the data_type key.

Like this?

#132 sounds like a great thing to explore. I re-scoped #1847590: Fix UX of entity reference field type selection; let's discuss that there.

That issue you linked there states this as the proposal: "Simplify forms used in contextual administration". I don't see how the field instance form is 'contextual administration'.

It's a similar problem space, is my point. The pattern there is also one of the patterns we could use to fix the ER fields settings page.

Meanwhile, let's:

1. Move the number of values field below the "Target type" field. If it's a Field API limitation, then let's form alter it and file a followup to fix the limitation properly.
2. Change the "Target type" label to something more meaningful. The best I could come up with sitting here is "What to reference." Other suggestions?

Actually, to keep this patch from getting monstrous, let's spin these generic ER fixes off into their own issues.

Filed #1953568: Make the bundle settings for entity reference fields more visible..

Move the number of values field below the "Target type" field. If it's a Field API limitation, then let's form alter it and file a followup to fix the limitation properly.

FWIW, I have no issues with patching Field UI to display:
- first, settings specific to the field type,
- then, generic settings present for all field types (i.e "number of values")
instead of the other way around right now. I'd just prefer it to be consistent across all field types, not just special cased / form_altered for entity_reference fields.

Filed #1953770: Move the field-specific settings form elements at the top of the form.

Other issues:
#1953444: Make 'target bundles' required on the Entity reference instance settings form
#1953438: Don't expose the term 'Entity' to users
#1953832: Replace 'target type' on entity reference field settings with something clearer

A proper issue summary

Like I said in #107 the removal of "term reference" from the initial dropdown really seems like the biggest issue here (especially since the others all have nice followups). It's the one issue that's directly caused by this patch, and if no one can figure out how to get past the first screen they won't even have an opportunity to flounder on the next ones :)

I'm probably going to get laughed at for this, but here's one suggestion for how to fix it (see attached). Maybe #1847590: Fix UX of entity reference field type selection will eventually come along with something better, but I think this brings the patch here towards a point where it could be committed.

I also found it interesting that the Taxonomy module does not have a dependence on Entity Reference after this patch. Technically that might be true, but how useful is the module without it? What will it mean for someone trying to turn on basic taxonomy functionality from the modules page?

It also means that taxonomy_help() is left completely incorrect after this patch (actually, it might be a bit incorrect either way, but it's very incorrect if there is no dependency on Entity Reference)....

Adding on to my not-a-patch, here's another related improvement (this one for the second screen in the field UI). The module already has some logic to try to guess the most likely target type (and pre-fill it in the dropdown), and since everyone seems to agree taxonomy terms are the most likely.... why not just use that?

The second screen then looks like this as soon it loads:

The type has to come before the number of values though.

& I just posted https://drupal.org/node/1847590#comment-7230112 with an idea for how to introduce the References concept in the first place

The type has to come before the number of values though.

Agreed. This is now being fixed in #1953770: Move the field-specific settings form elements at the top of the form.

InvalidArgumentException: Property revision_id is unknown. in Drupal\Core\Entity\Field\Type\EntityReferenceItem->setValue() (line 84 of /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Entity/Field/Type/EntityReferenceItem.php).

Hm. Does this have to do with this patch?

That was fixed in #1956434: Can't edit nodes with entity reference fields.

I think the suggestions here will help mitigate the small UX regression from D7's taxonomy term reference field, but I'm still worried about the huge UX regression that was introduced in D7 when we started requiring people create and customize fields for taxonomy in the first place.

What we need to get back to is a checkbox on the entity we are referencing that automatically creates (and customizes) a field based on the settings of the entity. For example: if we define a vocabulary as being free tagging, the term reference field widget should be autocomplete. Having site builders specify the settings once on the vocabulary and then again on the field is unnecessary.

Would it be a lot of work to put back the UI from Drupal 6?

Ideally, all a site builder should need to do is check a box to make the entity they just configured have a field on another entity.

Such a UI in D8 (or even D7) would have to account for all bundles of all entity types and, thus, can quickly get overwhelming. Entity translation contains a page where you can enable translation settings for all entites, bundles and fields and it took a lot of time to get it right. Conceptually we should stick with fields, but if we are sure that #154 is the way to go it would definitely be doable.

No one laughed @David_Rothstein

Is a way forward here to use that patch #145 and #147 ?
I think we also here need to fix the dependency and the help.

Make #154 a follow-up (issue needs to be created. is there one left over from D7.. a regression or something?)

We need to identify which of the related issues listed in the summary needs to get fixed before this, which are *needed* follow-ups and which are just... related.

And then let #1847590: Fix UX of entity reference field type selection fix it further.

trying to identify next steps

http://drupal.org/patch/reroll

other changes should be done *after* a reroll patch is posted so that we can get a meaningful interdiff of them. (http://drupal.org/documentation/git/interdiff)

That would be pointless until #1818560: Convert taxonomy entities to the new Entity Field API is in.

adding reroll to remaining tasks with link to instructions

added info about what this issue is postponed on

For #154, there are a couple possibilities:

• #889378: Create taxonomy fields from the taxonomy administration page
• #394482: Users should be able to enable a vocabularly from the content type configuration page

Re: #154, we needn't allow all entity types on the content creation form. Since taxonomy is the 80%+ usecase, I think it would be feasible to special-case it on the content type creation form, and then let the field UI present the generic ER. Let's discuss whether or not we want to do that in #394482: Users should be able to enable a vocabularly from the content type configuration page. It could provide a workflow similar to what I describe in #889378: Create taxonomy fields from the taxonomy administration page.

added the autocomplete starts with setting issue to related issues

Filed #1963340: Change field UI so that adding a field is a separate task.

Updated issue summary.

Removing sprint tag as it's no longer a blocker for my term NG conversion issue.

In #133, @Berdir asked for an updated summarized response to the points raised in the large UX review (#119). So just to keep track:

#1 & 5: being discussed in #1847590: Fix UX of entity reference field type selection, so not our business here.

#2: 'target type' renamed in #1953832: Replace 'target type' on entity reference field settings with something clearer, will be refactored in #1847590: Fix UX of entity reference field type selection, but for now we might want to form alter it.

#3 & 4: fixed, see #680546-135: Add a custom 'number of values' for 'multiple / multi-value' field. (e.g. nodereference, imagefield). Might be further generalized in #1207060: Interface pattern for custom option as alternative to predefined choices, so not our business.

#6 through #9: @TODO

[edit:] changed status because #1818560: Convert taxonomy entities to the new Entity Field API is in now.

Sorry for cross-posting, but:
Now that we have a form display tab, the handles for moving fields around were removed, so new fields can't be added in position anyway. This means there is no more reason to stick with the current layout with its space restrictions.

This isn't really a field system issue; it's a taxonomy one with many related field-system issues. :)

@Pancho, thanks for the updated summary.

Part of #6, #7 and #8 were fixed in #1953444: Make 'target bundles' required on the Entity reference instance settings form, so that only leaves us with the first part of #6 and #9.

Also, I've no idea why this is still assigned to me..

Updated issue summary.

No idea what to review here

I'd totally still love this to get in. But we definitely need to resolve the UX issues. And it's been so long now that I don't remember what they were. Is it possible to get this patch re-rolled on top of HEAD and we could discuss it @ DrupalCon?

I also support getting this in, if possible, but the remaining UX issues are definitely important to solve. @webchick, the UX issues were posted by xjm and are listed above: https://drupal.org/node/1847596#comment-7211430. As @amateescu says, the first part of xjm's #6 and all of #9 are what remain to be resolved. I hope this helps a little.

no longer postponed on #1818560

Apart of having an upgrade path, Entity-reference should also have the capability to autocreate entities. We can probably take advantage of the EntityWrapper for doing this not only for taxonomy terms.

Without that, deprecating the taxonomy reference would be a functional regression. The autocomplete tagging variant of a taxonomy reference allows for creating taxonomy terms if none currently exist in the vocabulary.

Another week, another patch from scratch :)

I re-read the entire issue and it seems that there are only two major UX regressions left:
1) losing the "term reference" option from the initial dropdown (as @David_Rothstein mentions in #145)
2) the fact that selecting the target entity type is not part of the initial screen (making it harder to know that one can select taxonomy terms as a reference type)

And I also have two proposals for fixing them:
1) when #2349991: Provide a trait for categorizing plugin managers and use it for conditions and actions gets in, implement it for field type plugins too and (borrowing the wording from David's suggestion) introduce option groups in the field type selector, something like this:

Numbers
  Number (decimal)
  Number (float)
...
References (to content, terms, files, etc.)
  Entity Reference
  File
  Image
...
Text
  Text (formatted)
  Text (plain)
  ...

2) finish #1963340: Change field UI so that adding a field is a separate task and, either in the same issue or a followup, bring the field settings form in the new "field add" form via AJAX, thus allowing users to easily see and select the target entity type in the first step

@Mixologic, entity reference has the "autocreate" feature already, it just needs a bit of love in #2370703: ER's "autocreate" feature is mostly broken (and untested).

The patch attached will fail until the autocreate issue is fixed but setting to NR anyway to get some feedback on the two proposals above.

This patch could potentially be both high-impact and high-disruption (https://www.drupal.org/contribute/core/beta-changes), but potentially more disruptive than impactful, so before investing too much time into it, I suggest getting a committer to comment on whether it's still in scope. Also, it will be much more disruptive to do after supporting a beta->beta upgrade path, so tagging accordingly.

We need "Needs issue summary update" addressed if we "Needs committer feedback."

Updated the issue summary and also posting a patch combined with #2370703: ER's "autocreate" feature is mostly broken (and untested) to prove that everything works.

Side note :

I was a bit worried by the fairly long text for the 'references' optgroup, making the select eat up more screen real estate for the "Field Type" column in the Field Overview table.
But with #1963340: Change field UI so that adding a field is a separate task, that select would be in a separate page, not in the Field Overview table, so that's fine.

The issue summary changes from #178 look good, but given the nature of this issue, I think the summary also needs a "beta phase evaluation" section similar to the one in #1963340: Change field UI so that adding a field is a separate task. I don't think I understand all of the ramifications of this issue enough to write that, so in the meantime, here's some initial thoughts:

Reasons for doing this before 8.0.0's release:

• With HEAD in its current state, you can either make a reference to terms using the taxonomy_term_reference field type or using the entity_reference field type. There's no meaningful difference in terms of content modeling between the two: either choice implements the exact same concept. But depending on which you choose, you'll get different formatters and widgets (and any other code that acts on fields in a type-dependent way) to choose from.
• If we ship D8 like that, then contrib will be writing formatters and widgets for each field type separately, resulting in an ever increasing gulf between the two options. Throughout the D8 lifetime, developers and site builders will be debating which option is "better". Once a site builder chooses which of the two field types to use for a particular field, she'll be stuck with that choice and the ecosystem of formatters/widgets available for it, because changing the type of a field that already has data is impossible to do without a custom migration process.
• As #2370703: ER's "autocreate" feature is mostly broken (and untested) shows, there would be benefits to eating our own dog food. Since core has uses of taxonomy references, using an ER field for those cases would help keep those ER features working well. Automated tests are great, but real usage is sometimes better.

Reasons for postponing this (probably to D9, unless someone can suggest a way to do this in a BC-compatible way while still achieving any of the significant benefits):

• Beta 1 was supposed to be a the milestone at which we have a stable data model. While some minor schema changes have been committed since then, I don't think any data model change as large as this one has been, especially for a non-critical issue.
• Contrib might already be porting/writing formatters, widgets, etc. for the taxonomy reference field type, and this would be an API break for them.
• There are existing sites already launched on D8 and others in development. While we've been clear that we're not supporting an upgrade path yet, this would present a harder change for those sites to incorporate than other things they've had to do. That could potentially be mitigated by requiring this issue to write its own hook_update_N() implementation, even if it is otherwise ready before we officially support an upgrade path.

Any thoughts/disagreements/additions to above?

I really like the list in #184. I noticed that the patch removes a lot of code. I would argue that the change makes D8 more maintainable, not to mention more logical and cleaner.

#180 seems like a fair assessment of the situation.

I would favor moving forward, myself. An API change now would affect a relatively small number of people. (There's a tiny number of Drupal 8 sites in the wild, relatively few D8 modules and I doubt many of them are messing with taxonomy terms, etc.) However, the decision we make here is one that site builders will be living with for 4-6 years. Let's have a little pain for a few people now to save a lot of ongoing pain for a lot of people later.

Thanks @effulgentsia, those are all excellent points! I agree that we need a "beta phase evaluation" section but I'm not sure how much I can help with it since I'm obviously biased :)

I'd like to reply to a couple of those "con" items:

Contrib might already be porting/writing formatters, widgets, etc. for the taxonomy reference field type, and this would be an API break for them.

While this is true, we also have to keep in mind that *a lot* of work was done in the unfication of the underlying layers for all reference-like field types (e.g. they all use the 'target_id' property instead of wild west that was before; they share a lot of code by inheriting from a single EntityReferenceItem base class, etc.). So, the actual API changes here are pretty minor:

• instead of binding a widget/formatter to the 'taxonomy_term_reference' field type, they would need to use the new WidgetInterface/FormatterInterface::isApplicable() API and check for the 'entity_reference' field type with a 'target_type' => 'taxonomy_term' field storage setting
• update any code that relies on the vocabulary selection being a field storage setting and look for it in the ['handler_settings']['target_bundles'] field setting

Or they can just assign an issue to me and I'll take care of it :)

There are existing sites already launched on D8 and others in development. While we've been clear that we're not supporting an upgrade path yet, this would present a harder change for those sites to incorporate than other things they've had to do. That could potentially be mitigated by requiring this issue to write its own hook_update_N() implementation, even if it is otherwise ready before we officially support an upgrade path.

Previous versions of this patch already have that hook_update_N() implementation so it would be (somewhat) trivial to bring it back and adapt it to the current core APIs.

However, the decision we make here is one that site builders will be living with for 4-6 years. Let's have a little pain for a few people now to save a lot of ongoing pain for a lot of people later.

+1

As a former maintainer of a Taxonomy Term reference widget: anything that brings more consistency and reduces fragmentation is extremely welcome. I'd have given a lot for even a tiny bit less fragility/fragmentation.

I think the "data model change" (the first con in #180) which then affects existing D8 sites (the third con in #180) are one and the same, and is really the only con we should take into consideration. How difficult would it be to provide such an upgrade path?

Err.. see the last paragraph from the comment above? :)

Oops, sorry! In that case: effulgentsia et al., assuming hook_update_N() is present, does that then mean your concerns are fully addressed?

Here's the upgrade path if we decide to include it.

+++ b/core/modules/taxonomy/taxonomy.install
@@ -0,0 +1,85 @@
+  if (!$field_storage_configs = \Drupal::entityManager()->getStorage('field_storage_config')->loadByProperties(array('type' => 'taxonomy_term_reference'))) {
...
+    if (!$fields = \Drupal::entityManager()->getStorage('field_config')->loadByProperties(array('field_name' => $field_name))) {

We can use FieldStorageConfig::loadByName() and FieldConfig::loadByName() here.

We also have to update forum_install().

We can use FieldStorageConfig::loadByName() and FieldConfig::loadByName() here.

Nope, loadByName() is a single-load operation while loadByProperties() is multiple-load.

We also have to update forum_install().

That's already handled in the patch from #178?

I went ahead and opened #2373491: Categorize field type plugins since it makes sense to group field types in the UI regardless of the outcome of this issue. Right now it (mostly) implements the groups shown in the image from the issue summary.

Thanks for explaining that and sorry for the useless review :$ @jibran--.
I have one question if we are going to add hook_update_N() do we also have to add the tests for the hook?

Also tagging VDC for the changes in views plugin files.

I doubt that we'll need automated tests for it, maybe just some minimal manual testing that anyone with a trashable D8 site can do.

In the meantime, the blocking entity reference issue landed so here's a reroll (not including the upgrade path for now).

I went through all the patch. It looks good to me just some minor points.

1. +++ b/core/modules/node/src/Tests/PagePreviewTest.php
   @@ -145,13 +134,13 @@ function testPagePreview() {
   +    $this->assertFieldByName($term_key, $edit[$term_key] . ' (1)', 'Term field displayed.');
   ...
   +    $this->assertFieldByName($term_key, $edit[$term_key] . ' (1)', 'Term field displayed.');
   
   @@ -163,7 +152,7 @@ function testPagePreview() {
   +    $this->assertFieldByName($term_key, $edit[$term_key] . ' (1)', 'Term field displayed.');
   

   Instead of (1) can we use term->id() here.

2. +++ b/core/modules/taxonomy/src/Plugin/views/argument_default/Tid.php
   @@ -136,7 +136,7 @@ public function getArgument() {
   +          if (is_subclass_of($field->getItemDefinition()->getClass(), '\Drupal\Core\Field\Plugin\Field\FieldType\EntityReferenceItem') && $field->getSetting('target_type') == 'taxonomy_term') {
   
   +++ b/core/modules/taxonomy/taxonomy.module
   @@ -672,7 +638,7 @@ function taxonomy_build_node_index($node) {
   +      if (is_subclass_of($field->getItemDefinition()->getClass(), '\Drupal\Core\Field\Plugin\Field\FieldType\EntityReferenceItem') && $field->getSetting('target_type') == 'taxonomy_term') {
   

   Why can't we use ($field->getType() == 'entity_reference' && $field->getSettings('target_type') == 'taxonomy_term')?

Wow! So, the upgrade path in #187 worked surprisingly well:

- I first installed HEAD, Standard profile, and created an article node with two tags.
- I then applied just the patch in #187 and ran update.php.
- I then applied #192, and viewing the article, with the Tags field converted, worked.

I was surprised by this, because I expected some schema change to be needed to the {node__field_tags} table, and didn't expect that to happen smoothly if there was data already in there. But, turns out, no schema change was needed, since TaxonomyTermReferenceItem::schema() and EntityReferenceItem::schema() define nearly the same schema. They differ on NOT NULL constraint, and the update process didn't update that accordingly, which the entity storage/schema system should have performed automatically, but that's likely a separate bug we need to fix anyway. TaxonomyTermReferenceItem::schema() also defines a foreign key to {taxonomy_term_data}, and I don't know if we want/need to preserve that when using an EntityReference to taxonomy terms.

Note that the order above is odd. It should be possible to combine the update function with the rest of the patch and have it work. But it doesn't, because update.php fatals due to field_system_info_alter() kicking off a entity_load_multiple_by_properties() which fatals when loading the definition of the not-yet-updated taxonomy_reference field whose code is already removed by the patch. But, I think we can find a way to fix that.

Given the ::schema() compatibility mentioned above, I feel that this is an acceptable data model change to introduce after beta, especially if we provide a working upgrade path for the configuration change, per #187, and get it to work when included with the rest of the patch.

Not my call though, so leaving the "Needs committer feedback" tag to get an official decision.

I'm supportive of removing taxonomy term reference field in favor of entity references. I believe the benefits outweigh the disruption, assuming this patch does not introduce UX regressions. I also agree that this issue is major (not critical). The deadline to get it done will be supporting a beta-to-beta upgrade path.

@effulgentsia, thank you so much for helping out with this issue! And @Dries for approving it :)

Regarding the upgrade problem described in #194, I think we can implement a nice workaround by keeping a dummy implementation of TaxonomyTermReferenceItem with no_ui => TRUE and which throws an exception in the constructor. This way, we're sure that nobody can use it anymore while still allowing existing D8 sites to run the update function.

Rerolled patch attached which includes the upgrade path from #187 and the dummy class mentioned above.

In the meantime, I've been hard at work on the UX dependencies listed in the issue summary (#1963340: Change field UI so that adding a field is a separate task is getting pretty close) but I think we need a new round of testing here and, more important, a validation of the current plan for fixing the UX problems introduced by this patch.

I also identified another regression that hasn't been mentioned in this thread so far: the fact that entity reference uses a entity label (entity_id) syntax in its autocomplete widgets, which will be very confusing for anyone familiar with a tagging system but not so familiar with the underlying structure in Drupal (e.g. "what's this weird number that appears after every tag?! But not when I enter a new tag. WTF?" :D).

But fear not, we have a solution for that as well: since the current autocomplete implementation doesn't allow us to improve that aspect of entity reference, we need to switch (again) to a better one, notably #2346973-37: Improve usability, accessibility, and scalability of long select lists.

And one last thing; even though the list of issues that are "required" in order to solve the UX regression introduced by this patch, please note that each of them are actually quite good usability improvements on their own, that's why I've been working on them even before this patch got the post-beta approval. It just so happens that if we look at them combined, they also fix most of the problems here :)

Adding the issue links from #196 as related issues.

Red patches are depressing, especially when the failures are from a different system that, apparently, is not fully working.

#1963340: Change field UI so that adding a field is a separate task is RTBC so here's a patch rerolled on top of that one. Setting to NW until we can send it to the testbot.