Add Entity reference field

Yes, ideally perform the entire port in a new 8.x-x.x branch in the project's existing contrib repository. This means you may grant commit access to further core contributors, you have an existing bug tracker, related issues, etc.pp. There's even a chance for backporting fixes to the D7 branch. It also means that there would be ready code to release in case this issue won't make it.

Once complete, you can use git-subtree to merge the entire repo+branch into the Drupal core tree and create a patch out of that. git-subtree also allows you to commit further adjustments to the subtree and push them, but also to pull in changes from upstream; i.e., it works both ways. We had great success with this approach for #501434: Move Link/URL field type into core already.

Note that "field types as plugins" is definitely on the roadmap (#1785256: Widgets as Plugins is in, #1785748: Field formatters as plugins is near-RTBC, field types are next - not before november, though). I'm not sure how the "behavior" plugin would be different.

Also, when field types are plugins (which they aren't for now, and a D8 port for core should most probably not stay postponed on that), "X reference" field types very much smell like plugin derivatives to me.

I know we're not supposed to do this, but it needs to be said: +1!!!

I have commit access and I'm planning to do just that.
Ping me on IRC if you want to help (and I'll try to create a set of issues in the next few days so that we can segment the work).

@Amitaibu
Wouldn't want it any other way :D

@bojanz - just pushed #1043198: Change notice: Convert view modes to ConfigEntity forward, something that entity reference field can benefit from as well.

Please, take care of proper separation of instance and field level settings as discussed in #1340098: Move target bundle to the field instance settings

@yched: the Behavior system of Entity Reference is about horizontal extensibility of fields. It allow you to extend a Field using one or more additional behaviors, in a very clean way. It's complementary, but actually very independent of the Field API as plugins.

We can have a great usage of ER for shortcut module to reference a menu-link entities
Related issues
#916388: Convert menu links into entities
#1811640: Convert shortcuts into configuration
#1802750: [Meta] Convert configurable data to ConfigEntity system

• I have ported the admin UI test to D8. The Selection access test doesn't work on my local, hopefully testbot can help get a clue
• Following Link and Views project, I've created a 8.x-1.x branch in ER, to make the code more visible, and allow opening issues in ER for 8.x

Still work in progress, but worth uploading a first patch to get some initial reviews.

> to add field values "on the fly"

Yes, free tagging is planned in #1472596: Free tagging entity reference to a vocabulary does not create the term, only allows existing terms

The last submitted patch, 1801304-er-core-22.patch, failed testing.

Sorry, I think I rolled a wrong version in #22, retrying.

Widgets and formatters converted to plugins.

Thanks yched! I've addressed issues from #34

I have moved the getInstance() logic into entityreference_get_selection_handler() but it is still very hardcoded. I Don't know exactly what's the best way to solve this. Also all the current sub-selection handlers (e.g. SelectionEntityTypeFile) have @todo code.
I think bojanz has some plans about it -- that involves entity_access()

This doesn't work. As far as I can see, there is no longer hook_field_widget_info_alter() in core.

The test fails in Drupal\file\Tests\FileManagedFileElementTest which doesn't seem to be related to this patch.

This test also passes on my local.

When we do something similar to this in Views, we use a try/catch instead, with no need for getDefinition(). Not sure which is better? Also, why the call_user_func?

This just looks weird.

Fixed the tests and the first comment from #42. The second one needs more thought.

Oh, the patch in #34 wasn't my most updated one -- don't know what I did wrong -- where I removed the getInstance() :(

@amateescu,
Any chance we can work on a sandbox for easier re-rolls?

Ok, from now on, no uploading of wrong patches from my side! ;)

• Patch moves getInstance login into entityreference_get_selection_handler()
• Rename widget/ formatter base classes

Patch removes PluginSettingsBase as previously commented by yched in #34

Patch moves getInstance login into entityreference_get_selection_handler()

The code in #50 for entityreference_get_selection_handler() is still wrong, not sure what's the best approach.

* If the handler == base we need to check if there's a entity-type specifc class, and call it; otherwise call the base class; otherwise BrokenHanler.
* If the handler == og we need to follow similar logic.

So we probably need some naming convention, and we'll have to use class_exists() for checking the entity-type specific handler.

Patch moves entity-type specific selection under the handler name (e.g. Drupal\entityreference\Plugin\Type\Selection\base\user\User)

Noticed two things in the patch...

Comment looks wrong here, probably copied from somewhere else?

Are you sure the unset() is necessary? I think content is initialized an empty array by default, there's even a test in core for this.

re plugin instanciation : #57 looks a bit better :-)
Now what's missing is;
- the logic within entityreference_get_selection_handler() should go into into SelectionPluginManager::getInstance().
- when its logic is done, SelectionPluginManager::getInstance() should then call $this->factory->createInstance() to create the actual object. SelectionPluginManager::__construct() currently misses declaring a factory for the plugin type, ReflectionFactory could do the job.
- entityreference_get_selection_handler() should then simply do:
$plugin = drupal_container()->get('plugin.manager.entityreference.selection')->getInstance($options);
(not sure then whether entityreference_get_selection_handler() is worth keeping as a separate function, your call)

I finally made sense of those lines :

So this means the actual Selection class that is used is strictly derived from the entity class - one selection class per entity class, or SelectionBase as default.

Then I'd say this is actually not a case for plugins. The classes are not interchangeable, but directly and imperatively derived from the entity class - and actually, as @Damz pointed, the patch only has one single plugin class, extended by classes that are not plugin themselves.

What we're talking about is more a 'reference_controller', whose class name should be set as part of entity_info(), like storage controller, form controller or view controller. No need to leverage the plugin API here, it only adds needless overhead.
It also means that the actual classes can be provided by each entity module rather than entity_reference.

But if OG wants to provide its own selection handler? Same entity type -- different selection logic.

Some fixes to make EclipseGc's code work (EDIT: work as in not WSOD -- didn't change selection_handler() yet)

I'm assuming that at one point ER should stop storing ids and start storing uuids.

Not sure if that should be a part of the initial patch or not.

A definite followup would be to make it support referencing a specific revision of an entity as well.

Re-rolled as per #1821060: Return values from getReferencableEntities() keyed by bundle
Taxonomy terms now appear with hierarchy, and if there are multiple bundles they appear as optgroup

Here's the update that I promised :) It doesn't include the changes from #75 because I think we should have that patch commited to the 7.x version before rolling it here.

I started a new branch in yched's Field API sandbox so it's easier to track progress: http://drupalcode.org/sandbox/yched/1736366.git/shortlog/refs/heads/enti...

I've committed #1821060: Return values from getReferencableEntities() keyed by bundle to 7.x, so added it to 8.x as-well.

I don't know why simpltest are failing, there are running locally ok.

I've started porting the Views selection handler in yched's sandbox under entityreference-1801304-views. Don't hesitate to land a hand ;)

Let's see if this one fixes the tests.

follow up on 7.x -- getReferencableEntities() should return Bundle's machine-name, and those should become a label in hook_options_list()

tagging; something went wrong with the previous comment and tags haven't been added.

Let's save reviewers some time; I did another run-down on documentation and some other minor code cleanup.

About #79: we provide a weight setting in order to allow other plugins to take over our core entity handling if they need to :)

+1 for entity_reference.
entityreference always bugged me:/

> everything needs to be updated for EFQ v2.

I'll give it a shot.

Thanks for the awesome work, folks ! Will try to do another review when I get back to a coding environment in a couple days.

One design topic I'd like to see discussed however is :
"One single field type with target entity type being a property" (what entityreference D7 and the current patch do)
vs
"One different field type per target entity type" (what userref / noderef / references did in D6/D7)

Could we get a summary of the reasoning that led to this design in entityreference D7 ? Was it a deliberate choice or did it derive from technical limitations ?

I personally see arguments in favor of separate field types:

- UI wise, as a site admin, I'd rather do :
1) in 'add new field', see 'User reference', 'Node reference', ... 'X reference' in the list and pick the one i want,
rather than
1) in 'add new field', pick an abstract, catch-all 'Reference' type, then
2) in the following screen, select the target entity type as a nested setting that I won't be able to change later on anyway.

