Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Coming from #1847596-142: Remove Taxonomy term reference field in favor of Entity reference, it seems that usability will be improved if we move the field cardinality setting below the ones provided by the field module.
| Comment | File | Size | Author |
|---|---|---|---|
| #19 | 1953770-19.patch | 2.63 KB | amateescu |
| #12 | allowed-values.png | 275.64 KB | yoroy |
| #4 | 1953770-4.patch | 1.89 KB | amateescu |
| #2 | term_ref.png | 36.07 KB | xjm |
| #2 | body.png | 26.55 KB | xjm |
Comments
Comment #1
amateescu CreditAttribution: amateescu commentedWe can either do this or move that whole piece of code up a few lines above in the function. I chose the small patch for now because it seems to have the least chances of conflicting with other field api patches.
Comment #2
xjmHere's some screenshots of this patch applied with various field types in HEAD. Overall I think this is an improvement.
Term reference
Longtext
Image
Date
Entity reference
File
List
Comment #3
tstoecklerI won't fight it (anymore than this comment), but I think it's a WTF to declare weights for form elements that you have just declared. I would prefer the code simply be moved appropriately. To my mind weights serve two distinct use-cases:
1. General defaults, not specific to the form, such as adding a weight of 10 to submit buttons. (I don't think we actually do that currently, at least not consistently, but it's something that IMO would make sense.) Or, for easier altering add weights in steps of 10 to all form elements in the order of declaration.
2. Adding form elements at a specific place between other elements in a form alter.
Having the form elements come out in a different order than they are declared in (in the same function) is strange to me.
Comment #4
amateescu CreditAttribution: amateescu commentedRe #3: I totally agree that what I did in #1 is not clean at all, I just chose this to way to reduce the potential for conflicts. We can also say that we don't care about that and do it properly :)
Regarding weights for form elements that were just declared, I actually think that's a very useful thing to do. Consider this following scenario:
- Module A wants to alter this form and insert 6 elements between the field cardinality element and field settings container. We can't rely on contrib for playing nice and using a container wrapper, so let's say it sets the weight of the field settings to 10 and inserts its elements in between.
- Module B also wants to alter this form, but has only one element to intert there, so it sets the weight of the fields settings to 5, overring what module A already did.
In conclusion, my impression is that a little babysitting in this case is not the wrong thing to do, so I moved the code in the natural form order but kept the weight of -10. Screenshots in #2 are still accurate.
Comment #5
tstoecklerI agree 100%. What I tried to say in #3 was that I find it very strange to use weights that do *not* follow the order of declaration. I think we should accept the possibility of patch conflicts in this case.
Comment #6
amateescu CreditAttribution: amateescu commentedUps, this means I misread what you said (quite possible that early in the morning), sorry for over-commenting then :P
Comment #7
tstoecklerNo harm done, just wanted to say that I like the patch in #4. :-)
Comment #8
yoroy CreditAttribution: yoroy commented+1 On moving the 'type' select into first place.
Based on what I see in the screenshots for Image, File and List, it looks like the 'Number of values' select should come second, before all the other nitty-gritty. It would make these forms more consistent across the board.
Comment #9
Dave ReidOne downside to moving these to up top, is that once the fields have data, you cannot adjust some of those settings at all.
Comment #10
yoroy CreditAttribution: yoroy commentedBut how is that a downside? (And how does that measure up to getting the first-time setup UX in order? :-)
Comment #11
xjm#9 is already the case for all fields on this form. (It's the fields settings form, not the instance settings.) So, I don't see how this patch affects that at all.
Comment #12
yoroy CreditAttribution: yoroy commentedI meant to suggest this:
Disregard if that is not in scope for this issue.
Comment #13
xjmSo @yched said he wants it to be one or the other--either always above, or always below, for consistency. From the screenshots I think the weight below is better most of the time, since the field-specific settings are generally going to be more interesting to the user.
Comment #14
xjmSetting back to NR for that.
Comment #15
yoroy CreditAttribution: yoroy commentedi dont understand what 'the weight below' means. my suggestion was:
1. type
2. number of values
3. other...
since the screenshots for image, file & list suggest there is no 'type', i propose to do
1. number of values
2. other...
that's consistent, no?
Comment #16
xjm@yoroy, sorry, I meant the number of values. Not sure why I typed "weight".
The number of values is a generic field API setting. Everything else is provided by the particular field type. Therefore, according to @yched, the number of values should either always be above the specific settings, or always below them.
Comment #17
jibran#4: 1953770-4.patch queued for re-testing.
Comment #19
amateescu CreditAttribution: amateescu commentedSo.. are we going to do this or not? :)
Comment #20
swentel CreditAttribution: swentel commentedyes!
Comment #21
swentel CreditAttribution: swentel commentedyes!
Comment #22
alexpottI think the issue summary suggests we doing what the opposite of the patch actually does. It would nice to have a screenshot showing the affect of the patch.
We also need to justify why a normal task should be committed under #2350615: [policy, no patch] What changes can be accepted during the Drupal 8 beta phase? - I think the usability provision should cover it.
Comment #23
amateescu CreditAttribution: amateescu commented@alexpott, nope, we're doing exactly what the IS says and the screenshots from #2 are still accurate :)
Comment #24
alexpottOh okay it's just that #12 and the responses to that comment makes it difficult to grok
Comment #25
yoroy CreditAttribution: yoroy commentedI was confused at first too. It's an improvement because some field specifics will be required items, which are good to show before the optional 'allowed values' which provides a sensible default for most cases (one).
Comment #26
alexpottThis issue is a normal task that will improve usability and the disruption it introduces is limited. Per #2350615: [policy, no patch] What changes can be accepted during the Drupal 8 beta phase?, this is a good change to complete during the Drupal 8 beta phase. Committed 6061967 and pushed to 8.0.x. Thanks!