Contributor tasks needed
Task Novice task? Contributor instructions Complete?
Update the issue summary Yes Instructions
Draft a change record for the API changes Yes Instructions

Remaining tasks

1. Write a change record
Read How to write a change record

for the 8.x version patch in #54 was committed by #57
for the 7.x version patch in #67 was committed by #83

Original report by @TomDude48

Recommendations:

Change label to “machine name”.

Present an autogenerated machine name, with an option to change.

CommentFileSizeAuthor
#87 drupal-1164812-87-temporary-rollback.patch13.03 KBDavid_Rothstein
#67 drupal-1164812-67.patch13.03 KBtim.plunkett
#63 drupal-1164812-63.patch13.03 KBtim.plunkett
#62 machine_name.png11.5 KBdroplet
#54 1164812-machine-name-ux-53.patch13.15 KBNiklas Fiekas
#48 1164812-machine-name-ux-48.patch13.17 KBNiklas Fiekas
#48 1164812-interdiff-48.txt1.46 KBNiklas Fiekas
#44 field-ui-max-width.png26.2 KBNiklas Fiekas
#44 1164812-machine-name-ux-44.patch13.17 KBNiklas Fiekas
#44 1164812-interdiff-44.txt469 bytesNiklas Fiekas
#41 1164812-machine-name-ux-41.patch13.17 KBNiklas Fiekas
#41 1164812-interdiff-41.txt799 bytesNiklas Fiekas
#40 field-ui-widescreen.png25.26 KBNiklas Fiekas
#40 field-ui-edit-mode-and-error.png28.36 KBNiklas Fiekas
#40 field-ui-start-entering.png24.19 KBNiklas Fiekas
#40 fied-ui-nothing-entered.png23.28 KBNiklas Fiekas
#36 1164812-machine-name-ux-37.patch13.15 KBNiklas Fiekas
#36 1164812-interdiff-37.txt1.53 KBNiklas Fiekas
#27 1164812-machine-name-ux-27.patch11.82 KBNiklas Fiekas
#27 1164812-interdiff-27.txt2.96 KBNiklas Fiekas
#26 1164812-26.patch11.75 KBxjm
#26 interdiff-20-26.txt1.3 KBxjm
#26 page_load.png47.01 KBxjm
#26 enter_label.png54.28 KBxjm
#26 editing_machine_name.png53.56 KBxjm
#21 field-ui-long-description.png30.66 KBNiklas Fiekas
#20 1164812-machine-name-ux-20.patch11.35 KBNiklas Fiekas
#20 1164812-interdiff-20.txt7.43 KBNiklas Fiekas
#17 1164812-machine-name-ux-16.patch10.62 KBNiklas Fiekas
#14 1164812-machine-name-ux-14.patch11.49 KBNiklas Fiekas
#14 1164812-interdiff-14.txt2.14 KBNiklas Fiekas
#11 1164812-machine-name-ux-11.patch9.95 KBNiklas Fiekas
#11 field-ui-machine-name.png26.99 KBNiklas Fiekas
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

yched’s picture

Component: field system » field_ui.module

recategorize

webchick’s picture

Yeah, this was pretty funny.

The field help text is "Field name (a-z, 0-9, _)"

The participant read the help text, and then the first thing they entered in that field was "a-z", which is of course wrong. I think this might be because they thought the things in parentheses were examples of field names. (But in any case, having the expectation that normal people natively understand Perl regex patterns is perhaps a bit of a stretch. ;))

I think if we just changed this to a #type managed field that would be fine. Participants didn't struggle with that widget at all.

xjm’s picture

Assigned: Unassigned » xjm

Based on the UX testing this week I think I want to try this. :) My thought is to kill that column entirely on manage fields and generate it as a machine name from the label.

jenlampton’s picture

Title: Field machine names had help text, but it wasn't helpful. » Update the description or help text on machine names for fields.
Assigned: xjm » Unassigned
Issue tags: +Novice

Participants in the Google Usability study also found the help text for machine names for fields misleading. One participant entered simply 'a' and another entered 'a-x, 0-9' generating an error.