- Also, on the list of existing fields presented on the 'Manage fields' overview, the current approach means all you see is that there are 'reference' fields, with no notion the target entity type, you need to open the field settings to know what the field actually references.
This is not ideal to make sense of the content architecture, esp. if we want at some point to have taxo fields, file/image fields replaced by reference fields.

- Similarly, code wise, when building logic on the reference tree (e.g do something for all file fields), it means you can't know the target entity type without loading the full field definition, which is costly CPU and memory wise.
Since #1040790: _field_info_collate_fields() memory usage, we're trying very much to avoid "load all field definitions", "load all instance definitions" calls, because those just cannot be made to scale. I might even even file an issue to remove field_info_fields() / field_info_instances(NULL, NULL) in D8.
The new field_info_field_map() gets you a lightweight map of fields, containing only the field type. But for reference fields in the current approach, that would not be enough, the field type tells you nothing.

- Widgets and formatters are 'per field type'. The current approach means you can't have a widget that's valid for 'file references' only.
Granted, the "separate field types" approach would mean a widget needs a way to declare it applies for all '*_reference' field types, but that one seems more solvable than the other way around.

#1828408: Re-add addTag() and AddMetaData() to EFQ

#102

Having a single field with property has some perks.
- you can change the field property until it has data: this means more flexibility
- it doesn't "polute" the field type with as many entity types you have in the site (node, user, taxonomy, product, etc, can be as many as 10 in a standard instalation): this means more organization

please keep the original design of a single field type + property, like in entityreference D7.

yched, i have little experience in field implementation but i think all the points you talked about in #102 should actually be fixed in field api itself and not require entityreference to accommodate this seemingly field api "weakness"

