Breaking off from #1002734-22: Multiple "Undefined index" notices in modules/field/modules/number/number.module
I have exhaustively searched for discussion as to why the current field system uses a space ' '
as the default for the thousand_separator
setting, to no avail.
I propose that the default should be changed to ''
(empty string) which, in the current code, is the first valid choice and ought to be the default.
Comment | File | Size | Author |
---|---|---|---|
#26 | before.png | 9.46 KB | dcam |
#26 | after.png | 8.5 KB | dcam |
#25 | thousand_separator-1275978-24.patch | 1.04 KB | cck |
#12 | thousand_separator-1275978-12.patch | 1.04 KB | Zgear |
#2 | thousand_separator-127598-2.patch | 1.34 KB | pillarsdotnet |
Comments
Comment #1
yoroy CreditAttribution: yoroy commentedI would support this proposal. A space is definately not a good default. Empty string would certainly be better.
Comment #2
pillarsdotnet CreditAttribution: pillarsdotnet commentedPatch.
Comment #3
pillarsdotnet CreditAttribution: pillarsdotnet commentedComment #4
Sheldon Rampton CreditAttribution: Sheldon Rampton commentedI support this also. I think in addition there needs to be a way to change the thousands separator via Drupal's web interface (perhaps under "Regional and language" configuration).
Comment #5
pillarsdotnet CreditAttribution: pillarsdotnet commentedTagging, since just about anybody should be able to mark this RTBC.
Comment #6
Sheldon Rampton CreditAttribution: Sheldon Rampton commentedUpon further review, I see that there is (in theory, at least) a way to change the thousands separator, by going to admin/structure/types/manage/{content-type}/display. However, the thousands separator configuration doesn't actually work on my D7 site (version 7.8). However, even after changing the configuration I still get a space instead of a comma when I try to create formatted output by invoking:
field_view_field('node', $job, 'field_assignment_hours')
Comment #7
pillarsdotnet CreditAttribution: pillarsdotnet commentedCan you open a separate bug report for the configuration issue?
Comment #8
Sheldon Rampton CreditAttribution: Sheldon Rampton commentedok, will do
Comment #9
pillarsdotnet CreditAttribution: pillarsdotnet commentedThank you. I see it here: #1292692: Thousands separator configuration not working
Comment #10
Zgear CreditAttribution: Zgear commentedThe patch no longer applies and needs to be rerolled. I'll take a look at it.
Comment #11
Zgear CreditAttribution: Zgear commentedComment #12
Zgear CreditAttribution: Zgear commentedComment #13
Zgear CreditAttribution: Zgear commentedComment #14
yoroy CreditAttribution: yoroy commentedI think I dare RTBC this just from reading the patch. Thank you Zgear!
Comment #15
chx CreditAttribution: chx commentedI am fine either way.
Comment #16
catchMakes sense to me, 1000 seems more common than 1 000. Committed/pushed to 8.x.
Comment #17
catchSorry folks, this actually got committed with #1077566: Convert html.tpl.php to HTML5 in http://drupalcode.org/project/drupal.git/commit/67c299d
Comment #18
pillarsdotnet CreditAttribution: pillarsdotnet commentedDo we need a change notification for this, as well?
Comment #19
catchHmm, might as well. It's a small change but there's a chance someone has a module overriding the default we'll no longer need or something.
Comment #20
xjmTagging.
Comment #21
willmoy CreditAttribution: willmoy commentedDraft at http://drupal.org/node/1388376 - needs review.
#6, I couldn't actually find the thousand separator in the interface where expected. Possibly a bug, possibly just me?
Comment #22
xjmThanks, the change notice looks good to me. Maybe a followup issue for the other bit?
Comment #24
catchJust ran into Drupal 7, I think this is probably backportable.
Comment #25
cck CreditAttribution: cck commentedDrupal 7 patch ported from thousand_separator-1275978-12.patch
Comment #26
dcam CreditAttribution: dcam commentedI tested #25. It correctly changes the default thousands separator of the number field from a space to nothing. I added two fields, one before and one after the patch. The field display configurations are displayed in the images below.
I'm marking this as RTBC.
Comment #27
David_Rothstein CreditAttribution: David_Rothstein commentedIs this sentence in the change notification correct?
My impression is that it's not correct (and that changing the default will never affect any existing fields on the site because they will have already had their settings saved to the database). But I'm not sure if that's true under all circumstances.
If that sentence in the change notification is true, then I don't think we can backport this (at least not without an update function) since it would introduce inconsistencies on existing sites. If it's not true, seems like we can, but the change notification needs to be clarified?
Comment #27.0
David_Rothstein CreditAttribution: David_Rothstein commentedTrivial grammar correction: moved a comma.
Comment #28
mgiffordGreat that @cck's patch still applies nicely. Would be great to know if this function could be backported. At this point it seems like it can't.
Comment #29
dcam CreditAttribution: dcam commentedI really don't think that the sentence in the change record is correct. Like you said, existing fields should be pulling their settings from the database. I can't even imagine how an existing field would know about the change. If the default value were a config setting or something, then I could understand how it could happen, but it's not. It's a line in number_field_formatter_info().
For an existing field to get that setting, it would need to have a custom formatter that invokes hook_field_formatter_info() or calls number's implementation directly to find out what the default is. That seems kind of ludicrous.
I did test the patch just now on a site with an existing number field. The existing setting did not change after applying the setting. I realize we're more concerned that there might be some one-off case where the setting will change with the default, but I can't think of what that might be.
Comment #30
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedCommitted to 7.x - thanks!
Yeah after some testing I don't see how this could affect existing fields, only new ones. So I'm going to remove that sentence from the change record and also mark the change record as affecting Drupal 7 too.
Comment #32
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedChange notice updated: https://www.drupal.org/node/1388376/revisions/view/7031047/8983337