D7's Entity reference implements a generic and plugable reference field, that can replace the taxonomy term and provide more functionality.
What's making ER powerful, is the concept of the plugin-types: "Selection" and "Behavior".
Selection plugin
Every field can have a single selection plugin, which will determine which entities are referencable. For example OG 7.x-2.x uses this set the group-audience field based on the group membership.
Behavior plugin
This is, as DamZ said, "field API implenented as CTools plugins". Currently the field API hooks aren't "real" hooks in the sense that just a single function is called. The behavior plugins allow implementing modules to attach their own unique "behavior" -- prepopulate a field value; Update the {taxonomy_index} table; etc'.
Possibly, the "behavior" concept should be extended to all fields not just ER.
Work on the behavior plugin is done in #1803064: Horizontal extensibility of Fields: introduce the concept of behavior plugins
Steps to test
#313 (originally found in #199 and #278)
Follow-up issues
#1847608: Improve CommentSelection access control
#1847588: Implement the new entity field API for the Entity-Reference field type
#1847590: Fix UX of entity reference field type selection
#1847594: Replace File and Image field types with Entity-reference
#1847596: Remove Taxonomy term reference field in favor of Entity reference
#1847600: Use proper entity 'view' access checks in entity reference
#1847602: Allow grouping in Entity-reference View's style plugin
#1847606: Improve FileSelection access control
#1847608: Improve CommentSelection access control
#1847924: Allow Entity-Reference field to autocreate items
#1891558: Improve TermSelection access control
#201 2. Provide an example Views configuration, for example "Content by user", so more advanced users can see the ER display style in use
#1904882: Entity reference field Reference method Default settings order should be Filter by then Sort by
#1904892: Add Entity reference fields like: User reference, Content reference, File reference
#1904896: when create view from entity reference field instructions to create one, default to have entity reference display
#1904900: Default auto pick the primary field for the type in view wizard to avoid red error: 'Display "EntityReference" needs a selected
#1904902: replace "autocomplete" in modal with more accurate word and replace "Search field" during entity reference view wizard process
#1906806: Provide a relationship for each entity reference field.
#1953568: Make the bundle settings for entity reference fields more visible.
Comment | File | Size | Author |
---|---|---|---|
#325 | entityreference-1801304-changelog.patch | 447 bytes | amateescu |
#320 | entityreference-1801304-320.patch | 213.64 KB | amateescu |
#320 | interdiff.txt | 3.13 KB | amateescu |
#319 | 1801304-319.patch | 213.5 KB | star-szr |
#319 | interdiff.txt | 26.59 KB | star-szr |
Comments
Comment #1
bojanz CreditAttribution: bojanz commentedAs discussed with DamZ, we'll want to do "Entityreference with selection plugins" in one patch, and the "Behavior plugins" in another patch, to minimize bike shedding.
Seeing how the selection plugins look will help people to realize how broken and incomplete entity access is, as well.
I guess the first step would be to create a 8.x-1.x branch of Entityreference and port the code, then divide it and post patches?
Comment #2
sunYes, 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.
Comment #3
amitaibuYes, I agree we should start with that, and behaviors can com later - ER specific, or for all fields.
Comment #4
yched CreditAttribution: yched commentedNote 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.
Comment #5
andypostEntity refs are mentioned in summary #1346214: [meta] Unified Entity Field API
Comment #6
DamienMcKennaI know we're not supposed to do this, but it needs to be said: +1!!!
Comment #7
cosmicdreams CreditAttribution: cosmicdreams commentedExciting! So is the best way to help this effort forward to do the following:
1. Goto http://drupal.org/project/issues/entityreference?categories=All
2. get commit access to new branch
3. start converting entityreference to use CMI and other D8 features?
Comment #8
bojanz CreditAttribution: bojanz commentedI 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).
Comment #9
amitaibu@bojanz, happy you start working on it. I've committed a completely messy, not working untested code to
1801304-amitaibu
in ER I've done yesterday. Maybe you'll be able to use it.Did I mention it's messy and not working? :)
Comment #10
bojanz CreditAttribution: bojanz commented@Amitaibu
Wouldn't want it any other way :D
Comment #11
amitaibuI'll play around with it a bit more -- first time PSRing! So you can have a look at it tomorrow.
Comment #12
swentel CreditAttribution: swentel commented@bojanz - just pushed #1043198: Convert view modes to ConfigEntity forward, something that entity reference field can benefit from as well.
Comment #13
amitaibu@bojanz, want to see something cool? Lots of @todos, but still :)
Comment #14
andypostPlease, take care of proper separation of instance and field level settings as discussed in #1340098: Move target bundle to the field instance settings
Comment #15
Damien Tournoud CreditAttribution: Damien Tournoud commented@andypost: the target entity type will have to stay a field setting, the rest can move to an instance setting.
Comment #16
Damien Tournoud CreditAttribution: Damien Tournoud commented@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.
Comment #17
Damien Tournoud CreditAttribution: Damien Tournoud commentedI opened a separate issue for the field behaviors: #1803064: Horizontal extensibility of Fields: introduce the concept of behavior plugins.
Comment #18
andypostWe 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
Comment #19
amitaibu@Bojanz,
I've pushed my recent work to
1801304-amitaibu
in ER. Basically it's ER7 ported to 8, we still need to do some cleanup, and tests, but it's already working.Comment #20
amitaibuComment #21
amitaibuSweet, all the tests are now fixed and green :)
Comment #22
amitaibuStill work in progress, but worth uploading a first patch to get some initial reviews.
Comment #23
falcon03 CreditAttribution: falcon03 commentedHi Amitaibu,
one of the things that makes me prefer ER instead of term reference is the ability to add field values "on the fly", even using the "select list" widget (provided by entity connect).
Will this feature be moved to core too?
I think it is really a good one and it will be great to have it in core!
Comment #24
amitaibu> 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
Comment #25
amitaibuSetting to needs review for testbot.
Comment #27
tim.plunkettRerolling, adding
use Drupal\Core\Database\Database;
Comment #29
amitaibuSorry, I think I rolled a wrong version in #22, retrying.
Comment #30
nils.destoop CreditAttribution: nils.destoop commentedI see that you are still using hook_field_widget_info. Widgets are beïng converted to plugins. The changelog can be found at http://drupal.org/node/1796000
Comment #31
nils.destoop CreditAttribution: nils.destoop commentedComment #32
amitaibuI'll be working on converting to widget and formatter to plugins.
Comment #33
amitaibuWidgets and formatters converted to plugins.
Comment #34
yched CreditAttribution: yched commentedHard for me to do a real, in-depth review, but here are a couple notes from visual inspection.
This and the factory presented as a static getInstance() method in the SelectionInterface itself is completely out of line with the usual instantiation flow promoted by the Plugin API (which is to call PluginManager::getInstance()). Could we get back to the regular Plugin API flow, or at least comment why a different flow has to be used ?
I think this this the correct way to do it, and it should work right now, the @todo can be removed.
That phpdoc looks like an unedited copy/paste :-)
Obsolete, can be removed.
Also, the patch still contains mentions of EntityReferenceHandler in phpdocs, while it seems this is now SelectionInterface ?
You don't seem to use PluginSettingsBase methods - besides, SelectionBase::construct() receives no settings, so I assume a Selection object has no settings of its own (in the sense that a widget or a formatter have settings), and you have no use for that superclass - wrong copy/paste from widgets code ? :-)
Please don't use
$this->settings
directly, but$this->getSetting('foobar')
(like your current settingsForm() above does). I didn't specifically check the rest of the code, but this applies to all widgets and formatters code. When field types get converted to plugins, that's also how field and instance settings will be accessed, FWIW.According to our current standards, this should be AutocompleteWidgetBase.
I'm not in front of the upstream code, but this looks like uneeded precaution, or it would be needed on each and every existing widget implementation.
Comment #35
amitaibuThanks yched! I've addressed issues from #34
I have moved the
getInstance()
logic intoentityreference_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.
Comment #37
yched CreditAttribution: yched commentedYeah, sorry, my bad. That's because #1705702: Provide a way to allow modules alter plugin definitions got in without WidgetPluginManager being updated to include the new AlterDiscoveryDecorator. #1817180: Tests: hook_widget_info_alter() is not called anymore is open for this.
But your code as it stands now should work once this one gets fixed.
Comment #40
amitaibuThe test fails in
Drupal\file\Tests\FileManagedFileElementTest
which doesn't seem to be related to this patch.This test also passes on my local.
Comment #41
amateescu CreditAttribution: amateescu commentedGave this patch some core love:
- moved the selection plugin manger instantiation to the DIC, where it belongs
- removed the ugly hack for getting the right selection class for core entity types
- a full rundown on the docs
- and a lot of other small fixes all around, it's all in the interdiff :)
Comment #42
tim.plunkettWhen 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.
Comment #44
yched CreditAttribution: yched commentedIt's still unclear why the object creation flow is so different from the usual Plugin API one (getInstace() method on the manager class plus __construct() methods on the plugin classes, rather than getInstance() static methods on th plugin classes here).
We should switch to the regular flow, or document why it can't apply here
Comment #45
amateescu CreditAttribution: amateescu commentedFixed the tests and the first comment from #42. The second one needs more thought.
Comment #46
amateescu CreditAttribution: amateescu commentedBecause no one bothered to update this part of D7 code to new Plugin API from D8 :) But I agree, we should switch to that.
Comment #48
amitaibuOh, 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?
Comment #49
amitaibuI'll re-roll - removing getInstance() and some widget/ formatter fixes I've already done.
Comment #50
amitaibuOk, from now on, no uploading of wrong patches from my side! ;)
entityreference_get_selection_handler()
Comment #51
amitaibuAlso un-assigning myself, as others are, thankfully, working on this as-well.
Comment #52
amitaibuPatch removes PluginSettingsBase as previously commented by yched in #34
Comment #53
swentel CreditAttribution: swentel commentedGave this a quick spin and this already looks really really great. I tested referencing basic pages from the article content type and that seems to work out nicely.
One minor thing I came across is the selection of the 'Rendered as entity' formatter, other than that, code in general is looking nice, so wow, yes, let's try to get this in! :)
Because this is empty by default, the formatter summary returns nothing when you switch over to this formatter, see attached screenshot.
Maybe, in case the view mode is empty (because you switch to this formatter from another one), we should print out another message to inform people they should select a view mode ? Or simply go for a default. To be honest, haven't really looked what the behavior is in the D7 version.
Comment #55
amitaibuThe 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.
Comment #56
Damien Tournoud CreditAttribution: Damien Tournoud commentedWe have a single selection plugin (the base one):
... that needs to workaround various quirks about core entities and thus needs different implementations depending on the entity type.
So we have a factory method on the plugin class (
public static getInstance($entity_type, ...)
) that returns the proper implementation. Is there a better way to allow multiple implementations per plugins depending of the parameters?Comment #57
amitaibuPatch moves entity-type specific selection under the handler name (e.g.
Drupal\entityreference\Plugin\Type\Selection\base\user\User
)Comment #58
amitaibuCross post with #56.
@Damz, what do you think about my solution from #57?
Comment #59
BerdirNoticed 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.
Comment #61
cweagansI proposed adding entityreference to core in IRC just now and was directed here. I don't really have anything to add, other than +1 :)
Comment #62
yched CreditAttribution: yched commentedre 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)
Comment #63
yched CreditAttribution: yched commentedThat comment is still wrongly copy/pasted from widgets :-). "Interface definition for reference field selection plugins." should be enough.
Comment #64
yched CreditAttribution: yched commentedI 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.
Comment #65
tim.plunkettYes! This makes much more sense to me.
Comment #66
amitaibuBut if OG wants to provide its own selection handler? Same entity type -- different selection logic.
Comment #67
EclipseGc CreditAttribution: EclipseGc commentedSo, I don't promise that this code works, but organizationally it should be better.
I've moved all the classes around to make each implementation a first class plugin. Added a derivative handler to the base plugin that removes supported entity_types from it's list (it might should also check if there's a base table, I leave that as an exercise to other). I added an entityreference_selection alter callback (via the AlterDecorator).
In addition to all of the the SelectionBroken logic really seemed out of place, so I overrode the createInstance method of the plugin manager (oh added reflection factory there) so now a simple call to createInstance should result in an instantiated plugin, or the SelectionBroken fallback if no class was available. DamZ and I discussed a custom mapper but I didn't attempt to do that as I think Amateescu likely has a better handle on what should be there than I do.
I've not updated any of the code that was actually using plugins, just the plugin code itself. Hopefully this is helpful here.
Eclipse
Comment #68
amitaibuSome fixes to make EclipseGc's code work (EDIT: work as in not WSOD -- didn't change selection_handler() yet)
Comment #70
amitaibuMoved logic out of get_selection_handler() -- however, I'm only getting the Base class, and I never see the code in
Drupal\entityreference\Plugin\Derivative\SelectionBase
being called. @EclipseGC, am I passing something wrong to the plugin manager ?Interdiff against #67
Comment #71
joshmillerComment #73
bojanz CreditAttribution: bojanz commentedI'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.
Comment #74
amitaibu@amateescu,
Are you going to re-roll with a fix to the Derivative code?
Comment #75
amitaibuRe-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
Comment #77
amateescu CreditAttribution: amateescu commentedHere'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...
Comment #79
amitaibuWhy add a 'weight' property? Wouldn't a 'base' property (TRUE/ FALSE) be better to indicate which one is a base and which is a derivative?
Comment #80
amitaibuI've committed #1821060: Return values from getReferencableEntities() keyed by bundle to 7.x, so added it to 8.x as-well.
Comment #82
amitaibuFollowing 7.x -- Split autocomplete callback to allow other modules (e.g. OG, OG-vocab ) to re-use ER's code.
Comment #83
amitaibuI don't know why simpltest are failing, there are running locally ok.
Comment #85
amitaibuFollowup on 7.x.
Now that Views is in core, we should also add a ViewsBase selection handler.
Comment #86
amitaibuI've started porting the Views selection handler in yched's sandbox under
entityreference-1801304-views
. Don't hesitate to land a hand ;)Comment #88
amateescu CreditAttribution: amateescu commentedPushed a small update for entityreference_options_list() from #1821060-7: Return values from getReferencableEntities() keyed by bundle. Now let's look at the tests :)
Comment #89
amateescu CreditAttribution: amateescu commentedLet's see if this one fixes the tests.
Comment #91
amateescu CreditAttribution: amateescu commentedGetting there :)
Comment #92
amitaibufollow up on 7.x --
getReferencableEntities()
should return Bundle's machine-name, and those should become a label inhook_options_list()
Comment #93
falcon03 CreditAttribution: falcon03 commentedtagging to let our accessibility team to review this patch before committing
Comment #94
falcon03 CreditAttribution: falcon03 commentedtagging; something went wrong with the previous comment and tags haven't been added.
Comment #95
amitaibuI think we are getting close, probably can start getting some reviews.. :)
@amateescu,
What do you think about #79?
Comment #96
amateescu CreditAttribution: amateescu commentedLet'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 :)
Comment #97
tstoecklerI know we recently introduced
keyvalue
into core, instead ofkey_value
, but for modules so far we have been consistent in splitting words with an underscore. I thinkentity_reference.module
would be a better choice.Don't know if we should keep the default or not, but if we do, this needs to be wrapped in a
module_exists('node')
, I think. I didn't actually try what happens when you disablenode.module
and add an entity reference field, so maybe this is bogus.I stumbled on this, as
options.module
is not required byentityreference.module
, but apparentlyoptions.module
does the same thing withlist.module
. Hmm...Leaving at needs review.
Comment #98
ParisLiakos CreditAttribution: ParisLiakos commented+1 for
entity_reference
.entityreference
always bugged me:/Comment #99
amateescu CreditAttribution: amateescu commentedFixed all the points from #97 in http://drupalcode.org/sandbox/yched/1736366.git/commitdiff/b01d222d0f9bc.... Not posting a new patch because everything needs to be updated for EFQ v2.
Comment #100
amitaibu> everything needs to be updated for EFQ v2.
I'll give it a shot.
Comment #101
amitaibuStarted a
entityreference-1801304-efq2
branch.I wonder if efq2 will allow us to remove some of the entityFieldQueryAlter() implementations (maybe in User?)
Comment #102
yched CreditAttribution: yched commentedThanks 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.
Comment #103
cosmicdreams CreditAttribution: cosmicdreams commentedpre_render from the Fields is this
which returns nothing, so this overridden pre_render shouldn't return anything either.
are we sure this function returns an array?
Comment #104
amitaibu#1828408: Re-add addTag() and AddMetaData() to EFQ
Comment #105
amitaibuentityreference-1801304-102
branch, to split ER into different field types. EDIT: Currently hook_field_widget_info_alter() is missing, so I've hardcoded some entities into the widgets and formattersComment #106
lpalgarvio CreditAttribution: lpalgarvio commented#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.
Comment #107
amateescu CreditAttribution: amateescu commentedThere are advantages and disadvantages for both approaches. We discussed this during the Gent sprint with yched and Damien but unfortunately we weren't able to reach an agreement.
On one hand, yched is really against the current approach because of UX, performance and widget/formatter concerns, and on the other hand Damien is really against chaging the current approach without having a sub-field-type concept in field api. I'm at a loss on how we can solve this in order to move forward here..
The latest work for field type per entity type is in the entityreference-1801304-102 branch from yched's field api sandbox, but I'm not pursuing it anymore because the workarounds that we have to do there are getting quite ugly.
Until we have some consensus for the issue above, I'm pursuing other things that everyone seems to agree on:
- UUID support (a separate 'target_uuid' column in the field table because in #1726734: Replace serial entity IDs with UUIDs in URLs, or even globally? it was decided to keep referencing by id internally)
- move selection handler settings from field level to instance level settings (except target_type, of course)
Comment #108
yakoub CreditAttribution: yakoub commentedyched, 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"
Comment #109
Fabianx CreditAttribution: Fabianx commented#107: So now that we know the positions, what are the reasons and especially goals Damien and yched have for their respective approaches?
I mean the general goal is clear: entity_reference in core
* Why does yched want to have a widget per field?
* Why does Damien want to change field api first?
to reach this goal?
Comment #110
sunHow 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)
Comment #111
amitaibuWhile we figure out #107, I've Added revision ID column. There's no UI for it, but if you set the value programmatically, and use the "Render entity" formatter you can see the difference. No tests yet.
Test will fail here until #1828408: Re-add addTag() and AddMetaData() to EFQ is in (it passes locally with that patch applied, tough)
Comment #113
eaton CreditAttribution: eaton commentedI'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.
Comment #113.0
amitaibuUpdated issue summary.
Comment #118
amitaibufyi, I've started to work on the "Behaviors plugins" in #1803064: Horizontal extensibility of Fields: introduce the concept of behavior plugins, (until we figure out #107, and get ER in)
Comment #119
amitaibuBetter handle getting base-table in
entityFieldQueryAlter()
Comment #121
amitaibuRe-key items array if entity was deleted.
Regarding #119 -- tests are passing locally for me..
Comment #123
yched CreditAttribution: yched commentedre @eaton #113
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)
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.
Comment #124
yched CreditAttribution: yched commentedMore generally on this question of "one single field type" / "one field type per target entity type" :
In the end, I don't really care how entity_ref does its job internally, as long as I don't have to maintain it :-).
My feeling is that :
- IMO this can only get in core if there's a plan to make it *replace* file fields, image fields & taxo fields, because duplicate field types would be confusing
- In doing so, we cannot degrade "too much" the UX of tasks that are likely to be the 1st things a newcomer tries on his freshly installed site (add an image or file upload fields, add taxonomy)
But in the end I won't be the keeper of *those* gates.
What I do know as a Field API maintainer is : while we can wish for a more granular way to say which widgets / formatters are eligible for which field, that's much more easily said than cleanly done :-).
For instance:
a) The exact granularity needed is unclear. "Field subtype", ugh, what is that exactly ?
It sounds like what is needed is a tag system - each existing field has tags derived of its configuration, and widgets say which tags they work with.
b) Potential for confusion and unpredictability - "what exactly to I have to configure where so that Widget 'foo' becomes available ?"
b) The tags on a field cannot change after field creation, or you find the widget you configured for the field does not apply anymore because you made this or that config change.
c) This does not play well with the current Field UI way of working (the list of widgets is presented upfront on "Manage Fields / add new field", and the field is created before you actually configure its properties). This can be changed, of course, and possibly should, but that's not a small preliminary task.
d) We still need to know a "default" widget applicable for all the fields.
That's a very non-minor change, it's very late in the D8 cycle to explore this, and I myself have more than enough on my plate already, so I'm not exactly looking forward to this...
Having separate field types doesn't require that much legwork on the underlying API. The problem of "sea of field types available in the 'add new field' dropdown" is strictly a UI issue, can be worked around.
- Config entities will account for at least 80% of the number of entity types. For starters, do we really want to have config entities in there ? "breakpoint reference", "input format reference", really ? It seems config entities have very different needs / capabilities in terms of widgets and formatters...
- Organizing the list of field type in optgroups ('Text', 'Number', 'List', 'Reference') would come a long way to make this dropdown more usable. That's a really low-hanging fruit, just one additional entry in widget plugin annotations...
Comment #125
eaton CreditAttribution: eaton commentedHmmmm.
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.
Comment #126
amitaibu@yched,
Taxonomy term -- absolutely, that's also stated as one of my goals in the issue summary. I've actually haven't thought about file/ image, for those we should first find the UI solution that will allow assigning the file-widget only to "File reference" entities.
It seems the UX questions might take awhile to be discussed and solved. I think it should be as follow-up issues (as well as deprecating taxonomy-term field), once ER gets in.
@eaton,
This is, if I understand you correctly, the current implementation. The field is generic, catering all entity-types. Regarding "which the id and entity type were both stored" In D7 we had the "target_type" inside the field, so theoretically you could reference via a single field multilve entity-types, but this was deprecated, as most implementing modules (e.g. Views) expect a single entity type to be referenced - and I think it's the right behavior.
Comment #127
sunFunnily, 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.
Comment #128
eaton CreditAttribution: eaton commentedI think the main lesson to learn from this thread is that I shouldn't post in issues while I"m high on cold medicine. Carry on, everyone. CARRY ON! ;-)
Comment #129
amitaibu> 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.
Comment #131
amitaibuThe test bot reports:
Symfony\Component\DependencyInjection\Exception\InvalidArgumentException: The service definition "plugin.manager.entity_reference.selection" does not exist. in Symfony\Component\DependencyInjection\ContainerBuilder->getDefinition() (line 690 of /var/lib/drupaltestbot/sites/default/files/checkout/core/vendor/symfony/dependency-injection/Symfony/Component/DependencyInjection/ContainerBuilder.php).
This doesn't happen locally, though. Seems related to the DIC?
Comment #132
chx CreditAttribution: chx commentedI 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.
Comment #133
Fabianx CreditAttribution: Fabianx commentedI strongly believe this is related to the fix here:
#1708692-59: Bundle services aren't available in the request that enables the module
Comment #134
Fabianx CreditAttribution: Fabianx commentedLets try that with the patch from #133 as multi patch.
EDIT: Oops, somehow I triggered a race condition that created the attachment twice.
Comment #137
amitaibuI'll be working on #132, although it can also be a follow up patch.
The current hold-back is the failing tests, which are for an unknown reason. Help is appreciated :)
Comment #138
amateescu CreditAttribution: amateescu commentedResponding to the bat signal :)
Comment #139
amitaibufyi, some discussion with fago on the property metadata implementation
Comment #140
amitaibuTest are passing, thanks amateescu!
Comment #141
nevergone CreditAttribution: nevergone commentedI'm fast tested:
- Download and install latest Drupal 8
- Apply the patch, cache clear
- Generate nodes and users
- Create two entity reference, target: node and user
The reference is works well, if:
Entity selection:
Mode: Simple selection
Sort by: Don't sort
But not works (empty result):
Sort by: A property of the base table of the entity
and Sort property is anything
Comment #142
amitaibu@nevergone, thanks. Indeed there was an error in the code and in the test :/
Attached the fixed code.
Comment #143
nevergone CreditAttribution: nevergone commentedTest:
- Download and install latest Drupal 8
- Apply the patch, cache clear
- Generate nodes, comments and users
- Create three entity reference:
source: user -> target: node, source: comment -> target: user
source: user -> target: node
widget: Select list
mode: simple selection
target bundles: Article
Sort by: Don't sort
default value: - None -
Display mode: no link
Create and edit existing user: Works well, correct operation
create another field (source: user -> target: node)
target bundles: Article
default value: some kind of Page type
Result: Works well: the field default value: - None -
source: comment -> target: user
widget: Check boxes/Radio buttons
mode: simple selection
sort by: A property of the base table of the entity
sort property: name
sort direction: Descending
default value: N/A
display mode: link
Create and edit existing user: Works well, but if select Anonymous, then fatal error, error message:
PHP Fatal error: Call to a member function label() on a non-object in /var/www/core/modules/field/modules/entity_reference/lib/Drupal/entity_reference/Plugin/field/formatter/EntityReferenceLabelFormatter.php on line 67
PHP Stack trace:
PHP 1. {main}() /var/www/index.php:0
PHP 2. Symfony\Component\HttpKernel\Kernel->handle() /var/www/index.php:35
PHP 3. Drupal\Core\HttpKernel->handle() /var/www/core/vendor/symfony/http-kernel/Symfony/Component/HttpKernel/Kernel.php:193
PHP 4. Symfony\Component\HttpKernel\HttpKernel->handle() /var/www/core/lib/Drupal/Core/HttpKernel.php:51
PHP 5. Symfony\Component\HttpKernel\HttpKernel->handleRaw() /var/www/core/vendor/symfony/http-kernel/Symfony/Component/HttpKernel/HttpKernel.php:73
PHP 6. call_user_func_array() /var/www/core/vendor/symfony/http-kernel/Symfony/Component/HttpKernel/HttpKernel.php:129
PHP 7. Drupal\Core\EventSubscriber\{closure}() /var/www/core/vendor/symfony/http-kernel/Symfony/Component/HttpKernel/HttpKernel.php:0
PHP 8. call_user_func_array() /var/www/core/lib/Drupal/Core/EventSubscriber/LegacyControllerSubscriber.php:52
PHP 9. node_page_view() /var/www/core/lib/Drupal/Core/EventSubscriber/LegacyControllerSubscriber.php:0
PHP 10. node_show() /var/www/core/modules/node/node.module:2451
PHP 11. node_view_multiple() /var/www/core/modules/node/node.module:1114
PHP 12. entity_view_multiple() /var/www/core/modules/node/node.module:2371
PHP 13. Drupal\Core\Entity\EntityRenderController->viewMultiple() /var/www/core/includes/entity.inc:542
PHP 14. module_invoke_all() /var/www/core/lib/Drupal/Core/Entity/EntityRenderController.php:128
PHP 15. call_user_func_array() /var/www/core/includes/module.inc:988
PHP 16. comment_node_view() /var/www/core/includes/module.inc:0
PHP 17. comment_node_page_additions() /var/www/core/modules/comment/comment.module:714
PHP 18. comment_view_multiple() /var/www/core/modules/comment/comment.module:741
PHP 19. entity_view_multiple() /var/www/core/modules/comment/comment.module:1042
PHP 20. Drupal\Core\Entity\EntityRenderController->viewMultiple() /var/www/core/includes/entity.inc:542
PHP 21. Drupal\comment\CommentRenderController->buildContent() /var/www/core/lib/Drupal/Core/Entity/EntityRenderController.php:121
PHP 22. Drupal\Core\Entity\EntityRenderController->buildContent() /var/www/core/modules/comment/lib/Drupal/comment/CommentRenderController.php:30
PHP 23. field_attach_view() /var/www/core/lib/Drupal/Core/Entity/EntityRenderController.php:63
PHP 24. field_invoke_method() /var/www/core/modules/field/field.attach.inc:1458
PHP 25. Drupal\field\Plugin\Type\Formatter\FormatterBase->view() /var/www/core/modules/field/field.attach.inc:181
PHP 26. Drupal\entity_reference\Plugin\field\formatter\EntityReferenceLabelFormatter->viewElements() /var/www/core/modules/field/lib/Drupal/field/Plugin/Type/Formatter/FormatterBase.php:101
create another field (source: comment -> target: user)
widget: select
mode: simple selection
sort by: A field attached to this entity
select field and sort direction
Create and edit existing user: Works well.
Comment #144
dawehnerWhat 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?
Comment #145
amitaibu@nevergone,
I've pushed a fix to #143
@dawehner,
Thanks for the review. I'll go over the non-Views issues first.
Comment #146
BerdirSupport 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.
Comment #147
chx CreditAttribution: chx commentedNote: EFQ expects you to have entity type in constraints and "id source" in settings for getPropertyDefinitions().
Comment #148
amitaibu@dawehner, I haven't addressed:
I don't remember now, but there was a reason for it, I'll need to try and remember :)
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?
This doesn't seem to be in Feed.php, and we need it for
views_get_applicable_views()
- did you mean something else?Sorry, didn't understand what you mean?
Comment #149
amitaibuDamien remembers :)
Comment #150
amitaibuAdded test for Views' entity-reference selection handler.
So, when does this patch, like hmm, you know... gets committed? ;)
Comment #152
amitaibuFailing tests don't seem to be related to ER. Let's try again.
Comment #155
DamienMcKennaThe failing tests are all for Views, so maybe wait a day for someone to fix it on the Views queue?
(in other words, argh)
Comment #156
tim.plunkett@DamienMcKenna, Views is in core, it's no different than any core module in terms of fixing failures. Have you tried running those tests locally? Those failures look like they'd be pretty evident on a local test run.
Comment #157
amitaibu@tim.plunkett,
Oops, indeed they fail also locally. Investigating :/
Comment #158
amitaibuI've renamed the test View, but the tests will still fail.
dawehner said he will have a look at it later on.
Comment #159
andyceo CreditAttribution: andyceo commentedComment #161
amitaibudawehner suggested via IRC to move the exported View to entity-reference-test module.
Comment #162
amitaibuGreen!
Comment #163
nevergone CreditAttribution: nevergone commentedI'm tested extensively (multiple sort mode: entity propery, field, views), and works well.
Great works!
Comment #164
BerdirHere'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.
Comment #165
yched CreditAttribution: yched commentedNo time for an extensive review tonight, just :
Could we name those classes differently than the corresponding entity classes ? Like NodeSelection ?
Having another Node class sounds really weird, despite namespaces.
[edit: actually, it's not just that it's weird, it's against code standards : "Classes and interfaces should have names that stand alone to tell what they do without having to refer to the namespace" - in other words : this is not a node, it's a "node selection", or a "node reference selection"]
Other than that, I have voiced my concerns about the "single field type" approach, and although I haven't seen proper responses to the "forbids widgets or formatters specific to an entity type" issue (other than "Field API should be made more flexible", which is still a pipe-dream at this point), I can live with being overruled :-)
In order to commit, though, I think it might be a good thing to bring core maintainers over to this issue, to get their feeling about the existing field types this would duplicate / possibly-but-maybe-not deprecate (image, file, taxo term).
Comment #166
yched CreditAttribution: yched commentedAlso, SelectionGeneric should probably be SelectionBase.
Comment #167
BerdirI was thinking about the name of the field and the module this morning and I was wondering if we shouldn't call this just Reference. The name Entity Reference made sense 7.x to separate it from the References module, but there's no need for that in 8.x.
Just thinking out loud...
Comment #168
amitaibuGreat, thanks for the review, I'll work on addressing the comments and opening follow-up issues.
Comment #168.0
amitaibuUpdated issue summary.
Comment #169
amitaibuI've updated the issue summary with the follow-up issues.
Comment #170
amitaibuAddressed the above comments except the following which I have no idea:
I think Entity-Reference is more descriptive. I know I can reference a generic entity, and not let say a config.
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"
Comment #171
Damien Tournoud CreditAttribution: Damien Tournoud commentedLet's get this in. EVERYTHING ELSE can be dealt with in follow-ups, please.
Comment #172
amateescu CreditAttribution: amateescu commentedI 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.
Comment #174
swentel CreditAttribution: swentel commented#172: entityreference-1801304-172.patch queued for re-testing.
Comment #175
swentel CreditAttribution: swentel commentedBack to RTBC.
Comment #176
yched CreditAttribution: yched commented@amateescu: the Base suffix doesn't mean all classes implementing the interface *have* to extend that base, it's just a hint.
Needs to be adjusted to the new class name (also applies to the other selection classes)
Comment #177
yched CreditAttribution: yched commentedCrosspost, didn't intend to change the status.
Although a quick reroll for the docblocks above would be sweet :-)
Comment #178
amitaibuRe-rolled according to #176
Comment #179
catchI haven't reviewed the patch yet, I'll try to soon.
Once we have this there are more follow-ups that come to mind, the answer to some of these might be a very reasonable 'no', but it'd be good to explore:
- what about using this for hierarchical relationships between taxonomy terms?
- and menu links? (by extension book module)
- comment threading?
All of these have significant performance challenges, but that'd give us the potential for pluggable hierarchy storage to allow the thorniest of them to be fixed in contrib (like taxonomy_get_tree() and other old friends).
- what about using it for node and/or comment authors instead of the uid property?
I'd also be very keen to see at least one conversion of an existing reference field, whether taxonomy terms or files, soon after this gets in. Otherwise I don't think we can really justify adding the feature in isolation (same as entity translation needs to get rid of node translation).
Comment #179.0
catchUpdated issue summary.
Comment #182
amitaibu> 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: Remove Taxonomy term reference field in favor of Entity reference, which is blocked by #1847924: Allow Entity-Reference field to autocreate items
Comment #183
amitaibuAccording to PIFR the test passed.
Comment #184
amitaibuRe-adding tag.
Comment #185
ParisLiakos CreditAttribution: ParisLiakos commentedtestbot having hard time with all the work we give it :P
Comment #186
chx CreditAttribution: chx commentedI am fine with a followup but can we make sure please that the new entity API is implemented some time?
Comment #187
amitaibu> please that the new entity API is implemented some time?
Yes, I've already started some work in #1847588: Implement the new entity field API for the Entity-Reference field type
Comment #188
amitaibuI can't assign it to Dries, but according to catch, Dries need to give his sign-off for this.
Comment #189
webchickI'll assign it to Dries. I don't know that he needs to sign off on it per-se, but it is a pretty big (and AWESOME) change, so doesn't hurt.
I'm trying to get us below thresholds atm, but I can't WAIT to see this committed. :D
Comment #190
Dries CreditAttribution: Dries commentedI'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.
Comment #192
amateescu CreditAttribution: amateescu commented@Dries, thanks for taking the time to actually test the patch :) While I agree with the usability concerns, this is still marked as a feature request.. and time is running out fast.
Tentatively assigning to catch for code review.
Comment #193
catchI'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.
Comment #194
moshe weitzman CreditAttribution: moshe weitzman commentedDries requested that the patch authors and UX Maintainers get together and improve the copy and the UX a bit more and then this can go in. Neither formal usability testing nor taxonomy reference conversion are a prerequisite.
Comment #195
webchickI 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.
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).
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.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"?
"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..?
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. :\
Comment #196
webchickOops, I missed a screenshot.
Comment #197
yoroy CreditAttribution: yoroy commentedFirst 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.
Comment #198
Bojhan CreditAttribution: Bojhan commentedPerhaps it makes sense to step by #drupal-usability and schedule in a moment, to go over it in depth and to figure out some solutions. We are often around, and we have office hours - next time please assign "Usability" tag earlier in the process as starting usability review upon RTBC is often hard.
@yoroy clearly laid out valuable improvements, it seems like we will need to structure these suggestions and explore more untill we can get to a solid plan as how to tackle the problematic UX parts.
Comment #199
webchickAaaand 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.
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.
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.
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.)
FEEDBACK:
Maybe just link right there from the error message directly?
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?
FEEDBACK
Ironically, this is showing it to me without anonymous, which is what I would expect. :D Go, Views!
FEEDBACK
Related to the "wizard" question above, can we default this to "Name" instead of create date (desc)?
...because you got distracted with 500 steps and forgot that you were actually configuring field settings. :P Oops. :P
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. :(
Comment #200
effulgentsia CreditAttribution: effulgentsia commented#199 means that ER could be a great initial use case for making ViewAddFormController::form() / Views Wizard plugins properly horizontally extensible. Currently, you can have a wizard per base table, but being able to say, "I'm launching this form from ER field settings configuration, so make it appropriate for that, by for example, automatically creating an ER display" would help address some of the issues webchick identified. And that would let us both make ER usable, and help make sure D8 contrib modules could also integrate fast Views creation workflows tailored to their needs. I'm totally ok with that being all done post-commit of initial patch, just pointing out an opportunity, and making sure our UX thinking isn't too constrained by what the current "Add View" form allows / doesn't allow.
Comment #201
amitaibuHad a conversation on IRC with webchick, in short:
Comment #202
nevergone CreditAttribution: nevergone commented"Hide the config entity from the UI. We can still add them programmatically."
This could be made conditional on some variables (or field property)? Given situation it may be necessary to make all entities visible.
Comment #203
BerdirI 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 ;)
Comment #204
catchOn listing config entities, I opened #1853856: Document that ConfigEntityBase and ConfigStorageController are tightly coupled. the fact that these two things are lumped into the same discovery/entity info with no real differentiation is a bug in itself, and the UX issue in this patch is just a symptom of that.
I haven't dived into this patch yet, but on using views for the selection, while it's very flexible I can see people wanting to provide custom listings without views (i.e. via raw EntityFieldQuery), how simple is it to extend it that way?
Comment #205
xjmFooClass instanceof ContentEntityInterface
was supposed to be the way to determine the distinction, except for users being weird and extending Entity directly right now.Comment #206
amateescu CreditAttribution: amateescu commentedHere's a small update which fixes 1), 3) and 4) from #201 as I think 2) deserves more discussion in a followup issue.
I also contacted Bojhan on IRC to talk a bit about how can we improve the initial patch, let's see how it goes :)
Re: @catch in #204
It's very easy actually, that's the purpose of the Selection plugin type. You can create a new plugin that extends SelectionBase (which is EFQ based) and go crazy with it :)
Edit: the
+// '#multiple' => TRUE,
leftover has already been removed in the latest sandbox commit. Not rolling a new patch just for that.Comment #208
amateescu CreditAttribution: amateescu commentedJust unbreaking some stuff from the previous patch..
Comment #209
amitaibu@amateescu,
Why not allow multiple selection on the user-roles?
Comment #210
amateescu CreditAttribution: amateescu commentedWe do allow that :) It was #type => 'select' with '#multiple' => TRUE but then I converted the type to checkboxes and forgot to remove #multiple.
Comment #211
Bojhan CreditAttribution: Bojhan commentedAlright, 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
User/Content/File
There are a couple of issues with this UI, primary some of the functionality grouping is really necessary.
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
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.
Comment #212
Bojhan CreditAttribution: Bojhan commentedComment #213
Bojhan CreditAttribution: Bojhan commented:D It seems just before feature freeze rush is over, hopefully this will see work before feb :) There is quite a lot.
Comment #214
amitaibuWork is currently done in #1847588: Implement the new entity field API for the Entity-Reference field type and #1847924: Allow Entity-Reference field to autocreate items + waiting for Catch's review on the current patch.
Comment #215
catchAssigning 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).
Comment #216
amitaibuFirst step in deprecating taxonomy-term is adding "auto-create" feature.
Patch adds "auto-create" option in the UI - with tests.
Note that currently referencing taxonomy terms is broken, from the code:
Comment #219
amitaibuI'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: Remove 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.
Comment #220
webchickWait. How can #1847596 get in without this issue? I'm confused.
Are you ready for a new UX review?
Comment #221
amitaibu> Wait. How can #1847596 get in without this issue? I'm confused.
No, what I meant is the code from #1847596: Remove 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: Remove Taxonomy term reference field in favor of Entity reference
Comment #222
amateescu CreditAttribution: amateescu commentedThere is still some work needed here before another UX review, #201 has been addressed but we still need to do Bojhan's feedback from #211. I'm on vacation and mostly away for another week but I'll try to see if I can steal some time for this :)
Comment #223
amitaibuVocabularies are now configuration entities, so we need to update the
entityQueryAlter()
method -- but not sure how?Comment #224
webchickI still think this would be a killer feature to have in D8. Escalating to a critical feature request.
Comment #225
amitaibu> I still think this would be a killer feature to have in D8
I agree :)
Work is now done in #1847596: Remove Taxonomy term reference field in favor of Entity reference. Help there is appreciated, cause it's going over and fixing lots of tests...
Comment #226
amitaibuI could use some help in #1847596: Remove Taxonomy term reference field in favor of Entity reference, deprecating field_tags, touches so many tests, and some of them are not just a simple find & replace.
Comment #227
amitaibu@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: Remove 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?
Comment #228
amitaibuI've merged some of the fixes into the main branch, let's see if testbot is still happy.
Comment #229
amateescu CreditAttribution: amateescu commentedHere'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
Comment #230
amateescu CreditAttribution: amateescu commentedCorrect patch and screenshot this time.. :/
Comment #231
joshmillerThe follow up amateescu proposed above exists and was part of the issue summary ... #1847590: Fix UX of entity reference field type selection
Comment #232
amateescu CreditAttribution: amateescu commentedRe-assigning to Bojhan at his request.
Comment #233
Bojhan CreditAttribution: Bojhan commentedA 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.
Comment #234
moshe weitzman CreditAttribution: moshe weitzman commentedIMO, it would be fine to change just the UI labels to 'Reference' and leave the code as is, with the more explicit 'entity reference'.
Comment #235
eaton CreditAttribution: eaton commentedSeconding "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.
Comment #236
pounardTarget type sounds good to me, we are not talking about the "reference" type but the "referenced object" type instead.
Comment #237
amateescu CreditAttribution: amateescu commentedImplemented 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 :)
Comment #238
amateescu CreditAttribution: amateescu commentedsigh :-<
Comment #240
amitaibuTest are failing as #1778178: Convert comments to the new Entity Field API got in. It's becoming really hard to chase head :/
Comment #241
amitaibu@amateescu, I'll be working on fixing the tests.
Comment #242
amitaibuI'm thinking we should change the key from
target_id
tovalue
. Apart of the whole find replace we need to do to adapt the tests, and different code to deal with finding the right key, I think it's be a better UX.There's no reason a developer should do
$comment->nid->target_id
. With$comment->nid->value
the developer doesn't need to know about the internal implementation.Comment #243
amitaibuLet's see if at least testbot likes my idea from #242 :)
Comment #244
amitaibubtw, the
target_id
key was introduced in this patch as legacy from D7. The current implementation in D8 is withvalue
Comment #246
amitaibuamateescu is against the target_id => value change, saying
Anyway, it's back to fixing the tests...
Comment #247
amitaibuIncremental progress, out of interest lets see how many more fails left.
Comment #248
amitaibuMissed a few more ->value to ->target_id replaces.
Comment #250
amitaibuWe should make sure to follow this pattern instead of hardcoding
->value
in foreach() when dealing with EntityNG. For exampleNot sure if my fixes covered all the failing tests, but got to run, so here's another patch :)
EDIT: I've improved the above code to have more context
Comment #252
amitaibuOops, changes #248 were somehow ignored. Re-rolled.
Comment #253
amitaibuOMG, it's green again! :)
Comment #253.0
plachUpdated issue summary.
Comment #256
aspilicious CreditAttribution: aspilicious commentedShouldn't we answer this question or open a followup for it?
Anyway I would love this to get in :)
Comment #257
amitaibu> 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 :)
Comment #258
stevectorLooks like this should go back to catch then :)
Comment #259
amateescu CreditAttribution: amateescu commentedI 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 :)
Comment #260
amateescu CreditAttribution: amateescu commentedI think this is as far as we can go without solving the problems from DatabaseStorageControllerNG in this patch.
Comment #262
amateescu CreditAttribution: amateescu commentedBah, failures were due to #611294: Refine permissions for Field UI. Fixed in the attached patch.
Comment #263
fagoThis will run on saving only, thus I don't see a big problem with it. But maybe that would be nicer?
Comment #264
sunAwesome work so far! Looks like we're really close to get this done! :)
I only found a few smaller things. I don't think anything of this should hold up this patch, but if we can fix them before commit, even better. :)
The auto_create condition in combination with the field_is_empty() condition is quite mind-boggling here ;)
Would it make sense to move the 'auto_create' condition first?
Not sure whether it will actually clarify the combined condition, but at minimum, it'll be faster ;)
Is there any particular reason for why we're excluding anonymous users for 'uid' here?
Or is it possible that the intention was to use
isset()
instead of!empty()
?That's some massive, ugly #process callback there...
The @todo the phpDoc is talking about looks like a one-line change in field_ui.admin.inc to me? So why don't we simply adjust it? :)
(Note: I've left out $form, since fields/instances/plugins shouldn't depend nor be aware of the full $form structure.)
1) Would UnknownSelection be a better name?
2) The form outputs a user-facing message that doesn't actually seem to be targeted at users? I wonder whether we should rather throw an exception somewhere?
It's unclear what implementations of these methods are supposed to do...
I guess the first probably expects an integer count as @return?
And the second is supposed to throw exceptions on validation errors? Or just return valid IDs and silently ignore invalid ones?
Unless I'm mistaken, CacheDecorator is now supposed to get a list of cache tags as last argument now. (I could be mistaken though.)
It's not really clear to me under which circumstances the selection handler plugin could vanish?
Now that I see an implementation, I wonder whether "validate" is the appropriate terminology for this method...? Looks more like a "filter"?
Also, any chance to move the
!empty($ids)
condition into the calling API code? Looks like needless duplication in all implementations?Renaming the $strict parameter to $required might help to clarify its purpose?
Alternatively, isn't #required available on the passed-in $element already?
That's fixed now. :)
Is it proper to use a select widget here, if there are just two options? Did we consider to use radios?
Stale "nodes" ?
I guess this should be a #type 'number' now?
It looks like this code originally passed an additional argument forward to
prepareElement()
, but that argument is no longer taken, so we can merge the code back intoformElement()
?I'm not too familiar with Views... but this PRE output looks like stale debugging code?
Would be great to leverage DrupalUnitTestBase for almost all of the new tests (they don't seem to use or need the internal browser)... but we can do that in a follow-up issue.
Hm. Multiple issues here:
1) Why are these plugin IDs prefixed with "base_"? Did you mean to use a '_default' suffix; e.g., "file_default"?
2) Why is the module = entity_reference, if the plugin is provided by file/node/etc module?
3) What's that "group" property and where does it get exposed?
Hm... That looks (a bit too) generic to me.
Something like that should probably be the base exception class for all exceptions being thrown in Views...
In any case, I'd think this should extend \RuntimeException?
Comment #265
amateescu CreditAttribution: amateescu commentedThanks sun for a great review! I think all these should be handled before commit `cause I'm tired of seeing followups everywhere :)
Comment #266
amateescu CreditAttribution: amateescu commentedI ran out of time for responding properly to every point from #246, so I'll just post the new patch and interdiff and leave the answers for the weekend :)
Comment #268
amateescu CreditAttribution: amateescu commented#266: entityreference-1801304-266.patch queued for re-testing.
That was a random test failure from #1817656: Let user_autocomplete also find the anonymous user..
Comment #270
amitaibuRe-exported Views, as Views now requires ID instead of Name
Comment #271
amitaibubtw, is it just on my local, or did auto-complete stop working in fields (e.g. taxonomy-term field)?
Comment #273
swentel CreditAttribution: swentel commented@Amitaibu, I've seen that as well. Author selection on node/add doesn't seem to work either, you will probably also get 'Theme hook hidden not found' messages in watchdog. I've posted a screenshot over at http://drupal.org/node/1886186 which is follow up on #1812724: Consolidate all form element templates and add theme_hook_suggestons which *probably* triggered this, although I'm not sure.
- edit - Core issue here #1893400: Autocomplete is busted with a patch
Comment #274
amitaibuI think the failing test in #270 (
Drupal\menu\Tests\MenuTest
) isn't related to this issue, as it fails on my local also on the main branch 8.xComment #276
sunThanks! Let's go ahead here :)
Comment #278
webchickOverall, LOTS of UX improvements this time. Thanks for that. However, did anyone give this a final run-through? It seems like not, because I'm hitting quite a few problems trying. Might just need a re-roll, not sure, but...
1) I'm getting this on the configure field form after adding the field:
2) When I attempt to add a user role filter, I get:
3) I added an entity reference view/display to my field, and when attempting to load the front page I get:
If this isn't just from HEAD changes in the past couple of days, we're going to need more test coverage here. I'm going to send it for a re-test to see.
Then just as a side-note to fatal errors and whatnot. When you create a view with an entityreference display, you get this error:
"Display "EntityReference" needs a selected search fields to work properly. See the settings for the Entity Reference list format."
This error message is confusing, because the view *already* has fields on it, and "search field" is some special thing that EntityReference introduces. We need some guidance here like "Configure the search field by clicking Settings next to blah blah", cos I got stuck here.
However, when I go to that form, "User: Name" is the only option available. So why can't it just auto-select it by default? That would've made this super simple.
Comment #281
swentel CreditAttribution: swentel commented#1822458: Move dynamic parts (view modes, bundles) out of entity_info() got in, that's most likely causing the first failure that webchick gets.
Comment #282
catchJust a note I started reviewing this, got around 1/3rd in, didn't have much feedback at that point, then got distracted.
If Dries or webchick gets to this before me and is happy committing I don't want to hold it up, so un-assigning myself.
Comment #283
amitaibuI'm out for the next week, so I'd be happy if someone else can bring this patch home.
Comment #284
effulgentsia CreditAttribution: effulgentsia commentedRerolled for #281.
Comment #286
effulgentsia CreditAttribution: effulgentsia commentedUpdated the test introduced in #1854874: Add serializer support for XML for the
value
->target_id
change.Comment #287
effulgentsia CreditAttribution: effulgentsia commented#278.2 is still happening, so "needs work" on that. I'm not clear on what the steps are for #278.3.
Comment #288
amitaibu@effulgentsia, thanks for working on this - it would be great if you could use our development git in yched/entityreference-1801304
Comment #289
amateescu CreditAttribution: amateescu commentedSo, let's get this back on track :)
@webchick, yes, 90% of the fails you saw were from HEAD changes and that Exception error was a missing
t()
wrapper on the exception message :/Merged @effulgentsia interdiffs as commits in the proper branch and fixed #278.2 and #278.3. I'll followup with a proper response to #263 and #264.
Back to RTBC if the bot agrees.
Comment #290
webchickAwesome. This seems to be working great now, and can't find much else to complain about that can't be done in a follow-up (e.g. that auto selecting of a search field thing). The display mode options are super handy, too!!
I think Dries might want another pass at this, since the intent is to use it in place of other *Reference fields and in any case we're blocked by thresholds atm anyway. :(
Comment #291
sunThe only way to make sense of this and to make this work and truly happen for D8 is to commit it now. (or rather: "yesterday")
Comment #292
effulgentsia CreditAttribution: effulgentsia commentedI agree with #291. While I appreciate the general usefulness of us having thresholds that block feature requests, I also think it's fine to make sensible exceptions to that when it's for a critical feature that other work is waiting on. Assigning to Dries in case that's the kind of decision only he can make, though I have no objection to webchick or catch committing if they're feeling bold.
Comment #293
moshe weitzman CreditAttribution: moshe weitzman commentedI also think this is a major building block for other patches and thus we can sensibly exempt it.
Comment #294
cweagans+1 for an exemption. This is pretty important.
Comment #296
Dries CreditAttribution: Dries commentedGood progress, but more work to do:
1) Testing this with a view, I got:
2) The 'Automatically create new entity' feature seems to have bugs when tried with users and default settings on content. It also raises a lot of questions as how it can work when entities have various required fields etc etc. Let's take this feature out for now, and create a follow-up issue to bring it back.
3) When initially selecting "User" content and "name" as sort field, then changing to a Content entity instead of User, no "name" field exists so you get this error on node/add/article:
Comment #297
webchickOne other minor thing, that hopefully is easy to do, can we default "Link to entity" on the entity reference field display settings? Without that, it's hard to tell if it actually worked or not.
Comment #298
amitaibuMaybe instead of taking it all out, we can - for now - limit it to taxonomy terms? like this after ER gets committed, we can continue with #1847596: Remove Taxonomy term reference field in favor of Entity reference.
btw, The required field did come up when we planned that feature, and is actually present also in the current D7 term-field -- you can create a new term, even if that bundle has a required field. Not sure what's the best solution is, though.
Comment #299
amitaibuI'll be working on #296.2 (limiting to taxonomy terms) #296.3 and #297
Comment #300
webchickI think that'll work. Just note that one of the problems we ran into testing this is that the newly created entities were unpublished by default. This was in the case of nodes, not sure about terms.
Comment #301
webchickBtw, this is now 300 comments, so I went through and
unpublisheddeleted a bunch of system message + one-line comments in an effort to keep the dreaded pager from appearing.Comment #302
moshe weitzman CreditAttribution: moshe weitzman commentedI want to echo a concern that Berdir raised in #203. Looks like we declare target_id as an integer. In general, the entity system does not require integer ids and we should ideally not start doing that here. Using integers mean we cannot reference to config entities, which have machine names as ids. Config entities are now a major part of Drupal, and even have EFQ support.
If we keep the design of a single entity type per field, we could make the schema dependent on the target type (details TBD). Until then, Perhaps we should explicitly exclude config entities
Comment #303
amitaibuPatch fixes issues from #296 (all three issues) and #297
* "auto create" option is no visible in the UI only for taxonomy terms.
* When the field's target-type is changed, the instance's hander settings are reset
Comment #304
amitaibu@moshe weitzman,
Yes, that's currently what happens in
entity_reference_field_settings_form()
we filter outConfigEntity
Comment #306
amitaibu#303: entityreference-1801304-302.patch queued for re-testing.
Comment #307
amitaibu"Token scan" test passes on my local. Re-testing patch.
Comment #308
amitaibuGreen :)
Comment #309
moshe weitzman CreditAttribution: moshe weitzman commentedOK, Lets go with this.
Is there a followup to change the storage from integer to varchar when referencing a config entity or a content entity that has non-integer keys (core doesn't do this but still).
Comment #310
sunNot sure whether we need a separate/dedicated issue just for entity_reference? We have this one already:
#1823494: Field API assumes serial/integer entity IDs, but the entity system does not
Comment #310.0
sunUpdated issue summary.
Comment #311
star-szrHere are some minor items for docs cleanup.
Typo: createInstance().
Can we wrap these at 80 characters please? There are a few other comments that are at 81 chars but I don't want to dump them all here.
Comment #312
amitaibu@Cottser, thanks. If there are more places it would be very helpful if you could re-roll the patch.
Comment #313
YesCT CreditAttribution: YesCT commentedtask A. add reference to user
following #195
task B. add reference to user, filter choices by role
following #199
task C. add reference to user, filter by custom view
Since that was so easy, going through similar to #199 view wizard
"Advanced"(advanced is a section on the next view screen. dont need that) next view screen, you get to do the following elaborate dance...check if there is a follow-up to auto pick the primary field for the typefollow-up #1904900: Default auto pick the primary field for the type in view wizard to avoid red error: 'Display "EntityReference" needs a selected)check if there is a follow-up for Maybe just link right there from the error message directly?add as possible solution to #1904900: Default auto pick the primary field for the type in view wizard to avoid red error: 'Display "EntityReference" needs a selected)Page not found The requested page "/javascript%3Avoid%28%29" could not be found.
Start all the way over at 1. (oh, pick username raw this time, operator starts with, enter a). OH! reordering filters also errors. start over again, again... dont reorder or remove anything.]Whew!
[edit:
OK. I'm saving what I have so far. My goal was to try and go through some steps to test, just regular first to get a feel for it, and then to try it with some multilingual something. But I didn't make it that far yet. I'll try and come back to this later today.I also need to check and see if follow-ups I confirmed are still problems are already listed in the issue summary in the follow-up list. So I'll edit this comment later with that info about the follow-ups: if they exist already or not.[edit: added follow-ups to issue summary]] [edit: resuming above with step 14.]Comment #313.0
YesCT CreditAttribution: YesCT commentedUpdated issue summary, added links to comments that sound like they have steps to test.
Comment #314
webchickSorry to keep pushing back on this, but if there's anything we don't need right now it's more critical bugs. :(
That JS error YesCT ran into seems like it needs to be addressed. Not sure if it's strictly related to this patch, though.
We hit one big issue in the most recent round of testing, which is that when we deleted the Entity Reference display in our view and then went to add a different one, every node-related page fails hard with:
This seems to be a variation on #1898804: Create a view block, place the block, delete the block, BOOM your site is dead.
Additionally, filtering users by the "Authenticated user" role didn't work; the list was empty. So either we should fix that (if it can be fixed; Moshe pointed out that that might not be queryable) or we should remove it from the list of available filter options. I also found it weird that on every other form the filters are displayed inline, but with users it's in a dropdown with only one option.
Additionally, Dries had some feedback on some wording things:
- The fieldset that you use to configure how the reference field gets populated is labeled "Reference Type", however that doesn't actually describe what options lie below. Suggestion was "Reference Settings" although the whole page is a settings form, so not sure.
- There are other labels that are extremely technical sounding like "View used to select the entities" and "Create referenced entities if they don't already exist" At the point of this form, you already know what kind of entity you're dealing with, so it should just be "content" or "terms" or whatever.
I imagine this isn't solvable, but it's really odd to see "EntityReference" strung together in the Views display listings. No way of making that "Entity Reference" is there?
We'll take another pass at this tomorrow, if it's ready, and Dries thinks it may be able to be committed then.
Comment #315
YesCT CreditAttribution: YesCT commentedI added followups (and edit, again, #313). I promise not to edit that comment again.
If the followups are duplicates, sorry. Just close them duplicates or let me know and I'll do it.
Comment #315.0
YesCT CreditAttribution: YesCT commentedUpdated issue summary with followups from #313 and other places
Comment #316
YesCT CreditAttribution: YesCT commentedSteps to test with multilingual
do it again more careful... with a use case.
task B. select a user reference, and only list user's who language setting in their account matches the language of the translation being edited.
make an entity reference view that takes an argument and lists users with that language in their account.
uh, I dont know how to do that... [edit: #1904934: use views context and translation langcode to filter user listing ]
Comment #317
YesCT CreditAttribution: YesCT commentedI can confirm that the deleting filter criteria from the rearrange is in a clean install of 8.x. Tim says a separate issue for that is coming.
I think that leaves mostly the other things in #314
Comment #318
tim.plunkett#1904932: Cannot remove a handler from the "rearrange" popup is the Views UI bug.
Comment #319
star-szr@Amitaibu - Thanks! I would have done a reroll in the first place but honestly I wasn't sure what was happening with sandboxes etc. - see #288.
Here's a reroll with some more docs updates. I tried to make the capitalization of "Entity Reference" more consistent as well when referring to the module and not the field. See #1430452: Use Proper Name Case for core modules.
The only code-related change I made was to remove one commented-out line of code from \Drupal\entity_reference\Plugin\views\display\EntityReference.
Comment #320
amateescu CreditAttribution: amateescu commentedI took care of most the issues from #314, namely:
- removed the authenticated role from the list
- changed that pesky EntityReference to Entity Reference
- switched the RuntimeException to a warning message, and also tweaked the message a bit with Bojhan on IRC
The strings that Dries thought were too technical were actually the result of Bojhan's UX review..
Also committed Cottser's patch to the sandbox, thanks for it btw! And thanks to @YesCT, we now have followups for everything else.
Comment #321
Bojhan CreditAttribution: Bojhan commented@amateescu I cannot remember reviewing those texts? I reviewed the primary labels, not each select option.
Comment #322
amateescu CreditAttribution: amateescu commentedThat's true, some others came from @eaton in this issue :) Anyway, any suggestion for improving them are welcome, as usual.
Comment #323
effulgentsia CreditAttribution: effulgentsia commented#319 and #320 look good. I think polishing strings and other things can happen more productively in follow ups, so RTBC.
Comment #324
Dries CreditAttribution: Dries commentedThis looks good now. Committed to 8.x. :-)
Setting to 'needs work' as there are a couple of follow-up tasks that need to be created and linked to from this issue.
Also could benefit from a CHANGELOG.txt and/or a Change Notice.
Thanks everyone!
Comment #325
amateescu CreditAttribution: amateescu commentedWow, creating this small patch felt so good :)
I also updated http://drupal.org/node/1796546 to include the ER field.
Comment #326
cweagansComment #327
webchickCommitted and pushed to 8.x. Thanks!
Comment #328
tim.plunkettComment #329
tim.plunketthttp://drupal.org/node/1796546 already covers this. Sorry for the noise!
Comment #330
effulgentsia CreditAttribution: effulgentsia commentedBack to original priority for posterity/statistics.
Comment #330.0
effulgentsia CreditAttribution: effulgentsia commentedUpdated issue summary with location of most recent steps to test.
Comment #331.0
(not verified) CreditAttribution: commentedadd another follow up
Comment #332
xjmAdded #1953568: Make the bundle settings for entity reference fields more visible. to the list of followups in the summary.
Comment #332.0
xjmUpdated issue summary.
Comment #333
jhedstromAdded #2429699: Add Views EntityReference filter to be available for all entity reference fields.
Comment #334
cilefen CreditAttribution: cilefen at Institute for Advanced Study commented