How much different are the implementation approaches in real terms?

Can't we go with a single field type first, and rather easily turn that into a derivative-whatever later on?

(assuming field types will turn into plugins very soonish)

I'm definitely a little late to the game, but wanted to chime in with some thoughts on the "one field type versus many" question that's come up.

Many of our projects have somewhat complex content models with many relationships, and many require custom (or customized) widgets to smooth things for content editors. We've made equal use of Node/User Reference and EntityReference, and I think the difference is a bit of a wash. Most of the times we've needed to parse the field info to find the entity-type it points to have been restricted to low-traffic administrative pages where the slight hit wasn't noticable. And from a UX perspective, having special-use widgets that say, 'Sorry, I can only be used with Users' is no worse than the proliferation of near-duplicate field types.

In some ways it's like the file field problem: If someone creates a widget that only makes sense for audio files, or PDF files, does that imply the need for mimetype-specific file fields, or is it just a configuration gotcha to keep in mind? I'd prefer a universal field, with ongoing future work to give widgets more control of where they're made available, to a system that enforces entity-specific reference fields.

Better handle getting base-table in entityFieldQueryAlter()

re @eaton #113

Most of the times we've needed to parse the field info to find the entity-type it points to have been restricted to low-traffic administrative pages where the slight hit wasn't noticable

What I'm worried about is house-keeping stuff like our current file_get_file_references (runs every time a file field value gets removed - dunno how much this qualifies as low-traffic)

And from a UX perspective, having special-use widgets that say, 'Sorry, I can only be used with Users' is no worse than the proliferation of near-duplicate field types.

This probably applies for custom widgets developed for your site's specific use case, I've done this myself, widgets or formatters coded for a specific field on a specific node type.
But do we want to have this UX for the 98% case of adding taxonomy or images to your articles ? I do remember the negative feedback when adding taxonomy on a node moved from "create a vocab and hit the "node types" checkboxes on the create form" to "create a vocab and then go add a 'taxo ref' field on each of your node types"...

Also, I have trouble convincing myself that having your "Manage fields" list of fields on a node type only say "there are those three 'reference' fields", with no clue to what they reference other than the convention you might have applied on the field name, does not impact grasping the content model. But well, I lack personal battlefield experience with D7 entityreference, TBH.

Hmmmm.

I'm begrudgingly coming around to your perspective, yched -- there ARE an awful lot of special cases to be handled there, and I can see the file reference in particular being thorny if it's handled generically. I think I'll back slowly away from this and concede that, as long as there's a way for widgets and formatters to announce that they apply to any reference field, the problems with individual field types aren't as serious.

If there were a way for us to create a "generic entityreference", in which the id and entity type were both stored, it might make more sense to have a single field type. Having a field that links to a comment OR a user OR a node etc is a use case that would require something generic, but for the moment even the generic reference field doesn't support that.

"target_type" inside the field, so theoretically you could reference via a single field multilve entity-types, but this was deprecated

Funnily, I assumed that this most basic and most generic case would be covered by ER in core. Adding constraints is more functionality, not less, no? ;)

Any chance we could retain the target_type field schema column and just pollute it, but don't necessarily use it in core? I understand there might be other components that won't support multiple different types, but I do see quite some potential for contrib and custom sites/modules to build on top of the core ER field instead of building an entirely new, if we'd store the target type.

> Carry on, everyone. CARRY ON! ;-)

Yap :)

@amateescu,
Can you figure out why tests are not working on testbot?
Anyway, since I was in the area, I've added a test for sorting.

I do not see the new property API being implemented. See #1839070: Implement the new entity field API for the taxonomy reference field type and the exisiting EntityReferenceItem. Also, regarding multiple entity types referenced, EFQ is getting relationship support #1446600: EntityFieldQuery (pseudo-)join support with a syntax like ->condition('field_name.entity.foo') now if field_name can refer multiple entity types then what am I going to join in for foo?? If it's strictly one entity type then of course 'entity' can be resolved based on field_name settings (and if you implement the property API with an entity api constraint and an id source then EFQ will already do that) What you can do, sure you can, and relation will need to, anyways, is that you can implement what I call a relationship specifier for every entity type being referenced so that you can do ->condition('field_name.node.foo') and ->condition('field_name.users.foo') and so on.

Anyways, for now I strongly recommend not doing that and just keeping with a single entity type so that the uniform field_name.entity.foo syntax can be used.

Lets try that with the patch from #133 as multi patch.

EDIT: Oops, somehow I triggered a race condition that created the attachment twice.

Responding to the bat signal :)

Test are passing, thanks amateescu!

@nevergone, thanks. Indeed there was an error in the code and in the test :/

Attached the fixed code.

What a great effort to get this into core. It would be amazing to have this super-useful feature into core.
Here are some points, nothing big but it's too late for a longer review ;)
Feel free to contact me, so we can work out the hook_views_data views integration (probably a follow up). In general you could try to save some time by getting just the base in, until december.