It would be good to replace this short-hand with something that's actually helpful for people wondering what to put in this field.

yched’s picture

Enhancing the description is fine of course, suggestions welcome.

I'm not sure the "auto-generated machine name suggestion" pattern applies here.
Unlike (I think) most current uses of this pattern, field machine names cannot be changed once the field has been created.
If you just created the field without paying special attention to the suggested name, you're screwed if you find out later that the machine name is wrong. And it's most likely wrong if you're using any language with accents, non latin chars, etc... (they are replaced with underscores : "Désert" -> "d_sert", "сказать" -> "_")

xjm’s picture

I think it'd be possible to give the user enough feedback to change the field name as needed. And, of course, they can always override the generated one. Hopefully the UX study recordings can be posted--users got REALLY stuck on this part of the Field UI, beyond just the description. They also consistently overlooked the label column. It's more than just that the description is bad.

xjm’s picture

Title: Update the description or help text on machine names for fields. » Improve UX for machine names for fields.
droplet’s picture

without description, would it better ? and rename column title from "Name" to "Machine Name"

Off-topic
Non-tech guys and beginning don't know what is "Machine Name", "Machine readable...." ...etc

similar issue:
#1399302: Rename Field UI's "Field" column to "Field Type"

xjm’s picture

Tagging.

jenlampton’s picture

Issue tags: +GoogleUX2012

updating tags

Niklas Fiekas’s picture

Status: Active » Needs review
FileSize
26.99 KB
9.95 KB

The attached patch puts in a machine name element for that field.

However machine_name needed some adjustments:

  • Add a standalone setting, so that the live preview stays in it's own table column, not behind the source field.
  • Support the #field_prefix field_. (And #field_suffix for symmetry.)
  • Don't display label if explicitly set to emtpy.

