Problem/Motivation
Since Entity reference module patch went in the taxonomy reference field, duplicates functionality already provided by entity reference.
Proposed resolution
Turn existing taxonomy reference fields into entity reference ones and remove the taxonomy reference field.
The patch has been authored by the Entity Reference maintainers (@amitaibu and @amateescu) and has signoff from the Taxonomy module maintainer (@xjm). @Dries also approved the direction of the patch in #195 (as of November 2014, during the beta phase). Since it introduces significant disruption, it has an upgrade path deadline.
Beta phase evaluation
Issue category | Task because there is no functional bug. |
---|---|
Issue priority | Major because it:
|
Prioritized changes | The main goal of this issue is usability. The patch also improves maintainability by removing what is essentially a second code path for the same functionality (diffstat: 373 insertions, 2042 deletions). |
Disruption | The patch introduces significant disruption:
|
Remaining tasks
- Address usability issue with presenting "Term reference" field selection on two different steps of the Add field workflow (see #261). To be addressed in #2458777: Improve the user experience for the "preconfigured field options" concept.
- A possible beta-to-beta migration path will be provided in #2456419: Upgrade to Drupal 8 and provide a beta2beta submodule.
- A labeling inconsistency in the field UI between adding and managing fields will be addressed in #2458127: Show field/field-storage summary instead of field type on field listings.
User interface changes
The taxonomy field storage and field settings are replaced by the entity reference ones.
API changes
The 'taxonomy_term_reference' field type is removed, including widgets, formatters, the taxonomy autocomplete controller, etc. All functionality is replaced with Entity Reference functionality.
Related issues
- #1953444: Make 'target bundles' required on the Entity reference instance settings form
- #1953832: Replace 'target type' on entity reference field settings with something clearer
- #1818560: Convert taxonomy entities to the new Entity Field API
- #1120144: Term Reference Autocomplete widget - add option to match only start of term and expose settings (size, match) (for 8.x) is waiting on this
- #1847590: Fix UX of entity reference field type selection
- #680546: Add a custom 'number of values' for 'multiple / multi-value' field. (e.g. nodereference, imagefield)
- #1953568: Make the bundle settings for entity reference fields more visible.
- #1953770: Move the field-specific settings form elements at the top of the form
- (waiting on usability testing) #1953438: Don't expose the term 'Entity' to users
- #889378: Create taxonomy fields from the taxonomy administration page
- #394482: Users should be able to enable a vocabularly from the content type configuration page
- #1963340: Change field UI so that adding a field is a separate task
- #2412569: Allow setting the auto-create bundle on entity reference fields with multiple target bundles
Original report by Amitaibu
That's kind of the whole purpose of #1801304: Add Entity reference field :)
Comment | File | Size | Author |
---|---|---|---|
#286 | er-steps.png | 203.29 KB | webchick |
#279 | interdiff.txt | 5.43 KB | amateescu |
#279 | bye-bye-taxonomy-reference.patch | 142.92 KB | amateescu |
#273 | interdiff.txt | 4.27 KB | rteijeiro |
#273 | er-1847596-273.patch | 141.25 KB | rteijeiro |
Comments
Comment #1
ParisLiakos CreditAttribution: ParisLiakos commentedsetting right status then
Comment #2
amitaibuAssigning to myself. Seems like a big task, so wouldn't mind some help ;)
Comment #3
amitaibuComment #4
amitaibuSetting to needs review just this time for Testbot (to fail). Patch is expected to be very broken still.
Also added a diff against #1801304: Add Entity reference field
Comment #6
amitaibuStarted the branch from scratch, as core changed too much.
Interdiff is against the main ER branch. Patch is currently just lots of find & replace. Letting bot fail, to see which tests need work.
Comment #8
amitaibuThis issue will need also to deal with #1586428: Integrate entyreference from nodes to terms with core taxonomy_index from ER7.
Comment #9
amitaibuLet's see how mant tests still fail...
Comment #11
amitaibuSome more tests are failing, but getting there...
Comment #13
amitaibuMore (not all) failing tets fixes.
Comment #15
amitaibuAgain, for testbot - converting more tests. If anybody is around to give a hand -- lots of work here..
Comment #17
amitaibu2 more tests fixed, don't know why #15 didn't apply.
Comment #21
Damien Tournoud CreditAttribution: Damien Tournoud commented^ This looks bogus. I guess you are looking for the formatter type and settings, don't you have that in
$element
already? (You have the formatter type for sure in$element['#formatter']
).Comment #22
yched CreditAttribution: yched commentedYou get $element['#formatter'] = the plugin_id of the formatter.
You don't have the formatter settings in $elements, though.
Right now if you want to get the settings of the actual formatter object being used for render, you should do something like :
[edit: obviously we could put more stuff in $element: the $display object, the $formatter plugin object... My only worry is serialization of all of those in case of render caching]
Comment #23
amitaibuThanks Damien for going over the patch!
Re #21 Yeah, it's a nasty hack - to my defense it has a todo :) - because I thought #1875970: Pass EntityDisplay objects to the whole entity_view() callstack should inject the EntityDisplay. @yched, am I wrong?
We are down to 12 failing test (a.k.a lots of good progress), 2 of them related to upgrade, which I wonder how I should attack -- is there an example in core already for converting from one field type to another?
Comment #24
yched CreditAttribution: yched commentedThat hunk is in rdf_field_attach_view_alter() - hm, crap, currently #1875970: Pass EntityDisplay objects to the whole entity_view() callstack leaves out hook_field_attach_view_alter().
Can't remember right now if that was a deliberate omission, or whether it was just because I'd so wish to get rid of those hook_field_attach_*() hooks in favor of the the existing hook_entity_*() hooks - which is not that easy.
But yes, while it's there, hook_field_attach_view_alter() should probably receive the $display too. I'm not sure I'll be able to update the #1875970 patch before a couple days though. Meanwhile, the code in #22 should work.
Converting fields from one type to another - aw, no, I don't think that's something that was ever done in core. Doesn't ER D7 have some support code for converting old [node|user]_reference fields, though ?
Comment #25
amitaibuIncremental commit - More fixes. Interdiff is now 100K!
> But yes, while it's there, hook_field_attach_view_alter() should probably receive the $display too
No problem, I'll wait until it gets in, and remove the hack.
> aw, no, I don't think that's something that was ever done in core.
hmm, no such code in ER7, so I guess it will be fiddling directly with the schema and some field_update_field()
@chx, note I've changed
Tables::addField()
to get the $propertyDefinitions form a mocked entity, to makeEntityQueryRelationshipTest
passComment #26
yched CreditAttribution: yched commentedfield_update_field() doesn't allow changing the field type.
But using it is not allowed within an update func anyway - direct writes to the {field_config*} tables instead.
For field values: well, you will only be able to handle the case of fields stored in field_sql_storage. Fields stored in contrib storage backends are out of reach...
Comment #28
amitaibuOk, we are down to 6 failing tests -- 3 upgrade & 3 Views!
I'd be happy if someone can grab the Views tests, while I'll deal with the upgrade... Please? :)
Comment #30
amitaibuOMG! the upgrade path is in place, and the upgrade test passed!
Comment #32
amitaibuOops, seems the upgrade tests are not finished yet. Not sure what I did to see it green :/
Comment #33
amitaibuAttached patch that fixes the upgrade path for real :)
@yched, I've changed in field_update_instance() and _field_write_instance():
The upgrade code is in
field_update_8003()
- It's not under entity_reference.install, as I want to make sure term fields are converted on updb.3 more Views tests to go...
Comment #35
yched CreditAttribution: yched commentedFunctions like field_read_fields(), field_update_field(), field_update_instance(), entity_get_display() and loading config entities are not supposed to be used in update functions, because they fire hooks.
- working on field and instance definitions has to be done by directly accessing the definition records (currently rows in the field_config* db tables, in the future raw CMI files). There are a couple helper _update_7000_field_*() functions already to that effect, but currently no function to update definitions though...
- working with $entity_display config entities can be done with _update_8000_entity_get_display() (update-friendly replacement for entity_get_display(), works on the underlying CMI data, not on the config entity). See user_update_8011() for an example.
Comment #36
amitaibuThanks @yched, I've added
_update_7000_field_update_field()
and_update_7000_field_update_instance()
.Comment #37
amitaibuSo I guess now we can continue the work here... :)
btw, "entity_reference.module" is missing from the "Component", whos' the guy/girl that can add it?
Comment #38
amitaibuLet's see where we are with the tests
Comment #40
amitaibuThe upgrade test seems to work on my local, let's re-try.
Comment #42
amitaibuTwins at home == no time this week or two for this patch ;)
Hope someone else can work on those failing test - I think there isn't too much work there
Comment #43
amateescu CreditAttribution: amateescu commentedHere's some progress.
Comment #45
amateescu CreditAttribution: amateescu commentedAnd with even more fixes and rollbacks of unnecessary changes :)
Comment #46
Dave ReidThe autocompletion changes look suspect. Looks like we're dropping a bunch of tests related to autocomplete, but not adding any tests for them? Are there already existing tests for the path
entity_reference/autocomplete/tags' . $tag_field_name . '/' . $tag_field_instance['entity_type'] . '/' . $tag_field_instance['bundle']
? This looks like it doesn't even match the entity_reference_menu() entry since there is no backslash in-between 'tags' and $tag_field_name.Comment #47
Dave ReidA few more quick things just based off patch read:
A lot of the tests in TermFieldTest seem like they could still be valuable, especially testTaxonomyTermFieldChangeMachineName()
Am I able to create an entity reference field that references not taxonomy terms and still select the entity_reference_autocomplete_tags widget? Should I actually be able to do that since my gut says no I shouldn't since it's very taxonomy term specific.
Comment #48
amateescu CreditAttribution: amateescu commentedThat's exactly what I said in IRC a couple of minutes ago, that we might've dropped too many autocomplete tests :/ I'll look into the changes prior to #43 tomorrow.
Comment #49
amateescu CreditAttribution: amateescu commentedThe autocomplete tags widget is not taxonomy term specific at all, just the auto creation of terms part of it, and that has been moved already to the taxonomy implementation of the entity reference Selection plugin.
Comment #50
Dave ReidIt looks like entity_reference module should actually support hook_field_attach_rename_bundle() to automatically fix any bundle renames for all entity types in all entity reference field configurations. That code shouldn't be living in taxonomy module anymore. Should also move testTaxonomyTermFieldChangeMachineName to a generic test in entity_reference.
Comment #51
ParisLiakos CreditAttribution: ParisLiakos commentedOk, i generated 10 nodes, added a few terms on them, applied patch run update.php.
When i visit /node teasers appear fine, that means that data where succesfully transferred to the entity reference field.
But when i try to edit a node:
Warning: array_key_exists() expects parameter 2 to be array, null given in Drupal\field\Plugin\PluginSettingsBase->getSetting() (line 50 of core/modules/field/lib/Drupal/field/Plugin/PluginSettingsBase.php).
Fatal error: Unsupported operand types in /var/www/d8/core/modules/field/lib/Drupal/field/Plugin/PluginSettingsBase.php on line 60
This also happens when i try to add a new node that has a taxonomy er field attached.
Field UI on the article content type again:
Notice: Undefined index: weight in Drupal\field_ui\FieldOverview->form() (line 112 of core/modules/field_ui/lib/Drupal/field_ui/FieldOverview.php).
Notice: Undefined index: weight in field_info_max_weight() (line 519 of core/modules/field/field.info.inc).
didnt have time to dive through the code, but a quick scan i spotted this..i know very minor, compared to upgrade path
i suppose you could store container in a variable the first time
Comment #52
Crell CreditAttribution: Crell commentedNote that the autocomplete logic has an RTBC patch to turn it into a pure route: #1801356: Entity reference autocomplete using routes. That will likely land first, which will impact this patch.
Comment #53
amateescu CreditAttribution: amateescu commentedYep, and the impact is that we'll need to basically undo that patch and implement it again for entity reference :(
Comment #54
Crell CreditAttribution: Crell commentedSince this patch would render that one redundant, what are the odds of this one actually landing? If they're high, I'm happy to postpone that one and let this issue steal its model and do it for ERs now, in this issue.
Comment #55
ParisLiakos CreditAttribution: ParisLiakos commentedi am pretty sure, this was one of the main reasons to get entity reference into core in the first place, so i would say confidently that this goes in
Comment #56
Crell CreditAttribution: Crell commentedTalked to catch in IRC. Marking #1801356: Entity reference autocomplete using routes as postponed on this one, on the assumption this issue will take care of such conversions.
Comment #57
amateescu CreditAttribution: amateescu commentedTo be honest, I think I'd prefer to just repurpose/re-title that one to 'Entity reference autocomplete using routes", because we already have a >100K patch in here, and, even more important, these tasks can be done in parralel, no need for anyone to wait on each other.
Comment #58
amateescu CreditAttribution: amateescu commentedOk, fixed all the problems mentioned by @rootatwc and @Dave Reid:
d7200fa - Fix upgrade path and View UI wizard.
9af70f2 - Added an upgrade path test.
097aa82 - Implemented hook_field_attach_rename_bundle() in ER, simplifying the Vocabulary storage controller.
ae9956b - Implemented hook_field_attach_delete_bundle() in ER, simplifying the Vocabulary storage controller even more.
f36ad4b - Revert some unnecessary (and also incorrect) changes.
ec36f2e - Add back some tests from the taxonomy term reference field.
Also reviewed and corrected everything that seemed suspicious from the changes prior to #43. The relevant tests were added back in
EntityReferenceFieldTest
andEntityReferenceAutocompleteTest
.Comment #60
amateescu CreditAttribution: amateescu commented*sigh*.. random failure from the new ckeditor module.
Comment #62
ParisLiakos CreditAttribution: ParisLiakos commentedUpgrade path looks good, all my term references are still there, even multiple tags..
only problem i can find is when i try to create a new article, i cant specify more than one tag..second tag always overwrite the first:/
Comment #63
amateescu CreditAttribution: amateescu commentedThat's not related to this patch, I opened a bug report for it here: #1914180: Unbreak and cleanup the autocomplete widgets
So.. the patch from #58 is green and in need of reviews :)
Comment #64
ParisLiakos CreditAttribution: ParisLiakos commented#1914180: Unbreak and cleanup the autocomplete widgets fixed it indeed! that was a quick commit;)
had a couple of minutes and took a look again..i am sorry, i cant find time, esp this week:/
UI and update looks good.next time this gets rerolled:
it implements field_attach_delete though
Comment #65
amateescu CreditAttribution: amateescu commentedYeah, that was already fixed in the sandbox :) Here's a new patch with more code removal (yay!).
8dfc0e8 - Remove taxonomy term reference views integration. This will be added to ER in #1906806: Provide a relationship for each entity reference field.
7f347c4 - Fix typo.
2a8ec81 - Remove obsolete TaxonomyTermReferenceItem class.
Comment #66
YesCT CreditAttribution: YesCT commentedberdir suggested this instead of #1120144: Term Reference Autocomplete widget - add option to match only start of term and expose settings (size, match)
Comment #67
BerdirUsual missing \ in various files and should be Contains.
That test class is a bit strange, comment/method is about taxonomy but class name isn't, description says it's about creating reference fields and it doesn't even seem to depend on taxonomy?
Is this a moved taxonomy term reference test?
I also have an issue that introduced a entity test base class that would simplify the dependency handling here.
enableModules() will soon no longer install the tables, you will have to do that yourself.
I'm in the process of replacing test_entity with entity_test, which is already NG. Can we change these tests to use that already?
Given that we will delete the code field type, this seems like a useless assertions, should probably check the field instance information instead.
Needs to use the update save field API functions.
Comment #68
amateescu CreditAttribution: amateescu commentedFixed all the issues raised in the review above. Yes, that test was indeed a moved taxonomy term reference test :)
I spoke to @Berdir on IRC and he agreed to leave EntityReferenceFieldTest as is here and clean it up along with all the other tests that use test_entity in #1822000: Remove Drupal\field_test\Plugin\Entity\Type\TestEntity in favor of EntityTest.
P.S. I couldn't merge latest HEAD in the branch that was previosuly used for this issue, so I pushed everything to a new one, starting with the patch from #65.
Comment #70
amateescu CreditAttribution: amateescu commentedReverted the change that broke all those upgrade tests.
Comment #72
amateescu CreditAttribution: amateescu commentedMerged HEAD.
Comment #74
amateescu CreditAttribution: amateescu commentedThis should do it.
Comment #75
ParisLiakos CreditAttribution: ParisLiakos commentedSame applies for the update case.
Also the introduced dependency of er on taxonomy module should be removed
Comment #76
amateescu CreditAttribution: amateescu commentedYou're right, I don't know what I was thinking when I added that dependecy :/
Comment #77
ParisLiakos CreditAttribution: ParisLiakos commentedawesome, thanks!
I run one more manual upgrade, check, everything works as should and cant find anything to complain about the patch
good job!
Comment #78
amateescu CreditAttribution: amateescu commentedMerged HEAD.
Comment #80
amateescu CreditAttribution: amateescu commentedThe first testbot result was a fluke, we're good to go here :)
Comment #83
ParisLiakos CreditAttribution: ParisLiakos commentedThis patch contains the fix for #1942106: Entity reference and revisions , so i am bumping this to major
Comment #84
BerdirRe-rolled, hope @amateescu wasn't already on it as well.
Not 100% sure about some parts e.g. in rdf.module. Let's see what the tests say.
Comment #86
amateescu CreditAttribution: amateescu commentedNope, I wasn't :) I was planning to reroll tomorrow, but thanks for taking it on. Can you please use the branch for this? http://drupalcode.org/sandbox/yched/1736366.git/shortlog/refs/heads/1847...
Comment #87
BerdirLet's see if that fixes the tests.
Weird that insert and update in forum.module are implemented completely different, wondering if there's a reason for that.
@amateescu: Oh, wasn't aware of that branch and did it using rebase, so can't push to that anymore. Pushed it to http://drupalcode.org/sandbox/yched/1736366.git/shortlog/refs/heads/1847... now.
Comment #88
YesCT CreditAttribution: YesCT commentedI started this earlier today, and since then some patches have been posted. So some of this might be addressed already.
should be public function
http://drupal.org/node/325974
should be one line,
and consistant in terms of Test ... or Tests.
See: http://drupal.org/node/1869794#comment-7173722
Tests ... would be consistant with 1354.
I'm not sure what the pattern is for this one line. http://drupal.org/node/1869794#comment-7173778
should be (optional)
http://drupal.org/node/1354#param
also needs to wrap at 80 chars.
Comment #92
BerdirPushed a number of fixes for the tests to my branch, TermTest still partially fails, something to do with preview.
Comment #93
BerdirOk, so I had to add a separate item class for entity_reference fields as the core doesn't provide everything that it needs: revision_id, label (for auto-create) and access.
No interdiff because I merged in 8.x instead of rebase but you can see the comments here: http://drupalcode.org/sandbox/yched/1736366.git/shortlog/refs/heads/1847...
Comment #94
amateescu CreditAttribution: amateescu commentedReviewed the changes and although I was a bit sad that we have to extend the core ER item class, I got over it quickly for the sake of progress :)
This was RTBC before the Node NG conversion and subsequent patch were mostly rerolls.
Comment #95
YesCT CreditAttribution: YesCT commentedThis was already rtbc, and I would have not mentioned the nit, if I had not found something that needed to be fixed otherwise.
I was looking to see if the things I mentioned before were addresses, not reading the code to see if it makes sense.
==============
1. (nit) 80 char limit on comment (not blocking)
wrap at 80 chars.
"Lines containing comments (including docblocks) must wrap as close to 80 characters as possible without going over, "
http://drupal.org/node/1354#drupal
2. this is blocking commit I think
left over merge conflict notes.
3. question about @see
I looked for this function, but didn't find it.
I looked in this patch, and also in a recent pull on core: $ grep -R " entity_reference_autocomplete_tags(" *
Where is it? Maybe it's referring to a plugin definition in a comment (annotation?) or a method which is camelCase now?
4. css changes. (just a question, not blocking anything)
the css changes (there are a bunch), look like the css was just for taxonomy term references, but now will effect all entity references.
Is there a case we can look at to see what a reference looked like before that was not a taxonomy term reference, and what it looks like now?
Is there a way to refer to just a taxonomy kind of entity reference in the css?
Do we want these to be general or just taxonomy entity references?
5. Issue summary
Updating the issue summary with the issue summary template might make committing this easier.
I'm thinking the sections on API changes and UI changes (might just be clarifying that there are none) would be helpful.
Also, maybe a commit credit recommendation since it looks like some work was done in a sandbox.?
========
patch coming right away.
Comment #96
YesCT CreditAttribution: YesCT commentedmaybe I look for a plugin like..
* @Plugin(
* id = "entity_reference_autocomplete_tags",
I'm going to have to learn how to do multi line grep like things.
$ grep -R 'id = "entity_reference_autocomplete_tags",' *
core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/field/widget/AutocompleteTagsWidget.php: * id = "entity_reference_autocomplete_tags",
So, how does the api autogeneration of docs work with plugin definitions? Is @see whatever() still the right syntax?
Comment #97
YesCT CreditAttribution: YesCT commentedThis addresses 1, 2, 3 from #95
Comment #98
YesCT CreditAttribution: YesCT commentedI looked at http://drupal.org/node/1354#see
Since it didn't say anything about a plugin id, I went with the class name.
And rootatwc and swentel helped me sort out the @see part.
Comment #99
BerdirThanks for the re-roll. The taxonomy_field_presave function should be completely removed, there should be no remaining taxonomy_field_* functions.
Comment #101
YesCT CreditAttribution: YesCT commented@Berdir is that removal of that function something I should do right now? or separate follow-up issue? [edit: doing it. I think it should be done here.]
(I canceled the testbot, because I found a small whitespace thing in the patch. new one coming right away)
Comment #102
BerdirNow, removing them is the point of this issue :) Not sure what happened there with my re-roll.
Also, if we're being nitpicky, i think the second line of the @todo that you changed should be indented 2 spaces.
Thanks!
Comment #103
YesCT CreditAttribution: YesCT commentedyep. :)
This fixes the indent of the @todo lines this patch changes (which was a nit, we could have let slide) and removes the function (which needed to be done).
Comment #104
YesCT CreditAttribution: YesCT commented@Berdir what about these:
$ grep -R "function taxonomy_field_" * | grep -v patch | grep -v "orig:" | grep -v interdiff
core/modules/taxonomy/taxonomy.install:function taxonomy_field_schema($field) {
core/modules/taxonomy/taxonomy.module:function taxonomy_field_extra_fields() {
[edit: I'm investigating]
Comment #105
YesCT CreditAttribution: YesCT commentedI got some help from @dawehner and @yched in irc.
Not 100% sure. But here is my thinking.
There are taxonomy terms. Those have fields. This issue is not about removing fields on terms.
core/modules/taxonomy/taxonomy.module:function taxonomy_field_extra_fields() {
is about the fields on the terms, so that is staying in. [edit: corrected typo]
core/modules/taxonomy/taxonomy.install:function taxonomy_field_schema($field) {
is about adding a field to other things to reference the tid of terms, that is coming out.
Comment #106
ParisLiakos CreditAttribution: ParisLiakos commentedgood catch, hook_field_schema implementation should go
Comment #107
David_Rothstein CreditAttribution: David_Rothstein commentedThose look like good questions. Seems like this patch will make #1218640: Bartik displays all taxonomy term fields in a smaller font than other fields worse (will make it occur for all entity reference fields) and also will result in taxonomy terms not getting any special classes in the CSS which allow themers to distinguish them from other entity reference fields?
***
Also, has there been any discussion of the user interface changes here? It got hard enough to add simple taxonomy fields to content types going from Drupal 6 to 7 :) But now with this patch, in Drupal 8 you won't even be able to select "term reference" in the dropdown on the Manage Fields page in the Field UI (but rather will need to figure out to select something labeled "entity reference" instead and configure it further from there)?
Comment #108
BerdirThe original issue was reviewed by the UX team and there is also #1847590: Fix UX of entity reference field type selection.
Comment #109
amateescu CreditAttribution: amateescu commentedI looked a bit into the css class issue and I found that the field subtype concept (basically, plugin derivatives after fields are converted to plugins) would help a lot for this as well.
In the meantime, I think the best we can do is something like this interdiff.
Comment #110
YesCT CreditAttribution: YesCT commentedfrom looking at the code, the css changes will now keep only effecting taxonomy terms.
if we need a follow-up, we can make one, to discuss if other references should be similarly styled and if taxonomy terms should also have a common entity reference class in common with the other types of reference.
Has anyone manually tested this? tagging for that.
I'm guessing that we want before and after screenshots to show no visual difference.
Comment #111
YesCT CreditAttribution: YesCT commented@amateescu is there an issue regarding field subtype concept? I did a search but didn't see one.
Comment #112
amateescu CreditAttribution: amateescu commented@YesCT, I'm pretty sure @rootatwc did a lot of manual testing on this, but I'll let him confirm. And for the subtype issue, it's the one linked by @Berdir in #108.
Comment #114
ParisLiakos CreditAttribution: ParisLiakos commentedYeah i have manually tested twice..seems css changes have some problems, see screenshots
Before
After
Comment #116
amateescu CreditAttribution: amateescu commentedYeah, I forgot to add 'field-type-' in front the class name, my bad :/ Also fixed those exceptions.
Comment #117
ParisLiakos CreditAttribution: ParisLiakos commentedexactly the same now, thanks!
Comment #118
xjmThanks @rootatwc @Amitaibu et al. We tested this patch live this morning with @Dries, @webchick, and a couple others.
We have a few concerns about the usability. Most of these things aren't specific to the taxonomy implementation; they're existing patterns in entity reference or the Field UI. However, this patch would take the new entity reference interactions from being a "10%" site building task to a "90%" site building task--most site builders are going to have to go through this interaction at some point. (I am the taxonomy component maintainer and it still confused me.) ;) So, tagging for usability review. We should be able to create some separate issues to fix some of the entity reference usability issues, but we need to be careful about adding this change.
I'll also follow up with some screenshots and specific things we found confusing, and also review the patch some more. Thanks for keeping this issue going! It will be great to standardize on a single API.
Comment #119
xjmAlright, here's some screenshots illustrating the UX issues we encountered when testing this. To reiterate, we realize these issues are mostly a result of the default behaviors of entity reference fields or the field API in general, but this patch still represents a usability regression from D7 taxonomy, which is why we need to solve at least some of this if we're going to replace the legacy term reference field type.
Note: This is written from the perspective of a reasonably experienced Drupal site builder, but a lot of these things actually confused me (the component maintainer for taxonomy!) as well as the two core maintainers who were watching me test it. ;)
I've already created my vocabulary and added terms to it, because that's what I've done first ever since Drupal 4.7. Now I want to add it to my posts.
We really shouldn't be exposing the word "entity" to users, the same way we don't expose the word "node".
Even once I figure out to select entity reference (spoiler alert: I read the issue title), it's still not obvious how to make it a term reference field. The maximum value option is first, and most of the time I really don't care what that is. It turns out the "Target type" field is what I actually want, but I have to click on the dropdown to figure that out, and it's the last thing on the form even though it's the most important.
The options for maximum value are weird.
I try "more" to see WTF it does, and wonder why we couldn't just use this widget in the first place. Skip the dropdown, let me just type in a number, and instead have a toggle for "unlimited values" that hides the number widget.
Only after all that, when I click on "Target type" to see what it is, do I finally get the option to make this a taxonomy reference field. (Also, why aren't the options in alphabetical order?)
The next screen is overwhelming (which has always been a problem with the field UI), but it used to be that I could just scroll past it all and click save.
When I scroll past everything and click save like I'm used to doing, something funny happens. Oops!
So I go back to editing the field and realize my mistake. All the vocabularies were checked by default under that "Reference whatever" fieldset I didn't know what to do with. That seems like an unusual default--usually, I just want to create a field to reference one vocabulary, or one kind of node, or whatever. Referencing multiple bundles is a nice feature, but definitely not the 80% usecase.
Also, when I tried to add an autocomplete widget, I was confronted by these two unfamiliar options. I had no idea what the difference between "Autocomplete (tags style)" and "Autocomplete" was. (Me, IRL, and I've even used the D7 entity reference contributed module. At first I thought it might have to do with whether terms were autocreated, because that's what "tagging" meant for vocabularies in D6, and I never did figure out what it actually meant until moshe weitzman and effulgentsia explained it to me like ten minutes later.)
Comment #120
amateescu CreditAttribution: amateescu commented1) We could rename to just "Reference" mayhaps?
2) I think I tried to put the 'Target type' on top but I couldn't because of a Field API limitation IIRC.
3) and 4) are totally unrelated, I don't see why they were included in this review?
For 5) we could expose a radio widget instead of that select.
For all the others, suggestions are always welcome :)
Comment #121
Dave ReidCould we for ease of use, continue to have a separate field type 'term_reference' that is owned by taxonomy module, but extends entityreference in the same way that image field 'extends' the file field? That way it's more clear to site builders. And we can technically do some different UI things to make this process easier since they are separate fields, but not really?
Comment #123
amateescu CreditAttribution: amateescu commentedRe #121: I think that would be a huge step in the wrong direction. Why would only taxonomy entities receive special treatement and UI improvements and not all referencable entity types?
Comment #124
Dave ReidOk, so just not taxonomy terms, but if field types are or will be plugins, they should have automatic derivative plugins for each referencable entity type... It would be nice to never even see the word 'Entity reference' at all. Because entity reference fields cannot reference more than one entity type in it's configuration, why don't we just list every referencable type as a field type? I would make a content reference field instead of entity reference. That also make way more sense to me in a site builder role.
Comment #125
amateescu CreditAttribution: amateescu commentedThat's why we have this issue open :) #1847590: Fix UX of entity reference field type selection
Comment #126
xjmI think that's a good first step until #1847590: Fix UX of entity reference field type selection is resolved. (That issue could use a clearer summary and/or title.)
Because I just made a list of everything that came up during our testing, as a hypothetical site builder used to D7, and it was part of the overall WTF. :) As I stated, we acknowledge that's a problem in the field API itself. @swentel found #680546: Add a custom 'number of values' for 'multiple / multi-value' field. (e.g. nodereference, imagefield) for me and we can address that there.
Our current interface guidelines always use single-select boxes instead of radios. I'd prefer to give the field a more meaninful label instead, and eventually move this option up a step in #1847590: Fix UX of entity reference field type selection.
Comment #127
ParisLiakos CreditAttribution: ParisLiakos commented7,8) would be easy to fix..dont have anything checked as default, then checkboxes are required and if user misses it, form wont submit
Comment #128
xjmOh, one other thing:
Worst case, we form alter the darn thing until #1847590: Fix UX of entity reference field type selection is resolved?
Comment #129
xjmMmm, in general, that's not good UX. What should actually happen (ignoring current field API and entity reference limitations for the moment) is that the bundle options only should be at the very top of the form, after the label, or even on the previous screen. (Yes, I know it's on this screen b/c of field settings vs. instance settings.) :)
Edit: See #1880168: Introduce top-level sections for all forms.
Comment #130
amateescu CreditAttribution: amateescu commentedRe #126, and #128:
That issue is only about discussing subfields/field derivates/whatever, I don't think fixing Field UI to allow form elements to sit above the maximum value option would be in scope there.
Re #129:
That issue you linked there states this as the proposal: "Simplify forms used in contextual administration". I don't see how the field instance form is 'contextual administration'..
That being said, I guess @rootatwc's proposal is the easiest way out for now. We don't like that all bundles are checked by default and we don't want users to skip that very important form element? Make it required :)
I'm really sad that Entity reference has become the dumping ground of everyone's frustrations with Field UI. I've seen a lot of reviews and comments that actually bash Field UI, but not *one* issue opened. Why is that? We have a field_ui component afaik, can we please try to fix it there instead of putting the burden on ER?
Comment #131
amateescu CreditAttribution: amateescu commentedComment #132
yched CreditAttribution: yched commentedOne possible track for breaking out of the constraints of the current shape of the "select field type" dropdown in Field UI's "add new field" UI could be to remove those "add new field" / "add existing field" as rows in the "Manage Fields" table, and move them to action buttons at the top of the page
Those would lead to separate form steps where the selection of the field type would have more screen real estate available to explore more suitable UX formulations.
Comment #133
BerdirNote: 3. is being handled in #680546: Add a custom 'number of values' for 'multiple / multi-value' field. (e.g. nodereference, imagefield) and does not overlap at all with this issue, it's the same behavior for all field types. If that is important to be fixed, raise the priority there :) This does not in any way block this issue, it's the exact same behavior for all field types.Edit: Just noticed that this was already in #126. I think what we need at this point is an updated summarized response to the points raised in the in the review so that we know what already was addressed, what will be addressed where and possibly some additional follow-up issues. I don't really see any remaining commit blockers..
@yched: Yes, I think that would make sense and there are issues for that I think. There are also accessibility problems with the current solution.
Comment #135
YesCT CreditAttribution: YesCT commentedtagging for issue summary, especially a remaining tasks section, and updating the follow-up issue section.
Comment #136
BerdirI've noticed in another issue that this wouldn't be necessary if we also fix field_data_type_info() to use the data_type key.
Comment #137
amateescu CreditAttribution: amateescu commentedLike this?
Comment #138
xjm#132 sounds like a great thing to explore. I re-scoped #1847590: Fix UX of entity reference field type selection; let's discuss that there.
It's a similar problem space, is my point. The pattern there is also one of the patterns we could use to fix the ER fields settings page.
Comment #139
xjmMeanwhile, let's:
Comment #140
xjmActually, to keep this patch from getting monstrous, let's spin these generic ER fixes off into their own issues.
Comment #141
xjmFiled #1953568: Make the bundle settings for entity reference fields more visible..
Comment #142
yched CreditAttribution: yched commentedFWIW, I have no issues with patching Field UI to display:
- first, settings specific to the field type,
- then, generic settings present for all field types (i.e "number of values")
instead of the other way around right now. I'd just prefer it to be consistent across all field types, not just special cased / form_altered for entity_reference fields.
Comment #143
amateescu CreditAttribution: amateescu commentedFiled #1953770: Move the field-specific settings form elements at the top of the form.
Comment #144
xjmOther issues:
#1953444: Make 'target bundles' required on the Entity reference instance settings form
#1953438: Don't expose the term 'Entity' to users
#1953832: Replace 'target type' on entity reference field settings with something clearer
Comment #144.0
xjmA proper issue summary
Comment #145
David_Rothstein CreditAttribution: David_Rothstein commentedLike I said in #107 the removal of "term reference" from the initial dropdown really seems like the biggest issue here (especially since the others all have nice followups). It's the one issue that's directly caused by this patch, and if no one can figure out how to get past the first screen they won't even have an opportunity to flounder on the next ones :)
I'm probably going to get laughed at for this, but here's one suggestion for how to fix it (see attached). Maybe #1847590: Fix UX of entity reference field type selection will eventually come along with something better, but I think this brings the patch here towards a point where it could be committed.
Comment #146
David_Rothstein CreditAttribution: David_Rothstein commentedI also found it interesting that the Taxonomy module does not have a dependence on Entity Reference after this patch. Technically that might be true, but how useful is the module without it? What will it mean for someone trying to turn on basic taxonomy functionality from the modules page?
It also means that taxonomy_help() is left completely incorrect after this patch (actually, it might be a bit incorrect either way, but it's very incorrect if there is no dependency on Entity Reference)....
Comment #147
David_Rothstein CreditAttribution: David_Rothstein commentedAdding on to my not-a-patch, here's another related improvement (this one for the second screen in the field UI). The module already has some logic to try to guess the most likely target type (and pre-fill it in the dropdown), and since everyone seems to agree taxonomy terms are the most likely.... why not just use that?
The second screen then looks like this as soon it loads:
Comment #148
yoroy CreditAttribution: yoroy commentedThe type has to come before the number of values though.
& I just posted https://drupal.org/node/1847590#comment-7230112 with an idea for how to introduce the References concept in the first place
Comment #149
xjmAgreed. This is now being fixed in #1953770: Move the field-specific settings form elements at the top of the form.
Comment #152
netsensei CreditAttribution: netsensei commentedHm. Does this have to do with this patch?
Comment #153
amateescu CreditAttribution: amateescu commentedThat was fixed in #1956434: Can't edit nodes with entity reference fields.
Comment #154
jenlamptonI think the suggestions here will help mitigate the small UX regression from D7's taxonomy term reference field, but I'm still worried about the huge UX regression that was introduced in D7 when we started requiring people create and customize fields for taxonomy in the first place.
What we need to get back to is a checkbox on the entity we are referencing that automatically creates (and customizes) a field based on the settings of the entity. For example: if we define a vocabulary as being free tagging, the term reference field widget should be autocomplete. Having site builders specify the settings once on the vocabulary and then again on the field is unnecessary.
Would it be a lot of work to put back the UI from Drupal 6?
Ideally, all a site builder should need to do is check a box to make the entity they just configured have a field on another entity.
Comment #155
tstoecklerSuch a UI in D8 (or even D7) would have to account for all bundles of all entity types and, thus, can quickly get overwhelming. Entity translation contains a page where you can enable translation settings for all entites, bundles and fields and it took a lot of time to get it right. Conceptually we should stick with fields, but if we are sure that #154 is the way to go it would definitely be doable.
Comment #156
YesCT CreditAttribution: YesCT commentedNo one laughed @David_Rothstein
Is a way forward here to use that patch #145 and #147 ?
I think we also here need to fix the dependency and the help.
Make #154 a follow-up (issue needs to be created. is there one left over from D7.. a regression or something?)
We need to identify which of the related issues listed in the summary needs to get fixed before this, which are *needed* follow-ups and which are just... related.
And then let #1847590: Fix UX of entity reference field type selection fix it further.
Comment #156.1
YesCT CreditAttribution: YesCT commentedtrying to identify next steps
Comment #157
YesCT CreditAttribution: YesCT commentedhttp://drupal.org/patch/reroll
other changes should be done *after* a reroll patch is posted so that we can get a meaningful interdiff of them. (http://drupal.org/documentation/git/interdiff)
Comment #158
amateescu CreditAttribution: amateescu commentedThat would be pointless until #1818560: Convert taxonomy entities to the new Entity Field API is in.
Comment #158.0
amateescu CreditAttribution: amateescu commentedadding reroll to remaining tasks with link to instructions
Comment #158.1
YesCT CreditAttribution: YesCT commentedadded info about what this issue is postponed on
Comment #159
xjmFor #154, there are a couple possibilities:
Re: #154, we needn't allow all entity types on the content creation form. Since taxonomy is the 80%+ usecase, I think it would be feasible to special-case it on the content type creation form, and then let the field UI present the generic ER. Let's discuss whether or not we want to do that in #394482: Users should be able to enable a vocabularly from the content type configuration page. It could provide a workflow similar to what I describe in #889378: Create taxonomy fields from the taxonomy administration page.
Comment #159.0
xjmadded the autocomplete starts with setting issue to related issues
Comment #160
xjmFiled #1963340: Change field UI so that adding a field is a separate task.
Comment #160.0
xjmUpdated issue summary.
Comment #161
BerdirRemoving sprint tag as it's no longer a blocker for my term NG conversion issue.
Comment #164
PanchoIn #133, @Berdir asked for an updated summarized response to the points raised in the large UX review (#119). So just to keep track:
#1 & 5: being discussed in #1847590: Fix UX of entity reference field type selection, so not our business here.
#2: 'target type' renamed in #1953832: Replace 'target type' on entity reference field settings with something clearer, will be refactored in #1847590: Fix UX of entity reference field type selection, but for now we might want to form alter it.
#3 & 4: fixed, see #680546-135: Add a custom 'number of values' for 'multiple / multi-value' field. (e.g. nodereference, imagefield). Might be further generalized in #1207060: Interface pattern for custom option as alternative to predefined choices, so not our business.
#6 through #9: @TODO
[edit:] changed status because #1818560: Convert taxonomy entities to the new Entity Field API is in now.
Comment #165
PanchoSorry for cross-posting, but:
Now that we have a form display tab, the handles for moving fields around were removed, so new fields can't be added in position anyway. This means there is no more reason to stick with the current layout with its space restrictions.
Comment #166
xjmThis isn't really a field system issue; it's a taxonomy one with many related field-system issues. :)
Comment #167
amateescu CreditAttribution: amateescu commented@Pancho, thanks for the updated summary.
Part of #6, #7 and #8 were fixed in #1953444: Make 'target bundles' required on the Entity reference instance settings form, so that only leaves us with the first part of #6 and #9.
Also, I've no idea why this is still assigned to me..
Comment #167.0
amateescu CreditAttribution: amateescu commentedUpdated issue summary.
Comment #168
amitaibuWho said the enemy of good is perfect?
So... let's try to revive this issue. Entity reference is such a powerful module and yet it's pretty hidden.
Personally, and as much as I'm concerned about UX, I believe we should get it in as soon as possible and then figure out the more complex UX stuff. I'm willing to put some effort back in this issue, but let's have a better understanding of what are the expected deliverables.
Comment #169
Bojhan CreditAttribution: Bojhan commentedNo idea what to review here
Comment #170
amitaibu> No idea what to review here
Currently nothing the review. I guess my biggest question is if this issue has a chance to get it in - As one can image
git merge
doesn't quite do the job :/Comment #171
webchickI'd totally still love this to get in. But we definitely need to resolve the UX issues. And it's been so long now that I don't remember what they were. Is it possible to get this patch re-rolled on top of HEAD and we could discuss it @ DrupalCon?
Comment #172
kbell CreditAttribution: kbell commentedI also support getting this in, if possible, but the remaining UX issues are definitely important to solve. @webchick, the UX issues were posted by xjm and are listed above: https://drupal.org/node/1847596#comment-7211430. As @amateescu says, the first part of xjm's #6 and all of #9 are what remain to be resolved. I hope this helps a little.
Comment #172.0
kbell CreditAttribution: kbell commentedno longer postponed on #1818560
Comment #173
MixologicWithout that, deprecating the taxonomy reference would be a functional regression. The autocomplete tagging variant of a taxonomy reference allows for creating taxonomy terms if none currently exist in the vocabulary.
Comment #174
amateescu CreditAttribution: amateescu commentedAnother week, another patch from scratch :)
I re-read the entire issue and it seems that there are only two major UX regressions left:
1) losing the "term reference" option from the initial dropdown (as @David_Rothstein mentions in #145)
2) the fact that selecting the target entity type is not part of the initial screen (making it harder to know that one can select taxonomy terms as a reference type)
And I also have two proposals for fixing them:
1) when #2349991: Provide a trait for categorizing plugin managers and use it for conditions and actions gets in, implement it for field type plugins too and (borrowing the wording from David's suggestion) introduce option groups in the field type selector, something like this:
2) finish #1963340: Change field UI so that adding a field is a separate task and, either in the same issue or a followup, bring the field settings form in the new "field add" form via AJAX, thus allowing users to easily see and select the target entity type in the first step
@Mixologic, entity reference has the "autocreate" feature already, it just needs a bit of love in #2370703: ER's "autocreate" feature is mostly broken (and untested).
The patch attached will fail until the autocreate issue is fixed but setting to NR anyway to get some feedback on the two proposals above.
Comment #176
effulgentsia CreditAttribution: effulgentsia commentedThis patch could potentially be both high-impact and high-disruption (https://www.drupal.org/contribute/core/beta-changes), but potentially more disruptive than impactful, so before investing too much time into it, I suggest getting a committer to comment on whether it's still in scope. Also, it will be much more disruptive to do after supporting a beta->beta upgrade path, so tagging accordingly.
Comment #177
webchickWe need "Needs issue summary update" addressed if we "Needs committer feedback."
Comment #178
amateescu CreditAttribution: amateescu commentedUpdated the issue summary and also posting a patch combined with #2370703: ER's "autocreate" feature is mostly broken (and untested) to prove that everything works.
Comment #179
yched CreditAttribution: yched commentedSide note :
I was a bit worried by the fairly long text for the 'references' optgroup, making the select eat up more screen real estate for the "Field Type" column in the Field Overview table.
But with #1963340: Change field UI so that adding a field is a separate task, that select would be in a separate page, not in the Field Overview table, so that's fine.
Comment #180
effulgentsia CreditAttribution: effulgentsia commentedThe issue summary changes from #178 look good, but given the nature of this issue, I think the summary also needs a "beta phase evaluation" section similar to the one in #1963340: Change field UI so that adding a field is a separate task. I don't think I understand all of the ramifications of this issue enough to write that, so in the meantime, here's some initial thoughts:
Reasons for doing this before 8.0.0's release:
taxonomy_term_reference
field type or using theentity_reference
field type. There's no meaningful difference in terms of content modeling between the two: either choice implements the exact same concept. But depending on which you choose, you'll get different formatters and widgets (and any other code that acts on fields in a type-dependent way) to choose from.Reasons for postponing this (probably to D9, unless someone can suggest a way to do this in a BC-compatible way while still achieving any of the significant benefits):
Any thoughts/disagreements/additions to above?
Comment #181
rickvug CreditAttribution: rickvug commentedI really like the list in #184. I noticed that the patch removes a lot of code. I would argue that the change makes D8 more maintainable, not to mention more logical and cleaner.
Comment #182
Crell CreditAttribution: Crell commented#180 seems like a fair assessment of the situation.
I would favor moving forward, myself. An API change now would affect a relatively small number of people. (There's a tiny number of Drupal 8 sites in the wild, relatively few D8 modules and I doubt many of them are messing with taxonomy terms, etc.) However, the decision we make here is one that site builders will be living with for 4-6 years. Let's have a little pain for a few people now to save a lot of ongoing pain for a lot of people later.
Comment #183
amateescu CreditAttribution: amateescu commentedThanks @effulgentsia, those are all excellent points! I agree that we need a "beta phase evaluation" section but I'm not sure how much I can help with it since I'm obviously biased :)
I'd like to reply to a couple of those "con" items:
While this is true, we also have to keep in mind that *a lot* of work was done in the unfication of the underlying layers for all reference-like field types (e.g. they all use the 'target_id' property instead of wild west that was before; they share a lot of code by inheriting from a single EntityReferenceItem base class, etc.). So, the actual API changes here are pretty minor:
WidgetInterface/FormatterInterface::isApplicable()
API and check for the 'entity_reference' field type with a'target_type' => 'taxonomy_term'
field storage settingOr they can just assign an issue to me and I'll take care of it :)
Previous versions of this patch already have that hook_update_N() implementation so it would be (somewhat) trivial to bring it back and adapt it to the current core APIs.
Comment #184
Wim Leers+1
As a former maintainer of a Taxonomy Term reference widget: anything that brings more consistency and reduces fragmentation is extremely welcome. I'd have given a lot for even a tiny bit less fragility/fragmentation.
I think the "data model change" (the first con in #180) which then affects existing D8 sites (the third con in #180) are one and the same, and is really the only con we should take into consideration. How difficult would it be to provide such an upgrade path?
Comment #185
amateescu CreditAttribution: amateescu commentedErr.. see the last paragraph from the comment above? :)
Comment #186
Wim LeersOops, sorry! In that case: effulgentsia et al., assuming
hook_update_N()
is present, does that then mean your concerns are fully addressed?Comment #187
amateescu CreditAttribution: amateescu commentedHere's the upgrade path if we decide to include it.
Comment #188
jibranWe can use
FieldStorageConfig::loadByName()
andFieldConfig::loadByName()
here.We also have to update
forum_install()
.Comment #189
amateescu CreditAttribution: amateescu commentedNope,
loadByName()
is a single-load operation whileloadByProperties()
is multiple-load.That's already handled in the patch from #178?
I went ahead and opened #2373491: Categorize field type plugins since it makes sense to group field types in the UI regardless of the outcome of this issue. Right now it (mostly) implements the groups shown in the image from the issue summary.
Comment #190
jibranThanks for explaining that and sorry for the useless review :$ @jibran--.
I have one question if we are going to add
hook_update_N()
do we also have to add the tests for the hook?Comment #191
jibranAlso tagging VDC for the changes in views plugin files.
Comment #192
amateescu CreditAttribution: amateescu commentedI doubt that we'll need automated tests for it, maybe just some minimal manual testing that anyone with a trashable D8 site can do.
In the meantime, the blocking entity reference issue landed so here's a reroll (not including the upgrade path for now).
Comment #193
jibranI went through all the patch. It looks good to me just some minor points.
Instead of
(1)
can we useterm->id()
here.Why can't we use
($field->getType() == 'entity_reference' && $field->getSettings('target_type') == 'taxonomy_term')
?Comment #194
effulgentsia CreditAttribution: effulgentsia commentedWow! So, the upgrade path in #187 worked surprisingly well:
- I first installed HEAD, Standard profile, and created an article node with two tags.
- I then applied just the patch in #187 and ran update.php.
- I then applied #192, and viewing the article, with the Tags field converted, worked.
I was surprised by this, because I expected some schema change to be needed to the
{node__field_tags}
table, and didn't expect that to happen smoothly if there was data already in there. But, turns out, no schema change was needed, sinceTaxonomyTermReferenceItem::schema()
andEntityReferenceItem::schema()
define nearly the same schema. They differ onNOT NULL
constraint, and the update process didn't update that accordingly, which the entity storage/schema system should have performed automatically, but that's likely a separate bug we need to fix anyway.TaxonomyTermReferenceItem::schema()
also defines a foreign key to{taxonomy_term_data}
, and I don't know if we want/need to preserve that when using an EntityReference to taxonomy terms.Note that the order above is odd. It should be possible to combine the update function with the rest of the patch and have it work. But it doesn't, because update.php fatals due to
field_system_info_alter()
kicking off aentity_load_multiple_by_properties()
which fatals when loading the definition of the not-yet-updated taxonomy_reference field whose code is already removed by the patch. But, I think we can find a way to fix that.Given the
::schema()
compatibility mentioned above, I feel that this is an acceptable data model change to introduce after beta, especially if we provide a working upgrade path for the configuration change, per #187, and get it to work when included with the rest of the patch.Not my call though, so leaving the "Needs committer feedback" tag to get an official decision.
Comment #195
Dries CreditAttribution: Dries commentedI'm supportive of removing taxonomy term reference field in favor of entity references. I believe the benefits outweigh the disruption, assuming this patch does not introduce UX regressions. I also agree that this issue is major (not critical). The deadline to get it done will be supporting a beta-to-beta upgrade path.
Comment #196
amateescu CreditAttribution: amateescu commented@effulgentsia, thank you so much for helping out with this issue! And @Dries for approving it :)
Regarding the upgrade problem described in #194, I think we can implement a nice workaround by keeping a dummy implementation of TaxonomyTermReferenceItem with
no_ui => TRUE
and which throws an exception in the constructor. This way, we're sure that nobody can use it anymore while still allowing existing D8 sites to run the update function.Rerolled patch attached which includes the upgrade path from #187 and the dummy class mentioned above.
In the meantime, I've been hard at work on the UX dependencies listed in the issue summary (#1963340: Change field UI so that adding a field is a separate task is getting pretty close) but I think we need a new round of testing here and, more important, a validation of the current plan for fixing the UX problems introduced by this patch.
I also identified another regression that hasn't been mentioned in this thread so far: the fact that entity reference uses a
entity label (entity_id)
syntax in its autocomplete widgets, which will be very confusing for anyone familiar with a tagging system but not so familiar with the underlying structure in Drupal (e.g. "what's this weird number that appears after every tag?! But not when I enter a new tag. WTF?" :D).But fear not, we have a solution for that as well: since the current autocomplete implementation doesn't allow us to improve that aspect of entity reference, we need to switch (again) to a better one, notably #2346973-37: Improve usability, accessibility, and scalability of long select lists.
And one last thing; even though the list of issues that are "required" in order to solve the UX regression introduced by this patch, please note that each of them are actually quite good usability improvements on their own, that's why I've been working on them even before this patch got the post-beta approval. It just so happens that if we look at them combined, they also fix most of the problems here :)
Comment #197
effulgentsia CreditAttribution: effulgentsia commentedAdding the issue links from #196 as related issues.
Comment #199
amateescu CreditAttribution: amateescu commentedRed patches are depressing, especially when the failures are from a different system that, apparently, is not fully working.
Comment #200
amateescu CreditAttribution: amateescu commented#1963340: Change field UI so that adding a field is a separate task is RTBC so here's a patch rerolled on top of that one. Setting to NW until we can send it to the testbot.
Comment #201
amateescu CreditAttribution: amateescu commentedThat issue is in, go testbot!
Comment #204
jibranAre you planning to address #193?
Comment #205
jhodgdonJust a note: This patch does not update the hook_help() for the Taxonomy module. If you are making changes to a module that affect end users, as this definitely does, you absolutely also need to update the help. Setting to Needs Work until that is done, at least.
Also @ifrik and I have been working on editing the current help for the Taxonomy module, and she noticed several usability regressions in trying to use Entity Reference for taxonomy terms. See discussion at
#2091367-43: Update hook_help for Taxonomy module. I am not sure whether or not she was using this patch or unpatched Core, but probably they all need to be addressed before the taxonomy term reference field should/could be removed:
#2412553: Taxonomy terms in an Entity Reference field are not sorted
#2412557: Difference between "Autocomplete" and "Autocomplete (tags style)" is not entirely clear for site builders
#2412569: Allow setting the auto-create bundle on entity reference fields with multiple target bundles
I haven't added them as related...
Comment #206
webchickI was asked by amateescu to take a look at the current proposal in the issue summary which is:
Basically, address the concern about "Users don't see the keyword 'term' in the list" by including a grouping around entity reference called "References (to content, terms, files, etc.)"
However, there are a couple of problems with this approach:
Looking at the rest of the options presented in that drop-down, my proposal was something like this:
References
- Reference (entity)
- Reference (content)
- Reference (comment)
- Reference (file)
- Reference (image)
- Reference (term)
- Reference (user)
Basically:
"Foreach type of entity that can be referenced, if it's a content entity, display it as an option the list." (and also keep "entity" there as a generic option for other kinds of entities.)
Then, when the user selected, say, "Reference (comment)", comment would already be set in the UI so they wouldn't need to do that part.
I don't think this problem is specific to entity reference fields; I can imagine there are other field types that are a single field but offer different variants, that may want to expose those in the UI for the user to click on.
So (per amateescu) from an API side what we would need is something like "put something in the list which converts to a field type with some default options already set" (a new method on field type plugins). He's going to see what Yves et al thinks.
Comment #207
webchickRan this past Bojhan (yoroy said the same), he was +1 except he suggests instead of duplicating the word "References" everywhere, just do:
That would also make lists, for example:
...which I actually like way better, was just not sure how much was reasonable to do.
Comment #208
Bojhan CreditAttribution: Bojhan commentedThis sounds sensible, to most users loading all meaning into the most abstract word eva (entity) is going to create a lot of usability issues. If we can reduce that problem by allowing them to choose entities (references) and in case its needed the "overarching" entity reference. Then that would be great.
Lets leave changing the wording of the other items in this element to a followup (lists, text, etc.), in some cases we are leading with the concept rather than the data storage model because it is more accessible for non programmers.
Comment #209
webchickAlso on IRC, Berdir pointed out that a blanket content entity vs. not content entity is probably not sufficient if we're planning to promote these as first-class options. For reference, here's what the list of things that can be referenced currently looks like on an entity reference field:
As you can see, even with just core this list is already starting to be a bit silly with options like "Menu link content" and such. Contrib will only make it worse... he noted "I also see stuff like flagging (not a flag type, but the actual user flagged node X info), log entries, payments, simplenews subscribers, redirects, translation jobs"
So we probably need some additional annotation like
frequent_reference = true
or something to differentiate between what to show in the drop-down and what to not. Content vs. config doesn't seem to be quite granular enough.Comment #210
webchickOk, attempting to upgrade the issue summary with the current proposed resolution, based on today's conversation.
Comment #211
webchickOops. One other detail from IRC from berdir.
Comment #212
yched CreditAttribution: yched commentedPre-packaged field / field storage configurarions is an interesting idea, but this requires some UI care IMO.
I don't think that mixing "actual, different field types" and "preconfigured variants of the same field type" on the same level in a single list would really make that list clearer.
Seems potentially very confusing to me, you don't really know what your actual options are, you don't know that options A and B are completely different, while options A and C are really variations of the same.
Also, we'd need to take the work being done in #2373491: Categorize field type plugins into account ?
Comment #213
amateescu CreditAttribution: amateescu commented@yched, it's true that it requires quite some UI/UX planning, but do you agree that it's a good direction to pursue?
Keeping in mind that my previous proposal (finish #2373491: Categorize field type plugins and #552604: Adding new fields leads to a confusing "Field settings" form) was deemed in #206 as not good enough to fix the UX problem introduced by this patch, I feel that we're running out of options.
@jhodgdon, I looked at the contents of
taxonomy_help()
and it does not mention anything about the taxonomy reference field, so I'm not sure what I can update there..@Berdir pointed out in IRC that we were missing the custom Views filter for ER fields that reference taxonomy terms so I brought it back, and also fixed the review from #193. Note that I left one class check in place because it's useful for any field type that extends entity reference.
Rerolling this was quite painful :/
Comment #214
jhodgdonRE #213, yeah the help is being updated in #2091367: Update hook_help for Taxonomy module. My mistake, it isn't in the Taxonomy module yet. But it will need to be updated again if/when this patch goes through.
Comment #217
amateescu CreditAttribution: amateescu commentedRerolled.
Comment #219
amateescu CreditAttribution: amateescu commentedSome parts of this patch can be offloaded to another issue, now that we have #1959806: Provide a generic 'entity_autocomplete' Form API element. That issue will also fix the failures from #217, so putting back to NR if anyone wants to take a look at the current patch in the meantime.
Comment #220
jhedstromBefore this goes in, I think #2429699: Add Views EntityReference filter to be available for all entity reference fields will need to be addressed, since without that, once taxonomy fields are removed, there is no way to expose a views filter with all terms as checkboxes.
Comment #221
Bojhan CreditAttribution: Bojhan commentedComment #222
xjm@jhedstrom, #2429699: Add Views EntityReference filter to be available for all entity reference fields doesn't seem like a blocker to me. Both Views and ER are contrib modules in D7. And, given the fact that this issue has an upgrade path deadline and significant data model disruption, I'd prefer only to ensure we have the MVP in terms of usability.
Comment #223
BerdirSo, it took a discussion with @xjm in IRC, but I eventually remembered that I already pointed out this problem to @amateescu a while ago, because I noticed it on my site while testing the switch to entity reference fields.
*And he already fixed it*, see comment #213.
The custom views filter is still available, taxonomy.module just makes it available for entity reference fields (pointing to terms) instead of taxonomy term references, so the views integration is identical (only for terms of course, but that is enough to not have a regression here).
Comment #224
jhedstromWhile working on #2429699: Add Views EntityReference filter to be available for all entity reference fields, I realized the part that made a fix for that non-trivial is that entity reference stores bundles/selection plugins with field config, rather than with field storage. Taxonomy, on the other hand, stores bundles (vocabs) with the storage settings, allowing the views filter plugin to readily narrow down the terms to expose.
#213 appears to move bundle/selection config to the field config, and I'm not seeing code that would allow exposed term filters in views.
Comment #225
yched CreditAttribution: yched commentedSide note : #2433513: [PP-1] TaxonomyFormatterBase should extend EntityReferenceFormatterBase
Comment #226
amateescu CreditAttribution: amateescu commented#2428941: Update the Node views wizard to use 'entity_autocomplete' for the "tagged_with" field was the issue that helped us fix the test failures in #217 in a nice way.
@jhedstrom, the code you're looking for is in
Drupal\taxonomy\Plugin\views\argument_default\Tid
. I agree that the status quo is not ideal for every other reference, but this patch does not cause any feature regression for taxonomy references.Comment #227
Wim LeersThat diffstat! OMG that diffstat! <3 <3 <3
This patch looks great. What's left to be done? I only found one problem in the patch:
This still needs to be fixed?
This looks like a simple bugfix. Nice catch.
Comment #228
amateescu CreditAttribution: amateescu commentedFix the UX regression? :) I just opened #2446511: Add a "preconfigured field options" concept in Field UI for that.
#227.1: Not sure, I think we uncovered a bug in Migrate there and, afaik, we don't need to fix migrate bugs in "regular" issues, but comment out the code and open an issue in the migrate queue.
#227.2: A test caught that :P
Comment #229
Wim LeersSo… this basically means that the UX regression must be fixed in #2446511: Add a "preconfigured field options" concept in Field UI, not here? Which means there's nothing left to be done here, except for filing an issue for #227.1?
Comment #230
amateescu CreditAttribution: amateescu commentedI would love if this wouldn't be postponed on the issue that tries to fix the UX regression, but I'm pretty sure no one else will buy that :) We need to have at least some agreement that #2446511: Add a "preconfigured field options" concept in Field UI is the way to go.
Also opened #2446613: Fix MigrateVocabularyFieldTest.
Comment #231
benjy CreditAttribution: benjy commentedThis wasn't my understanding. How I understand it is that if we have a migrate issue that isn't trivial to fix blocking a critical issue then we'd comment out the tests and fix it in a follow-up.
Comment #232
Wim LeersNW just to update the code cited in #227.1 to point to #2446613: Fix MigrateVocabularyFieldTest.
But from what I can tell, @benjy is saying that it's *not* ok to commit this patch first, and *then* do #2446613, because this issue isn't critical. Did I understand that correctly, @benjy?
Comment #233
amateescu CreditAttribution: amateescu commentedI understood the same. So that disposition was only for critical issues, which makes sense :)
I'll try to fix it in the separate issue and bring the fix back here.
Comment #234
Wim LeersOk — then my NW is indeed pointless, no need to update a todo if it needs to be fixed in a child issue first. Let's get #2446613: Fix MigrateVocabularyFieldTest fixed!
Comment #235
Wim LeersActually, this is blocked on #2446511: Add a "preconfigured field options" concept in Field UI also AFAICT, which in turn is blocked on #2373491: Categorize field type plugins, which I just RTBC'd. I might be mistaken though.
Adding this metadata to clarify the issue dependency chain.
Comment #236
amateescu CreditAttribution: amateescu commentedOh, great, you uncovered the issue dependency hell I'm going through to get this patch in. It would've been [PP-10] or so a couple of months ago :P
Comment #237
Wim LeersYes, issue dependency hell it surely is. Which flavor ice cream cone do you prefer down there? :P
Comment #238
catch#2373491: Categorize field type plugins is in.
Comment #239
amateescu CreditAttribution: amateescu commentedUpdated
taxonomy_help()
as requested in #205, since #2091367: Update hook_help for Taxonomy module was committed in the meantime.Comment #240
benjy CreditAttribution: benjy commented@Wim, yeah for sure. I'm happy to help with any migrate test failures. I don't want to start a precedent of just commenting out migrate tests when any major issues breaks migrate but i'm more than happy to work immediately on fixing migrate issues that block critical issues or create follow-ups in the case if it's non-trivial.
Comment #241
amateescu CreditAttribution: amateescu commentedThe migrate stuff was fixed and reviewed in #2446613: Fix MigrateVocabularyFieldTest, so we're down to just one blocker now.
Comment #242
jhodgdonI took a look at the taxonomy_help() in taxonomy.module portion of the patch.
I also tried to test the patch out on simplytest.me but I got some kind of error that makes me think this needs a reroll.
Anyway, that aside I have some suggestions on the help:
a) In the "Classifying entity content" item, we should at least mention that you have to use an Entity Reference field. The help previously listed the two types, and that was taken out, but I think it would be useful to mention the name of the field type, because it might not be obvious to everyone what type of field you'd need to add (which is why we put things in Help).
Specifically, I'd suggest changing "...for a certain entity type may add reference fields to the entity..." to
"...for a certain entity type may add <em>Entity reference
fields to the entity...".b) In the "Adding new terms during content creation" section, I suggest changing
"...is chosen for an <em>Entity reference</em> field. "
to "... is chosen for the Entity reference field" (no EM tag, and "an" becomes "the" since now there is no choice on the field type).Also, it looks like in the Entity Reference field, you need to have the " Create referenced entities if they don't already exist" checkbox checked in the field settings, in order for this to work? I don't actually know if that is true or not, but if it isn't true, I have no idea what that setting would be used for? If it is true, let's add a note to the documentation. If it isn't true, let's remove the setting?
c) In "Configuring displays and form displays" it still talks about the "reference fields" but there is now just one reference field type, so this should be singular. Extra credit if you can remove the extra space in the item header where it says 'Configuring displays and form displays ' [remove that last space]
d) In that same section, in the two Autocomplete items, you can take out where it says "of an Entity Reference field".
e) In that same section, the list of formatters in the docs do not match what I see in the UI. The formatters that are actually available are:
- Label
- Entity ID
- Rendered entity
- RSS category
So the help needs to be adjusted for this. The list that is there is what you'd see, I believe, on a taxonomy term field, not for an Entity Reference field.
Comment #243
amateescu CreditAttribution: amateescu commentedRegarding the help text, #2446511: Add a "preconfigured field options" concept in Field UI might change this area significantly because the 'Entity reference' option could not even appear anymore in the field type selection, so it's probably more useful at this stage to review that patch instead of this one :)
Comment #244
jhodgdonIs one of those issues postponed on the other one? It seems like each issue's patch needs to update the help so that it matches what is in that issue's patch. We can't assume all the patches will eventually get into the code base, or commit patches that don't also update the help to be consistent with the changes the patch makes, right?
Comment #245
amateescu CreditAttribution: amateescu commentedYup, this one is postponed on #2446511: Add a "preconfigured field options" concept in Field UI.
Comment #246
amateescu CreditAttribution: amateescu commentedJust keeping this patch fresh; rebased on top of #2446511: Add a "preconfigured field options" concept in Field UI.
Comment #247
amateescu CreditAttribution: amateescu commentedWe're no longer postponed on anything!
Comment #248
Wim LeersThis patch looks great. I couldn't spot any problems, but I'm no Entity/Field API expert. I'm hoping one of them can review this soon :)
Comment #249
amateescu CreditAttribution: amateescu commentedRerolled for #2452619: Move entity_reference_create_field function to EntityReferenceTestTrait.
Comment #251
jhodgdonBesides the current test failure, my feedback on #242 hasn't been addressed, I think? At this point, the help needs to be reviewed vs. the current UI to make sure the text matches as well.
Comment #252
amateescu CreditAttribution: amateescu commentedUpdated
taxonomy_help()
according to the suggestions from #242 and fixed the new test fail. I was poking around the code and found a few problems with the d6 vocabulary migration, fixed those as well.I also found that the upgrade path is not working anymore due to #2030633: Expand FieldStorageConfig with methods, but I'm not sure if we want to keep it or not. At least I remember @catch saying that we shouldn't :)
And do we want a change notice?
Comment #253
Wim LeersI think so. The final patch that brings us completely unified entity reference formatting — that's quite newsworthy IMO. Contrib developers dealing with entity references will be very excited, I think :)
Comment #254
webchickTalked about the upgrade path question with @effulgentsia, @alexpott, and @xjm today.
While it's totally awesome that there was at one point a working upgrade path for this patch (way to over-achieve ;)), committing it to core would set a precedent, and we are not yet ready to commit (pun!) to supporting beta-to-beta upgrades in core.
What should probably be done, as long as there are no heinous objections to #2455949: [policy, no patch] Re-activate the head2head project and use it for beta2beta upgrades in the short-term, is remove the upgrade path+test from here, but file it as a "needs work" patch in https://www.drupal.org/project/issues/head2head instead, and work to make that function over there (perhaps as the first such upgrade path that's semi-supported in D8).
Comment #255
amateescu CreditAttribution: amateescu commented"needs work" is not my thing and I like the head2head plan a lot, so I just did everything needed to have a *working* upgrade path for this issue: #2456419: Upgrade to Drupal 8 and provide a beta2beta submodule :)
Comment #256
jibranI think every possible feedback is addressed. Let me dare to set it RTBC after 2 whole years. Thanks @amateescu for sticking to this. Let's cross the finishing line now.
Comment #257
amateescu CreditAttribution: amateescu commentedPosted a change record draft which I'll update tonight with some code samples: https://www.drupal.org/node/2456869
Does anyone consider that we need a new round of usability reviews before committing this?
Comment #258
xjmYep, we should definitely have new usability testing, since a new solution has been implemented and lots of other blocking work has gone in.
Which sounds like a good project for a Saturday. :)
Comment #259
jhodgdonIf you're reviewing usability, could you also review the Taxonomy help page, and make sure that the UI text referred to on the page (like the visible name of the field, etc.) matches what this patch is actually setting up? Thanks!
Comment #260
jhodgdon(Taxonomy help page: admin/help/taxonomy I mean)
Comment #261
xjmI've tested the latest patch, and with the work that's gone into the field UI to unblock this, the user experience is much improved from where it was two years ago (eek) and also much closer to what's in HEAD.
When I first add the field, I can understand quickly exactly what I want to do to get a taxonomy field:
The next step is slightly confusing, because I again have the option to select the type of item to reference, even though I just did that when making my initial field selection. It does default to "Taxonomy term", but I also have the opportunity at this point to change it to a different entity type, which doesn't make sense. (I didn't try what happens if I select a different entity type.) I wonder if it is possible to hide this field when I've picked a specific field type already on the initial Add field form?
This form is also where I encounter the first difference from HEAD. I can't select vocabulary at this point (or put another way, I don't need to) because the vocabulary isn't a storage-level setting for the field. So I just submit and go to the next form.
Now, in the last step, I have the opportunity to select the vocabulary I want. It's still kind of buried on the page, but at least now since it defaults to no values (rather than all of them) I am forced to actually configure the field rather than just skipping over it:
I also read over the help text on
/admin/help/taxonomy
as @jhodgdon suggested and confirmed that all of it is accurate. There are a few formatting and wording inconsistencies, but nothing worth blocking this patch on. I will say that after reading this help text (and stumbling over a few unrelated field UI things that have changed from D7) I think a tour or such would go a long way, as the help text is hard to process on a page by itself. But that's totally out of scope and no different from HEAD -- actually, it's a bit better, because there is no longer a vague and confusing distinction between two different kinds of term reference field. That being the entire point. :)I'll take a look and see if I can improve the help text a little later, and maybe do a code review. The patch doesn't need to wait on that since it looks like Wim and others have done code review already, and the docs are fine as they are really. I would like some more information about the bit above (wherein I select "Taxonomy term" as the referenced entity type on two forms in a row) before RTBCing as there is a UX regression there.
@amateescu++
Comment #262
xjmAdding a beta evaluation and the remaining tasks to the summary. If anyone has more specific details to fill in for the Disruption section, that would be helpful.
Comment #263
claudiu.cristeaI thought that after this patch, taxonomy will only deal with Node in an abstract mode like a pure entity reference. Now I see that some methods in TermStorage, having node specific code are still there. Just curious about that... what I'm missing? :)
Comment #264
xjm@claudiu.cristea, removing the Node module dependency from Taxonomy is not in scope for this issue. This issue is only about the term reference field itself. :) Edit: Before or after the patch, as well as in D7, Taxonomy can be referenced by a field that can be applied to any fieldable entity type, but the module itself has some node-specific optimization.
Comment #265
xjmPoking at that
hook_help()
a bit.Comment #266
xjmI noticed a minor labeling inconsistency while updating the
hook_help()
. When you add a field, you select Taxonomy term under References in the Add Field UI. However, once you've configured the field, it's listed simply as Entity reference with no indication that it has anything to do with taxonomy. I don't think that's worth blocking this patch on, but it might be worth a followup issue. (Really it'd actually be a followup to #2446511: Add a "preconfigured field options" concept in Field UI I think.)Comment #267
xjmThe
hook_help()
states:Note that the UI doesn't actually enforce this; you can select the Create referenced entities if they don't already exist option with multiple vocabularies enabled. The term will just be created in... the vocabulary that comes first by weight. Which is also not the ordered they're listed in for the field configuration; that's alphabetical. ;) Followup maybe.
Comment #268
yched CreditAttribution: yched commented@xjm: that was one of my gripes with #2446511: Add a "preconfigured field options" concept in Field UI too. One approach could be to change the "field type" column in the "manage fields" overview to contain a "summary , like we have on “manage Display“. Would need a new method in FieldItemInterface though. Definitely follow-up material.
Comment #269
amateescu CreditAttribution: amateescu commented@xjm, thanks for reviewing :) Yes, the two problems you found are actually followups for #2446511: Add a "preconfigured field options" concept in Field UI. See comments #32 and #33 in that issue for a discussion about why you are still able to see the target type selection even if you pick a preconfigured field from the dropdown.
Also, it's not really possible right now to hide any storage or field settings in their initial add forms because the first step (i.e. the "add field" form) creates and saves both configuration entities in the submit handler, so the next two steps are just editing two pre-existing FieldStorageConfig and FieldConfig entities that don't have any knowledge about what happened in the first step of the workflow.
However, we can improve this potentially confusing "I already chose to reference taxonomy terms, why am I seeing the option again?" situation in #552604: Adding new fields leads to a confusing "Field settings" form, which is still blocked on #2446869: Convert the "Field storage edit" form to an actual entity form.
I'm fully aware that #2446511: Add a "preconfigured field options" concept in Field UI was a bit "rushed in", but I still think that its immediate benefit was very much needed for this patch :)
Comment #270
amateescu CreditAttribution: amateescu commentedWe have #2412569: Allow setting the auto-create bundle on entity reference fields with multiple target bundles for that and it's already RTBC :P
Comment #271
xjmThanks @yched and @amateescu. I posted a comment on #552604: Adding new fields leads to a confusing "Field settings" form. I think maybe it should be an explicit followup for that issue, though. That said... could we not just stick a form alter in
taxonomy.module
or something, to make it a hidden field that submits the value? I recognize that would be a big ugly hack. :P But it would get rid of the usability regression until it can be fixed properly.Meanwhile, some improvement to the
hook_help()
. I moved a bunch of stuff that is actually properly about Entity Reference rather than Taxonomy into the ERhook_help()
instead, and made the Taxonomy help reference it.Comment #272
xjmComment #273
rteijeiro CreditAttribution: rteijeiro commentedFixed a few nitpicks :)
I've been tempted to fix all
array
references to match new coding standards using[]
but maybe it's out of the scope of this issue.Comment #274
amateescu CreditAttribution: amateescu commented@rteijeiro, thanks, the interdiff looks good to me.
@xjm:
I thought about this for a while but I'm pretty sure that not even a big ugly hack like a form alter can't help us here. Like I said in #269, the second step of the workflow (the field storage edit form) has no way of knowing that the user came there after adding a new field or by manually navigating to that page. Well, except for http referrer, but I don't think we want to go that far :)
Or maybe I just can't think outside the box here because I have a very straightforward plan in mind: when we'll be able to show the storage settings form in the initial "add field" form via AJAX, we'll have at least
$field_storage->isNew() == TRUE
and we can get creative with that.Comment #275
webchickFWIW I am a lot more comfortable with seeing the same information twice (which is a bit awkward) compared to not seeing it at all (which was the previous situation). So if that's the only thing holding this patch up, I'm cool with RTBC. (Though I would love a chance to go through it myself before commit if possible.)
Comment #276
jibranSo from the remaining tasks of issue summary
We have #275 for that
We can improve it after the actual commit as well.
This need further discussion and we have a dedicated issue (#2455949: [policy, no patch] Re-activate the head2head project and use it for beta2beta upgrades in the short-term) for that so once we have a policy we can fix it.
Created #2458127: Show field/field-storage summary instead of field type on field listings
So none of the remaining tasks are commit blocker so just going to mark it RTBC.
Comment #277
alexpottHow come we're not just adding this logic to the template? field--node--field-tags.html.twig which already exists?
The use statements that are removed above can be removed by this patch.
It is exciting that this patch is so close.
Comment #278
amateescu CreditAttribution: amateescu commentedI was just doing a self-review and noticed another problem with field_tags, we're losing the smaller font size. Fixing that as well as #277.
Comment #279
amateescu CreditAttribution: amateescu commentedAlright, fixed the visual regression as well as the unused use statements. Turns out that #277.1 was not even needed anymore.
Also removed some text from taxonomy.module that was factually inaccurate, placed in the middle of nowhere, and duplicating what we're already saying in hook_help().
Assigning to @webchick for the final verdict, as requested in #275 :)
Comment #280
xjm@jibran, thanks for filing #2458127: Show field/field-storage summary instead of field type on field listings. I've added it to the summary.
No, we still need to actually file the specific followup issue for it (which may be postponed on #552604: Adding new fields leads to a confusing "Field settings" form), even if we scope it to a followup (which I am okay with if webchick and maybe Bojhan sign off on it, and webchick already has).
No, we can't. A valid change record is a blocker for commit. Right now it says:
These are small tasks though compared to the issue overall. :) For me this is RTBC once they are done, and following that RTBC we can keep it assigned to webchick for review/commit.
Comment #281
amateescu CreditAttribution: amateescu commentedUpdated the change record (https://www.drupal.org/node/2456869) and filed #2458777: Improve the user experience for the "preconfigured field options" concept for #261.2.
I call this issue done! But still assigned to @webchick for a final review :)
Comment #282
xjmThe updated CR looks great to me, and +1 for the RTBC.
Comment #283
xjmUpdated the CR for the bonus beta this morning: https://www.drupal.org/node/2456869/revisions/view/8284349/8287715
Comment #284
Bojhan CreditAttribution: Bojhan commentedThis looks good (from reviewing screens, did more in-depth reviews earlier). I agree with the followups regarding the repetition that can be quite confusing. Hopefully that can be resolved though
Looking forward to seeing this in
Comment #286
webchickThanks a lot for giving me a chance to review. Here's the gist:
So basically there's a UX regression here with vocabulary selection. It used to be the second page of the form (which is logical); however, now the second page of the form essentially repeats the same selection (which is silly), and the vocab selection (which is required) is shoved to the *bottom* of the third page which is usually the optional "you can ignore this" page.
This is pretty bad, and I'd really like to see this solved before release. However, unlike the problem with not seeing "Taxonomy term" (and its ilk) in the list at all, this one is at least workaroundable, if annoying, by non-experts. They'll save the third form without looking, just as they usually do, then get an error message, then grumble and select the right vocab.
xjm/amateescu pointed out #552604: Adding new fields leads to a confusing "Field settings" form and #1953568: Make the bundle settings for entity reference fields more visible. already exist to address this problem, and so we bumped both of those to "major" as a result. Talked it over with Bojhan and he's cool committing this patch despite the UX regression, as am I, since I certainly appreciate all of the TONS of effort that's gone in to addressing UX feedback to-date, and we're down to 5-6 upgrade path blockers now.
So with that, committed and pushed to 8.0.x. YEAH!!!!! :D
Comment #287
plachYay! @amateecu++++++
Comment #288
Wim LeersHUZZAH!
Comment #289
Jeff Burnz CreditAttribution: Jeff Burnz commentedOK, so how does one now target all taxonomy fields with one template? afaict this patch committed a regression for Bartik, since now if the user creates a new taxonomy field it won't be styled correctly. This patch even hard coded in field-name classes. When someone upgrades D7 site to D8 and they have taxonomy fields other than than "tags" their sites style will be broken, theres no CSS and no template to account for it afaict?
Comment #290
amateescu CreditAttribution: amateescu commented#289, that's right, the patch converted specific styling to target only the 'field_tags' field. If we want to bring back the old behavior all we need is to either 1) implement a new theme hook suggestion that includes the target entity type for entity reference fields or 2) add a entity reference specific css class to the existing field template. Any idea how we can do that in D8? Also, can you please open a new issue for it? :)