Not sure, but if you have parameters at the end you currently don't need to specify them explicitly.

What means invalid, couldn't you say that the entities don't exist?

You could simplify this to !empty($element['#ajax'])

Just in general i'm wondering whether this could use efqv2 directly and deal with that so you could use other entity storage backends.

'NULL' as strings seems to be a bit odd.

Better use "@todo Improve when ..." here.

What about just use drupal_explode_tags first and then count() and the items. See taxonomy_autocomplete() for a similar behavior. This also saves us one menu item.

Nitpick: You should probably also document these fields.

What about use return parent::createInstance('broken') instead? Ah i see, the broken class is not an actual plugin.

Nice idea!

What about appending the second part of the other error message here as well?

Just linking to #1552396: Convert vocabularies into configuration as this has to be reworked based on the other issue.

Just throwing out some idea: Couldn't you use 'type' => 'entity_reference' for that, similar to Feed.php does it.

This three properties should be defined on the class as properties.

If it makes sense to hide the config of cache/title from the UI, you could use optionsSummary and just not return the appropriate items.

You could use the even_empty method on the style plugin.

You can assume that the id_field is the $this->view->storage->get('base_field') now (see Sql::add_field()) Btw. i think the properties on storage are protected now.

You could also document them via "Overrides \Drupal\views\Plugin\style\StylePluginBase::$usesRowPlugin".

It seems to be for me that you want to not allow grouping for entity reference style plugins, so $this->usesGrouping = FALSE; and use $this->displayHandler->getFieldLabels(TRUE); instead

What about output the array in a human readable way via print_r or var_export with a pre around it (similar to Feed.php). In general this overriding can be done in preview()

It seems to be that we have to improve views_get_views_as_options to be more generic.

It's enough to just check the access as the view access method takes care about invalid displays.

I'm wondering whether we want to throw exceptions instead of watchdog messages in d8 for semi bad things like that.

This piece of code does not work because the result array keys are just ascending array indexes. (0, 1, 2 ...)
What you probably want is to just get the entity from $view->result[$row_index]->_entity as it's loaded there already.

You could use $this->view->pager->get_total_items() which is a bit safer.

What about let Sql::load_entities() return all the entities?

Not sure, but if you have parameters at the end you currently don't need to specify them explicitly.

Support for this might be removed and is not yet supported by the new router system. And even if not, it's better to explicitly define them if they're required.

@dawehner, I haven't addressed:

'NULL' as strings seems to be a bit odd

I don't remember now, but there was a reason for it, I'll need to try and remember :)

What about just use drupal_explode_tags first and then count() and the items. See taxonomy_autocomplete() for a similar behavior. This also saves us one menu item.

I actually think it's a bit more readable as it is. It allows us to understand the matches can be for a tags style autocomplete VS a single autocomplete per delta. Maybe better docs would be ok for you?

Just throwing out some idea: Couldn't you use 'type' => 'entity_reference' for that, similar to Feed.php does it.

This doesn't seem to be in Feed.php, and we need it for views_get_applicable_views() - did you mean something else?

What about let Sql::load_entities() return all the entities?

Sorry, didn't understand what you mean?

Added test for Views' entity-reference selection handler.

So, when does this patch, like hmm, you know... gets committed? ;)

The failing tests are all for Views, so maybe wait a day for someone to fix it on the Views queue?

(in other words, argh)

@tim.plunkett,
Oops, indeed they fail also locally. Investigating :/

Green!

Here's a nitpick review. Looks quite nice, noticed quite a few @todo's and comments about limitations that ER has to work around. Can we get a few follow-up issues and link them in the issue summary? We actually have a chance to fix the source and clean up the code here now :)

Same for follow-up tasks to attempt to replace something like the taxonomy term reference field with this.

I also think this needs a sign-off from the field system maintainers, so setting back to needs review.

Should be intended correctly, even when commented out like this.

That sounds... wrong, should be referenceS I think.

Should be Contains... now

I think we can simplify the class name to RecursiveRenderingException. No need to repeat the module name.

We have a mess with this already, but I think it should be integer, not int.

Should have some basic @param's.

I think someone already suggested that these should be moved to the corresponding modules, is there a reason for not doing it?

This looks weird, why not just use where directly?

Do we still need the standard check? No other test that I know of does something like this and we default to testing now.

Should have a description.

Missing descriptions.

Also, SelectionGeneric should probably be SelectionBase.

Great, thanks for the review, I'll work on addressing the comments and opening follow-up issues.

Addressed the above comments except the following which I have no idea:

I was wondering if we shouldn't call this just Reference.

I think Entity-Reference is more descriptive. I know I can reference a generic entity, and not let say a config.

and although I haven't seen proper responses to the "forbids widgets or formatters specific to an entity type" issue

Well, that was not done out of no respect to your comments -- It's that 1 DEC deadline that keeps reminding me we need to stick our foot in door :) . I've opened a follow up issue for this #1847590: Fix UX of entity reference field type selection, as it probably deserves it's own issue.