(I realize that this might need a seperate issue, if it gets in at all. Despite that it's a really useful feature, that I could have used in other places, already.)

With that: Let machine_name do the #maxlength checking, uniqueness checking, validity checking.

And: Changed the table column to Machine name, as well as the field description.
field-ui-machine-name.png

xjm’s picture

This is exactly what I had in mind. Awesome awesome! I'll do a closer code review later, though I can see scanning that the callback docblock needs a tweak. Reference: http://drupal.org/node/1354#callbacks. Actually, it's not that kind of a callback, so a different paradigm applies. Is exists a Render API thing? (Exposed data structures!) If so: http://drupal.org/node/1354#render

We'll want to add some automated tests for this. We'll also need to solve the transliteration issue yched mentions, but in my opinion that is a separate issue, since it already affects all machine names in core. Finally, we'll want to do some usability review with the UX for new patch (for cases where the machine name is invalid, already in use, etc.)

Status: Needs review » Needs work

The last submitted patch, 1164812-machine-name-ux-11.patch, failed testing.

Niklas Fiekas’s picture

Status: Needs work » Needs review
FileSize
2.14 KB
11.49 KB

There is quite some test coverage already. The tests failed on a string change: Fixed. The tests also catched that machine_name fields are required by default, preventing the field from being left empty and thus not allowing to just add an existing field: Fixed.

Doxygen: Fixed. (Assuming that Render API callback is the right thing here. Actually it's more a callback passes to machine_name using the Render API, but that could be fine.)

The last submitted patch, 1164812-machine-name-ux-14.patch, failed testing.

xjm’s picture

Status: Needs review » Needs work

There is quite some test coverage already.

Right, but as we're adding new functionality, we need new test coverage to match. ;) We also need to adjust the existing tests.

Regarding the docblock, I still think it needs the paragraph about how the callback should be used. Thanks!

Niklas Fiekas’s picture

Status: Needs work » Needs review
FileSize
10.62 KB

Rebase.

Edit: And just a rebase. #15 was posted in the mean time and not yet accounted for ;)

Bojhan’s picture

Looks fine to me. It's a bit funky to put such an interaction pattern in a table, but Field UI sucks anyways.

The only thing I would recommend changing is the font-size, having that be special and smaller is distracting. Can this be normal?

The description is now just repetition, can't this be like "Label in the database"

Bojhan’s picture

Issue tags: +Usability
Niklas Fiekas’s picture

Changes:

  • No description by default, only in edit mode.
  • Tried the default description:
    A unique machine-readable name. Can only contain lowercase letters, numbers, and underscores.

    That one is way too long, though, and breaks the layout. The attached patch has A unique machine-readable name now, which has about the right length, but might not be a good description. Maybe Field name (a-z, 0-9, _) isn't a too bad one.

  • Added testDuplicateFieldName().
  • The changes to the machine_name widget are pretty small now. The standalone flag, support for prefixes (and suffixes), display no label when explicitly empty. All of them seam to be good to have anyways. (Plus the diff looks bigger than it really is, because a few lines have been moved into an if-block.)
Niklas Fiekas’s picture

Screenshot of the layout with the long description. Layout broken, so some more CSS changes would be needed (and probably not trivial ones).

xjm’s picture

Assigned: Unassigned » xjm

Neat! Gonna try to fix the CSS.

droplet’s picture

Even use fixed width, it will be 3 or 4 lines. can it use short description and long description on warning / error message.

droplet’s picture

eg. Google never told user about it. show description on error message

https://accounts.google.com/CreateAccount?hl=en-US

myself never try to type in "#$$@#$#%#" and I think it's for 80% users.

"lowercase letters" can be "letters", and an extra function to convert it.

xjm’s picture

That's a good point; we might want to consider updating the default machine name description to be shorter as well. Separate issue for that, though.

For now, let's try:

A unique machine-readable name containing only letters, numbers, and underscores.

I'll roll a patch with that + the CSS fix.

xjm’s picture

Patch with the CSS fix and description change, interdiff, and a couple screenshots with some other things I noticed.

Form on page load

page_load.png

Entering a label

enter_label.png

Editing the machine name

editing_machine_name.png

An aside, I think the issue with the tabledrag icon wrapping next to the label when the window is small is a separate issue.

Niklas Fiekas’s picture

Awesome, xjm! Thank you.

Risking to take an issue that's assigned to you once more, here are some changes:

  • An aside, I think the issue with the tabledrag icon wrapping next to the label when the window is small is a separate issue.

    Well ... at least an issue that was there, but that shows up when this gets in. word-wrap: nobreak is a quick fix. I think we can do that here.

  • Don't show the machine name when the label is empty. Actually that's the default behaviour for machine_name field, so I am happy to remove another hack from the machine_name code.
  • Increased the size to 15, which is pretty random. But 32 minus field_ would be too big.
  • One hunk about the field_-prefix validation, that I forgot to include in my earlier patch.
  • The catched <-> caught xjm pointed out.
droplet’s picture

Status: Needs review » Needs work

#1.

type in "你好嗎"
EDIT disappeared

#2.

It added white-space: normal / word-wrap: nobreak for small screen. But in big screen is really bad. Can it add a max-width for the texts.

Niklas Fiekas’s picture

#1: It's default machine_name behaviour not to display the machine name preview (and also not [edit] as well), when the human name is empty or just replaced characters. (I am not saying, that behaviour is good, though. I guess this will be solved when the transliteration is improved.)

droplet’s picture

Default behaviour ?? so why show it for a-z,0-9... ?

and for other non-latin user must hit error to type the machine name ??

nod_’s picture

droplet’s picture

@nod_,

thanks for the info. I posted my thought there.

But for this issue, I would suggest to postponed until that issue get solved. Because its changes making editable field unusable. Create 10 fields with CJK characters will hit 10 times of error.

amateescu’s picture

Status: Needs work » Needs review

#32: I disagree, they have nothing to do with each other. This one is a big improvement for the Field UI that can get in on it's own.

Niklas Fiekas’s picture

Status: Needs review » Needs work

The test failures were probably legitimate. Gotta fix them.

@32: Yes, two issues. In the end both will get in. No reason to delay one. (I hope not, but this issue might take its time anyway.)

xjm’s picture

Assigned: xjm » Unassigned

Oops, meant to unassign after I added my fix.

Niklas Fiekas’s picture

Status: Needs work » Needs review
FileSize
1.53 KB
13.15 KB

Ok, tests fixed. It was (now incorrectly) assumed, that the field_ prefix should be entered into the form.

droplet’s picture

+++ b/core/modules/field_ui/field_ui.admin.incundefined
@@ -511,16 +511,23 @@ function field_ui_field_overview_form($form, &$form_state, $entity_type, $bundle
+        '#maxlength' => 32 - drupal_strlen('field_'),

fixed num is better? 26.

+++ b/core/modules/field_ui/field_ui.cssundefined
@@ -29,6 +32,10 @@ table.field-ui-overview tr.region-populated {
+  white-space: nowrap;

for specified browsers ? white-space: normal; is good enough.

I hope no delay one but #1065626: Machine name not editable if every character is replaced. forgive users for a year. 99% developers use fields and create a bundle of it. D8MI focus on more foreign lang users. we introducing a buggy feature into higher usage place. If this is create content page, it's a critical bug?

Don't make an assumption. when another issue never get in, it's no advantage to non-latin users.Trade-off, we can forgive them for 80% users.

The issue start with "Usability". It's my concern.

11 days to next Drupal core point release.

xjm’s picture

I think that's a fair point, @droplet. I increased the priority of #1065626: Machine name not editable if every character is replaced. for that reason.

Bojhan’s picture

Can we get a screenshot? I feel like this issue is getting close to being done.

Niklas Fiekas’s picture

+++ b/core/modules/field_ui/field_ui.admin.incundefined
@@ -511,16 +511,23 @@ function field_ui_field_overview_form($form, &$form_state, $entity_type, $bundle
+        '#maxlength' => 32 - drupal_strlen('field_'),

fixed num is better? 26.

Yeah ... not sure about this. It's clearer why 26 like this, so maybe hardcode 26 and make a comment?

Here are some screenshots:

Before entering anything

fied-ui-nothing-entered.png

Starting to enter a field name

field-ui-start-entering.png

The edit mode on a small window + error message

field-ui-edit-mode-and-error.png

A window large enough to have the description on one line

field-ui-widescreen.png

Niklas Fiekas’s picture

Alright, so reroll with 26 hardcoded.

Bojhan’s picture

It's looking!

Is it much trouble to wrap the machine name description? I believe The edit mode on a small window + error message description, is a more desirable pattern than one long line. It being condensed is easier to scan and doesn't turn it into a horizontally large table.

xjm’s picture

@Bojhan: The description wraps when the window is small enough to need it, as in the second-to-last screenshot. If we wanted we could also specify a max-width on it, I suppose.

Niklas Fiekas’s picture

It looks like this with a max-width of 250px:

field-ui-max-width.png

xjm’s picture

I tested this with the changes from #1065626-23: Machine name not editable if every character is replaced. applied alongside it, and that works with this patch to resolve the issue of the field not being editable if the user enters a non-latin-alphanumeric machine name. So, yay! We will just need to reroll one or the other depending which goes in first.

Bojhan’s picture

From my POV its RTBC, if code review is good to - lets get it in!

Tor Arne Thune’s picture

+++ b/core/includes/form.incundefined
@@ -3334,6 +3334,9 @@ function form_process_tableselect($element) {
+ *     - standalone: (optional) Whether the live preview should stay in it's own

it's -> its.

+++ b/core/misc/machine-name.jsundefined
@@ -19,6 +19,10 @@ Drupal.behaviors.machineName = {
+   *   - standalone: Whether the preview should stay in it's own element rather

it's -> its.

Otherwise this looks good :) Great to see this stumbling block get some polish.

Niklas Fiekas’s picture

Thank you Tor Arne Thune.

I have blurry memories of xjm telling me about that in IRC, but it looks like I forgot it.

klonos’s picture

...the transliteration thing issue is #567832: Transliteration in core btw. If that one gets in too, then this UX series of WTFs will be completely fixed ;)

cosmicdreams’s picture

previously tested this one. It's ready for manual testing. (looks good to me)

xjm’s picture

Issue tags: -Usability, -Novice, -UMN 2011, -GoogleUX2012

#48: 1164812-machine-name-ux-48.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Usability, +Novice, +UMN 2011, +GoogleUX2012

The last submitted patch, 1164812-machine-name-ux-48.patch, failed testing.

xjm’s picture

Niklas Fiekas’s picture

Status: Needs work » Needs review
FileSize
13.15 KB

Rebased and verified it still works (including #1065626: Machine name not editable if every character is replaced.).

Edit: Yay! Crossposts seldom work, but this one did ;)

xjm’s picture

Status: Needs review » Reviewed & tested by the community

Whee!

Bojhan’s picture

W00t!

catch’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)
Issue tags: +Needs change record

Makes sense to me. Committed/pushed to 8.x.

webchick was talking about trying to backport this to 7.x, not sure whether or not that's going to be doable, but moving it there anyway.

This is also going to need a change notification either way.

klonos’s picture

Great! I hope to see a fix for D7 soon.

xjm’s picture

So the worst that happens if we backport this:

  • It changes the FAPI array for this form, and so could break custom theming or form alters for the page.
    • Since it's an administrative form, end users would not see it; only site or content administrators.
    • On the other hand, sites that might have this form altered would be sites that already put extra care into customizing content management UX.
  • It changes the UI, potentially confusing users. "Where did the box go?" (This question is answered when they type something into the label field, but there is a conspicuous white spot there.) Since #1065626: Machine name not editable if every character is replaced. was backported, the worst-case scenarios here are:
    • "My Drupal is broken; the machine name field is GONE!"
    • Someone unwittingly generates a field name that already exists or consists only of underscores. Not really a problem since form error handling then takes over and the machine name field is expanded automatically and highlighted.
  • It invalidates existing books, docs, etc. that have screenshots of the form as it was previously or that instruct the user to enter a value. This doesn't seem a big deal to me since it's a relatively small change and the overall UI for manage fields is the same.
webchick’s picture

Just a note that the machine name field is only gone for *new* fields. Existing fields' machine names still show up in the table.

Agreed on the other risks, though. I still think that this is worth backporting, perosnally.

yched’s picture

Might be good to have swentel and stalki's opinions. DS and field_groups are the main modules hooking onto / altering that form (notably by adding their own 'add new X' rows, with associated machine name)

droplet’s picture

FileSize
11.5 KB

"My Drupal is broken; the machine name field is GONE!"

I don't think end users will ask it but non-english users may ask
"Why I hit save and show me error, no default for machine name ...etc"

machine_name.png

#1447860: Show machine name input if every character is replaced

tim.plunkett’s picture

Status: Patch (to be ported) » Needs review
FileSize
13.03 KB

I didn't try this at all, just rerolled it.

Status: Needs review » Needs work

The last submitted patch, drupal-1164812-63.patch, failed testing.

tim.plunkett’s picture

Assigned: Unassigned » tim.plunkett

How did I know that blindly rerolling wouldn't work? :)
I'll get to this tomorrow.

xjm’s picture

That's a truly impressive number of fails. Backport needs to wait on #811542: Regression: Required radios throw illegal choice error when none selected perhaps? (Wild guess based on the fails on the bot.)

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
13.03 KB

D8 uses complete_form, D7 still uses complete form.

tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned

Straight reroll and a one character change, I love when it's easy.

klonos’s picture

Assigned: Unassigned » tim.plunkett
Status: Needs review » Needs work

Yeah, the risks taken are worth the UX improvement. Thanx for taking this task for 7.x Tim. I will test and provide feedback as soon as you get those tests passed. I'll even test with field_groups too (might also give displaysuite a go, but I don't promise) and file issues in their respective issue queues. Any other popular contrib that comes to your minds that might need testing?

swentel’s picture

Status: Needs work » Needs review

Ran all tests for Display suite and turned all green. Did some manual testing with field group and saw no problems either, so this is safe for us :) I'll let someone else RTBC this.

swentel’s picture

Status: Needs review » Needs work

Woops, didn't mean to change the status. To recap, ds and field_group are fine, no idea about other contribs though.

klonos’s picture

Assigned: tim.plunkett » Unassigned
Status: Needs work » Needs review

...that was fast :)

tim.plunkett’s picture

Those were the modules I had in mind, not sure of others.

klonos’s picture

Good to see that at least one of the maintainers from field_groups that is also the maintainer of DS is following. I was about to file heads-up issues to these projects issue queues :)

PS: ..do you think that field_collections might be effected by this too?

swentel’s picture

I don't see any problems with field collections either, as they simply follow the same behavior of creating fields as any other core field.

klonos’s picture

Great!

xjm’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +7.13 release notes

Thanks @swentel! Going to be bold here. So long as we document this nicely in the release notes, I think the UX bonus of backporting this is worth the risk, and no one seems to disagree yet.

jenlampton’s picture

I think this patch still needs work, but since this is so much better than what we have now I'm not changing the status :)

If we assume all fields will be shared across all entities, and name fields like this without the entity type name in the field name we're going to be creating a lot of problems for new people down the road.

Here's the best practices approach to naming fields:

If the field is shared across content types (or other entities), for example an address field that applies to both members and events, then the field name should be field_address. If the field is one that's specific to an individual content/entity type, then the name should contain the name of the content/entity type too, for example, field_event_image or field_profile_image.

Here's why.

On your events, you want the image to be a square 200px by 200px, but if you had a profile image cropped into a square, you'd be cutting off people's heads, so your profile image needs to be 200px by 300px.

Now create a list (view) that includes both profiles, and events. If this view's row style is of type node, then you're fine because the node displays of this field are used and those are different for each content type. But now, change the row style to field. If you are using the *same* field for these different images, you can't use different image styles without custom code. It's very easy to make two different fields look or behave the same (when needed) but very hard to make the same field behave two different ways.

I'm not sure that we should be worrying about how people will or won't use the views module in core... But I do worry that we are creating bad habits by setting the default values of these field names incorrectly. For people who don't know any better we are going to be forcing them into a corner where they won't be able to do what they want - without custom code. Yes, everything is "possible" in Drupal, but this makes it harder.

Part of what makes Drupal so awesome is that you *can* do so much without writing custom code, but people will be missing out on some that if they get their architecture wrong from day 1.

This solution is 100% better than what we have now (since new people don't know how to name these fields anyway) so I'd be willing to give on this point.. But just wanted to make my concerns known. :)

tim.plunkett’s picture

@jenlampton I definitely agree that it makes that approach less discoverable, but I personally didn't discover the method you describe until someone told me about it, having a machine name field presented didn't help me discover it directly.

klonos’s picture

This issue's goal is to "improve" in general (judging by its title). No specifics are defined, so I take it we'll be opening and closing it as we improve more and more bits each time. No harm to commit improvements as we go ;)