EDIT: The interdiff is big due to changing the file location, and replacing the word "generic" with "base"

I don't agree with the renaming of "generic" to "base" because this is a selection plugin implementation that's not really supposed to be used as a base from which other classes/implementations extend upon. Our own views selection plugin is an example, it doesn't need to extend the generic one. But nevermind, I'll let it rest for now becuase we really need to get this in :)

Fixed a file name that was forgotten and added an entry in MAINTAINERS.txt.

#172: entityreference-1801304-172.patch queued for re-testing.

@amateescu: the Base suffix doesn't mean all classes implementing the interface *have* to extend that base, it's just a hint.

Re-rolled according to #176

> I'd also be very keen to see at least one conversion of an existing reference field, whether taxonomy terms or files

I agree. The first task is #1847596: Deprecate Taxonomy term reference field in favor of Entity-reference, which is blocked by #1847924: Allow Entity-Reference field to autocreate items

Re-adding tag.

I am fine with a followup but can we make sure please that the new entity API is implemented some time?

I can't assign it to Dries, but according to catch, Dries need to give his sign-off for this.

I'm on board with adding this feature. It's very useful and powerful, and could be a building block for more. But ...

I tested the patch and it seems to work well, except for some serious usability issues. Conceptually it makes sense to drop "taxonomy term references" in favor of this, but it would require this to be pretty useable for non-developers. For example, I don't think some of the sorting options are useable enough. It wasn't immediately obvious what "Filter by an entity reference view" meant. Same for "a property of the base table of the entity" or "a field attached to this entity". This language is very developer-y and requires a deep understanding of Drupal. We can't really drop taxonomy term references in favor of this, until we fix some of the usability issues. At a minimum, I'd like to see a usability review and some screenshots before this gets committed.

As for reviewing the code and its performance; I won't be able to review a 200k patch right now though. I'd be happy to have catch sign off on the code review if he has time.

I'm not clear if Dries wants usability testing before this patch goes in, or before reworking taxonomy reference to use this. Assigning back for clarity.

I can translate. The usability of this patch right now is pretty bad, unfortunately. Adding $entity reference fields is a site builder task, but the patch in its current state it involves a deep, *deep* understanding of both developer-facing terminology as well as Level 93 Views knowledge in order to use it for all but the trivial cases. These concerns severely compound if we're planning to get rid of targeted reference fields such as Taxonomy and File in favour of this field. Adding this kind of stuff is "table steaks" site builder stuff, which means people who are not programmers and have never seen a database before need to be able to use it.

So what Dries is asking for is basically a general usability review + some fixes to make it less painful.

Here's a walkthrough of the patch as it currently stands to help that along:

USE CASE: As a site builder, I want to add an "Author credit" field to my Article type, which can reference a user on my site.

1. Apply the patch and enable the Entity Reference module.
2. Go to Structure >> Content Types >> Article >> Managed Fields (admin/structure/types/manage/article/fields).
3. Add new field "Author credit" , choose "Select list" (or whatever) as the widget. Click Save.
   

   NOTE: for whatever reason, tabledrag is broken on this page for me but I've been informed this is something jacked with my local install somehow, so disregard (unless someone else can reproduce this).

4. Next, you're directed to select the type of entity you'd like to reference on the Field Settings form (field label says "Target type", description is "The entity type that can be referenced through this field."):

   

   FEEDBACK:
   a) I find it weird that both "Category" and "Taxonomy term" are listed here. What's the difference?
   b) I also find it weird that things I'd consider "config entities" like Breakpoints and Views are showing up here, along with "content entities" such as Content and Users. This seems potentially very powerful, but Is that intended? Seems like eventually (hopefully) Block, Menu, and various other things are also showing here as well, leading to an overwhelming list pretty quickly.

   What if we used an <optgroup> to separate Content from Configuration entities? You have to be pretty far down the "advanced use case" road to have a reason to attach a Breakpoint reference, IMO, so that just becomes visual clutter standing in the way of doing what you actually want to do.

5. Choose "User" and click Save.
6. On the next screen, you choose your "Entity selection" method. Here's where things get whacky.

   

   Under "Mode" there are two options: "Simple Select" and "Views: Filter by an entity reference view." Let's cover Simple Select first, since the use case doesn't prescribe any particular fanciness that would require a view.

   "Sort by" defaults to "Don't sort" which is pretty much what you never want. So you click that to expand it and you get... a whole pile of developer-ese gobbeldygook:

   

   FEEDBACK:
   Um. WAT? :P Sorry, but this is totally whack. :\ It's not reasonable for a site builder to understand what any of these words mean and know what choice to make here. :\

   Can we instead just expose all of the possible things you can sort by, including "none"?

7. "A property of the base table of this entity" will give you options like "uid", "pass", "created", "langcode", etc.

   

   "A field attached to this entity" will only give you "PIcture" since that's the only field on users.

   

   FEEDBACK:
   Hm. Well I guess not, since that'd mean a list with both "uuid" and "Picture (column fid)" in the same list, which would be pretty awkward. Is there any general API that we could hook into around Views to get a standard "human readable" version of this info instead? Or is this only a temporary state, which will eventually resolve itself as as we convert things to the Entity Field/Property API..?

8. Pick "property of the base table of the entity" and the "name" property and click Save.
9. Yay, your field is made. Now go to Create content >> Article (node/add/article) and you'll see your lovely select list.

   

   FEEDBACK:
   It's weird that "Anonymous" is showing up here as well. While there's probably a use case for that, seems like it'd be better to give me a checkbox to enable/disable that in the field settings.

Ok. Got all that? Now let's try an only slightly more advanced use case:

USE CASE: As a site builder, I want to add an "Author credit" field to my Article type, which can reference a user in the 'editor' role on my site.

Stay tuned... spoiler alert: it's not pretty. :\

First very rough review based only on webchicks screens above in #195, the simple use case

2. A common problem with this kind of functionality in general is that you don't necessarily think of these things as fields so don't necessarily end up here to even find it. Does the module description in 'Extend' provide a pointer about this?

3. First hurdle is the 'Entity reference' label. Especially 'entity'. Good thing we got rid of 'node' :) Anyway, thingies are called entities and we're trying to reference them so lets go with this for now.

4. What webchick said. Alpha-sorting is not helping us here. Can we make this list shorter and/or group and sort it more intelligently? The label 'Target type' uses new words where it's probably better to keep using 'reference' and 'entity'. "Entity to reference"? "Reference this/these entity(ies)?

6a. Selection mode: Incomprehensible (again, only looking at the screenshots). I *think* defaulting to 'simple' and providing a checkbox or something to opt for 'Views: filter by…' would be better. I don't know what I'm choosing here, the system doesn't explain what the choice entails so lets find a way to either clarify that or UI-wise, postpone on having to choose between these at all.

6b. Sort by. What webchick said. How many options can we expect here? Can we pick a better default than the current one?

Simpler wording suggestions:

- Sort by a property of the base table of the entity > Sort by property of the referenced entity?
- A field attached to this entity > "This" confuses me. This entity I'm adding the reference to or the ones that are being referenced? I guess from the scope of things here it is the later. Suggestion: A field of the referenced entity
- Drop the redundant 'column' from the 'sort field' select list, the info will stay relevant to those who understand it, abstract to those who don't.

Overall, the workflow from 4 to 8 needs a closer look. I think we can sequence these steps in a better way that provides more guidance. Which goes beyond this first initial review. Massaging the actual wording and labelling will help a lot too.

Aaaand we're back. To refresh your memory:

USE CASE: As a site builder, I want to add an "Author credit" field to my Article type, which can reference a user in the 'editor' role on my site.

1. Repeat steps 1-5 above to add your field (or edit the one from above). You should be looking at this:

   

2. Because there's no way to filter the list of users by role in a "Simple select" type, we now need to jump face-first into Über Advanced Land, which means selecting "Views: Filter by an entityreference view." The first thing you'll be told is that you have no entityreference views, and you need to create one:

   

   FEEDBACK:
   One really positive thing I will say is that throughout, already knowing Views, I was actually to complete this task thanks to the very precise wording in these and other various field descriptions. So bravo for attention to detail and clear use of language here.

   Unfortunately, the vast majority of people wanting to build a website out there have not used Views, and if they have used Views they most certainly probably have not messed a lot with Displays, since the wizard takes care of that. :( My first question would be, "Can we just expose the most common things people are likely going to want to filter by (e.g. Users by role, Nodes by type, etc.) on this form right here, without requiring people to jump all the way to Views to do them? I can totally see going to Views for "I want my author credit field to reference users with a certain number of user points and who have created between 1 and 5 articles with the word 'Snoopy' in them" but not for something so basic.

3. Click "Create a view" here. But before doing that, I recommend writing down "Entity Reference display" on a napkin, or else having a really good memory, because the next screen doesn't say anything about it:

   

   FEEDBACK:
   Would it be possible to customize this wizard for the task at hand? For example, adding an auto-checked fieldset for Entity Reference display so that part is already done at the time I'm specifying filters and stuff like that?

   FOLLOW-UP: Those fieldsets should not say "Details" :P Not the fault of this patch.
   FOLLOW-UP: Not sure if "Save and exit" makes sense as the primary button here. Not the fault of this patch.
   FOLLOW-UP: It was surprising that "Role" isn't one of the exposed filters on the wizard for "User." I would've thought that was pretty common.

4. View name: editors, Show: Users, click "Continue & Edit."
5. Now on the "Advanced" view screen, you get to do the following elaborate dance...
6. Click "+ Add" at the top (FOLLOW-UP: The button should likely be "Add display"? Not part of this patch.) and add an "EntityReference" display:

   

7. All appears fine, but if you scroll down to the preview section, you will see:

   

   FEEDBACK:
   In a similar way that the Views wizard defaults to having User: Name field already populated, can the EntityReference display default to whatever the "primary field" is of the given view type?

   FOLLOW-UP: Had the error been at the top of the page, I could've reacted faster. Not the fault of this patch. (I don't think.)