...alternatively, we can make this a meta and file separate smaller issues as we go. The changes we've made so far will greatly improve UX so they shouldn't wait on future plans. We'll never get it to be *perfect* anyways.

Niklas Fiekas’s picture

Given that we have multiple things to do: Note that we'd be also going D8 -> Backport to D7 -> D8 .... The issue is currently at 7.x. Maybe we should really have different issues for other improvements, maybe some of them aren't even backportable.

droplet’s picture

@jenlampton concerns also apply to #1436596: Support transliteration of machine names.

At this issue, we only focus on "autogenerated machine name" is easier for newbie. But missed someone want/need to set "custom machine name".

so now, for general English speakers it may be a good solution but @jenlampton / experts / non-english users, they need one more click to finish this job.

another solution is leave it as a input field (no edit link) but with autogenerated machine name.

just come up a new idea. see #62.. when it's empty value, can we hint users: (empty) [edit link]

and found that isn't generated unique machine name

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Ok! Let's do this thing! :)

Committed and pushed to 7.x. Yay!!

Regarding the last few comments, I think *new* issue(s) for those might make sense. Jen's comment in #78 is particularly interesting, but I can't really think of a way that Drupal could envision what the ultimate intent is of a field (shared v. not shared) when it is first created, and of course changing the machine name after the fact can be problematic. Hrm. It's worth thinking about, but we should definitely not kick off this discussion in a thread with > 80 comments already. :)