8. Despite being scary, the message is descriptive, so click on "Settings" next to Entity Reference list

   

   FEEDBACK:
   Maybe just link right there from the error message directly?

9. Click it and get a modal, with a required field with one option and nothing checked. ;) Tee hee. Check it.

   

   FEEDBACK:
   (minor) This is not an autocomplete widget; it's a select list. So the description is incorrect.

   I don't really understand what "Search field" means. It seems to think I know more about how this is implemented under the hood than I currently do. Can you explain this, and we can try and come up with a better word?

10. Yay, now the preview shows stuff:
    

    FEEDBACK
    Ironically, this is showing it to me without anonymous, which is what I would expect. :D Go, Views!

11. Add a filter criteria for user role (I won't screenshot this, you've all done it before. But worth bearing in mind that it's like 3 more steps).
13. Also add a sort criteria since you most likely don't want the list sorted by newest first. Another 3 steps. Then you want to remove the one you don't want. Another 2 steps.

    FEEDBACK
    Related to the "wizard" question above, can we default this to "Name" instead of create date (desc)?

14. Cool, preview updated to show only editors, in alphabetical order:



15. Save the view. Hooray!!!!
16. Now, simply go to node/add/article and... hey wait. It's still looking like this. What gives? :(

    

    ...because you got distracted with 500 steps and forgot that you were actually configuring field settings. :P Oops. :P

17. Go back to admin/structure/types/manage/article/fields/field_author_credit, select the view, save. NOW you're done.

    FEEDBACK
    Is it possible for Entity Reference to just ship with some sensible default content entity views that would show up in step #2 that basically replicate what Simple select does? I still wouldn't have exactly what I wanted most likely, but it would probably eliminate a TON of these steps.

Whew! Ok. So now we need to figure out what of this is commit-blocker and what of this is follow-up. But we definitely can't commit this as-is, sadly. :(

Had a conversation on IRC with webchick, in short:

1. Give a checkbox for the user, to filter by role -- that's a typical use case, and we don't need Views for this
2. Provide an example Views configuration, for example "Content by user", so more advanced users can see the ER display style in use
3. Hide the config entity from the UI. We can still add them programmatically
4. In the sort, have properties + fields together, like View does. We can use field API to get the description

I assume that config entities do not work right now anyway? their id is a machine name, that's going to be hard to save in a target_id integer column :) Unless I'm missing something...?

And trying it out, you don't even get that far, it dies with a "LogicException: Querying configuration entities is not supported" after selecting e.g. Views.

So I would even say that hiding them is actually our only choice right now + an issue to figure out if and how we can support them. Maybe that needs #1803064: Horizontal extensibility of Fields: introduce the concept of behavior plugins so that we can build the correct field schema for each entity type? And @chx's query with grep issue for the qerying part ;)

FooClass instanceof ContentEntityInterface was supposed to be the way to determine the distinction, except for users being weird and extending Entity directly right now.

The last submitted patch, entityreference-1801304-206.patch, failed testing.

@amateescu,
Why not allow multiple selection on the user-roles?

Alright, my review + suggestions! I focused primarily on Field UI as @webchick covered Views UI quite well (maybe we should split fixing that off? seems quite large). I took about 45 minutes with amateescu earlier today to go over this, we walked through the difficult parts and had some discussion about possible solutions. This was with the latest patch(es) applied which resolves a number of the issues listed earlier.

Fundamentals

• It is not discover able that you will need "Entity reference" to be able to select Content, Users, Files, Terms etc. I'd much rather have "User reference, Content reference, File reference" as options and simply keep the config options under a Config reference with a dropdown. I think the discover ability you need such an abstract term to get a field with users, is a big issue. I'm sure there are some name space conflicts, but core should use the most sensible label.
• Why are we introducing the word "Entity" here, although I am sure it has useful qualities, as @yoroy already mentions its another abstraction label people won't get and in this case its not really necessary. We can just use the word "Reference" which makes it much less scarier to select

User/Content/File
There are a couple of issues with this UI, primary some of the functionality grouping is really necessary.

• Simple selection is not a meaningful label, given that this is simply the default method. Why don't we just call it "default" ?
• Lets remove all notions of "Don't sort, Don't filter" and replace them with - None -

I was quite puzzled to find Sort by and Sort field seperated, although I am sure this might scale better with hunderds of different categories. It is essentially just one list, where - None - by default, is the only nonfield. Given that there is no feature loss, and it reduces complexity significantly (e.g. I have no idea what "a field of the referenced entity" means, but do know what sort by "name" means). We should move this to just one select list, with -None - on top - this also brings it closer in line with the Views interaction which is one long list of sort by options with human readable names.

Furthermore onto labeling

• Let's call this section "Selection" or "Reference"
• Mode, does not give a lot of clue what this is about. Perhaps "Source" or "Method" with a little description
• Target bundles, sounds like a really abstract label when we know what the contents are. For Content, its 99% going to be "Content types" and for terms its 99% going to be "Vocabularies" lets default core with sensible labels where we are aware of the contents. This is also in line with Views.
• We should not use the pattern of "select none, to get all". That pattern has proven time and time again, to be hard to understand by users. Instead we should do something similar to Views, where selecting a "All *" toggles all, and when you remove a toggle from a specific vocab/content type the "All *" removes its toggle.
• We found that, the labels should be human readable names - amateescu seemed to already be working on that.

Just an idea how some of those suggestions might end up looking:

I moved it below the "Article settings" - I am totally handwaving Field UI here, but it is really sucky to have fieldsets inside fieldsets like this. I am also not very fond of all the indenting, if we merge the two fields at the very least there is less indenting going on.

There goes my initial review :) Hope this is enough to go from.

:D It seems just before feature freeze rush is over, hopefully this will see work before feb :) There is quite a lot.

Assigning to me so I can do a pass over the API-level stuff while the usability fixes are being worked on (this shouldn't stop anyone else doing the same).

I've merged #1801304: Add Entity reference field into ER's main branch (you can see the diffs in that issue).

Next is go back to #1847596: Deprecate Taxonomy term reference field in favor of Entity-reference. Once that issue is in, I really hope we can get ER to core, as this patch is becoming huge.

> Wait. How can #1847596 get in without this issue? I'm confused.

No, what I meant is the code from #1847596: Deprecate Taxonomy term reference field in favor of Entity-reference was merged into the main ER code that lives in yched/entityreference-1801304

> Are you ready for a new UX review?

I'd prefer to first finish #1847596: Deprecate Taxonomy term reference field in favor of Entity-reference

Vocabularies are now configuration entities, so we need to update the entityQueryAlter() method -- but not sure how?

> I still think this would be a killer feature to have in D8

I agree :)
Work is now done in #1847596: Deprecate Taxonomy term reference field in favor of Entity-reference. Help there is appreciated, cause it's going over and fixing lots of tests...

@catch,
Talked with Berdir, and I'd like to confirm. Is what holding this issue the deprecation of taxonomy term or just fixing the UI?
#1847596: Deprecate Taxonomy term reference field in favor of Entity-reference is getting closer, but it's a big patch, so maybe treat it as a follow-up?

Here's an updated patch and screenshot that addresses Bojhan's usability feedback from #211.

About the Fundamentals part:

1) this was discussed at length from #102 to #124 and from what I remember the agreement was to continue with the current implementation for now. If you feel strongly that having *a lot* of 'references' vs. only one option in the Field UI is better for new users, maybe we should open a follow-up to discuss this?

2) I think I agree that renaming to Reference makes sense, but this is quite a big change if we want to want to go the full way (renaming everything in code, not just a few UI labels) so maybe we could gather more feedback about this

The follow up amateescu proposed above exists and was part of the issue summary ... #1847590: Fix UX of entity reference field type selection

A whole lot less to review :) This is going in the right direction, a small review;

Perhaps we should relabel this and move the type to the top. I still think just calling this Reference will make it a lot more easier to grasp, and allows for all kinds of things to implement the idea of being a Referencable type.

We should seek ways to make these labels more sensible, on initial sight I always miss this fieldset because it doesn't stand out being about the reference option.

Seconding "Reference Type" and "Reference Method".

I'd lean towards "Create referenced entities if they don't already exist" as opposed to "Auto-create." More verbose, but.

Implemented the feedback from #233.2 and #235.

#233.1 is a limitation of Field UI, we cannot (easily) move the target type select box above the values :( Also left the 'Target type' text intact because @pounard is right in #236.

Moving back on catch's radar :)

The last submitted patch, entityreference-1801304-237.patch, failed testing.

@amateescu, I'll be working on fixing the tests.

Let's see if at least testbot likes my idea from #242 :)

The last submitted patch, entityreference-1801304-243.patch, failed testing.

Incremental progress, out of interest lets see how many more fails left.

The last submitted patch, entityreference-1801304-248.patch, failed testing.

The last submitted patch, entityreference-1801304-250.patch, failed testing.

OMG, it's green again! :)

> Shouldn't we answer this question or open a followup for it?

Yes, it is in the follow-up issues list #1891558: Improve TermSelection access control :)

We should make sure to follow this pattern instead of hardcoding ->value in foreach() when dealing with EntityNG. For example

<?php
foreach ($this->entityInfo['schema_fields_sql']['data_table'] as $name) {
  $key_name = key($entity->$name->offsetGet(0)->getProperties());
  $record->$name = $entity->$name->{$key_name};
}
?>

I don't agree with this, it's a true performance killer.. think of how many function calls it adds for each field of each queried entity. I'll try to come up with something else tonight :)

Bah, failures were due to #611294: Refine permissions for Field UI. Fixed in the attached patch.