klonos’s picture

Perfect! Thanx Angie ;)

David_Rothstein’s picture

Status: Fixed » Needs review

I'm going to play devil's advocate and ask if the timing on this commit was maybe not ideal and should be reconsidered?

Assuming there's a core release this week (which I think there's supposed to be) that means this patch is scheduled to only have a few days in 7.x-dev before being released. Given the possible risks, this seems like the kind of patch that would be safer to commit right after a point release instead, so it has a while to live in 7.x-dev and more time for module authors to find possible issues.

Also, it's been a while since I followed up on it, but the discussion at http://groups.drupal.org/node/210973 seemed like it was moving towards some ideas for how to do these kinds of changes in a safer way (basically, a "backports" module included in core, and the changes to the Field UI form in a patch like this would be done via a form alter in that module, which would be safer for a number of reasons). Not sure if we'll get to that (and I wouldn't hold this issue up too long over it) but it's something to think about.

In terms of risk, while it's not too bad, the fact that the patch changes #type puts it relatively high on the "riskiness" list that @effulgentsia compiled at http://groups.drupal.org/node/210973#comment-701613. And that's not entirely theoretical - the last core change I know about that changed a #type seems like it probably broke Context module (see #1346760-38: Add a scalable weight select element).

klonos’s picture

Thanx David! I already knew about Fix Core and Edge, but I wasn't aware of the existence of Backports. They seem somehow related/similar in what they try to offer and I wonder if it would be wise(r) if their maintainers came together in order to have a single module that does what they all do and at the same time have more "manpower" to help with maintenance. Also it seems a great pity to have audience split across three projects when we can have one single place where we could all "scratch that same itch" ;)

I don't know how the other two work, but Fix Core offers a settings page that allows one to select to enable only whichever features/backports/fixes they require. This way seems very intuitive to me in that it's easier to troubleshoot what the culprit is if nasty things start to happen.

I realize that there might be a lot of major changes between core versions that would perhaps make this impossible (take #22336: Move all core Drupal files under a /core folder to improve usability and upgrades for example), but I always was font of how one can test "incompatible" addons in firefox and was wondering how we could achieve a similar thing in Drupal: Do we have an "advanced", hidden-from-UI setting that allows D6/D7 modules to be installed under D8 somehow or do we "push" (as in nag) for as many things as possible to be backported to previous versions? Well, it seems that these projects are the answer and I really like the idea of having a backports.module in core!

PS: besides the discussion you link to, is there any issue filed against D8 to move backports to core (or have a similar module in core anyways)?

David_Rothstein’s picture

Hm, I didn't know about some of those other modules, thanks. I went ahead and created a core issue now:
#1505456: Add a "Backports" or "Drupal update" module to Drupal 7 core so that UI changes from Drupal 8 can be backported

So, meanwhile, is there agreement that rolling back this issue for now is a good idea? We have to decide, like, real soon, so I went ahead and attached a rollback patch.

To clarify the proposal is:

  1. Roll this issue back now, before the imminent release.
  2. Commit it again immediately after the release.
  3. Work on #1505456: Add a "Backports" or "Drupal update" module to Drupal 7 core so that UI changes from Drupal 8 can be backported with the goal of moving the Field UI code from this patch to a separate backports module. If that issue doesn't get resolved before the following Drupal 7 point release, that's OK; we can still go ahead and release it as is, since at least it will have had a while to sit in 7.x-dev and for people to catch any problems with the Field UI changes.
webchick’s picture

I suppose I could roll this back tomorrow morning, but can you explain where your trepidation comes from? To me this feels very low-risk. We even grepped contrib, and they affected module maintainers have commented here saying +1.

webchick’s picture

Also, fwiw, #1346760: Add a scalable weight select element was committed a full 53 days (!) before the following stable release, and this obviously didn't help us catch breakage. :(

David_Rothstein’s picture

The trepidation comes from the last paragraph of #85 :)

webchick’s picture

Yeah, ok, but see #89. :(

I dunno. Does anyone else feel this is important to rollback? So far it's only David. We have about 1-2 hours to decide.

catch’s picture

I don't feel it's important to roll back, but it's easy and no risk enough to do that then commit it again after the release is out, and it's not something that feels like it'd absolutely have to get into 7.13 at all.

webchick’s picture

Status: Needs review » Fixed

Ok, we found an easy solution. We just don't release this month! :P

In addition to this issue, there's some stuff the sec. team is still working on, and also #1280792: Trigger upgrade path: Node triggers removed when upgrading to 7-dev from 6.25 is a scary upgrade-path-affecting issue that could use more testing.

And! We even get a bonus week since April 25 is during the Drupal.org upgrade sprint, so the next core release will be May 2.

Moving this back to fixed, and going to go work on my taxes now. :)

xjm’s picture

I'd agree with opening additional followups for the additional improvements @jenlampton describes. Edit: I was also going to say make a followup for the backport stuff, but David already did.

In terms of risk, while it's not too bad, the fact that the patch changes #type puts it relatively high on the "riskiness" list that @effulgentsia compiled at http://groups.drupal.org/node/210973#comment-701613. And that's not entirely theoretical - the last core change I know about that changed a #type seems like it probably broke Context module (see #1346760-38: Add a scalable weight select element).

(Heh @ I worked on that patch too.) So does this patch potentially break Context, or is that just an example? I think this patch has much narrower reach than the weight select element did. That one changed the element everywhere (though IMO the change was justified because of numerous seriously major performance bugs associated with it). This issue has less of a strong case, but the only change it's making to the machine name element itself is the transparent addition of support for prefixes and suffixes. Other than that, the API change is only to a specific form (the manage fields form).

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

xjm’s picture

Priority: Normal » Major
Status: Closed (fixed) » Active

Did this change notice ever get written? I don't see it in the summary.

RaulMuroc’s picture

Issue summary: View changes
Status: Active » Needs work
filijonka’s picture

Version: 7.x-dev » 8.x-dev
Issue summary: View changes
Issue tags: +Missing change record
David_Rothstein’s picture

Wouldn't any change record here be for Drupal 7 rather than Drupal 8?

xjm’s picture

Version: 8.x-dev » 7.x-dev

We need a change record for any change that has a BC break. This needed one as of #57; it just got moved back to D7 for the backport at the same time. However, since there is no BC break with the latest Drupal 7 to Drupal 8 for this issue, we can put "7.x" in the change record's branch field. Thanks @filijonka!

minneapolisdan’s picture

Issue summary: View changes
SeanA’s picture

There seems to be a minor glitch when adding a new field and changing the machine name...
1) Enter a 'Label' in the 'Add new field' row.
2) Edit the automatically generated machine name.
3) Drag the 'Add new field' row to another location.
The machine name disappears! It does save properly, but before that you can't see what you entered or edit it again.

walangitan’s picture

@SeanA, I am able to reproduce this bug. Looks like there's an issue with the javascript of the table drag. Should we create a new issue instead to track that as it seems to be unrelated to most of the discussion here? I am working on a resolution to the issue, but not sure if this should be in it's own separate issue as this just relates to improving the UX for the machine names of fields, which was closed and reopened primarily for a change record.

SeanA’s picture

  • catch committed d444202 on 8.3.x
    Issue #1164812 by Niklas Fiekas, xjm: Improve UX for machine names for...

  • catch committed d444202 on 8.3.x
    Issue #1164812 by Niklas Fiekas, xjm: Improve UX for machine names for...
Sutharsan’s picture

Issue tags: -Novice

Removing Novice tag. I'Il don't consider issue summary and change record of a 100+ issue as novice.

  • catch committed d444202 on 8.4.x
    Issue #1164812 by Niklas Fiekas, xjm: Improve UX for machine names for...

  • catch committed d444202 on 8.4.x
    Issue #1164812 by Niklas Fiekas, xjm: Improve UX for machine names for...

  • catch committed d444202 on 9.1.x
    Issue #1164812 by Niklas Fiekas, xjm: Improve UX for machine names for...