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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

yoroy’s picture

I would support this proposal. A space is definately not a good default. Empty string would certainly be better.

pillarsdotnet’s picture

Status: Active » Needs review
FileSize
1.34 KB

Patch.

pillarsdotnet’s picture

Title: The thousand_separator for numeric field should default to '' (nothing) instead of ' ' (space). » The thousand_separator for numeric fields should default to '' (nothing) instead of ' ' (space).
Sheldon Rampton’s picture

I 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).

pillarsdotnet’s picture

Issue tags: +Novice

Tagging, since just about anybody should be able to mark this RTBC.

Sheldon Rampton’s picture

Upon 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')

pillarsdotnet’s picture

Can you open a separate bug report for the configuration issue?

Sheldon Rampton’s picture

ok, will do

pillarsdotnet’s picture

Zgear’s picture

Status: Needs review » Needs work

The patch no longer applies and needs to be rerolled. I'll take a look at it.

Zgear’s picture

Assigned: Unassigned » Zgear
Zgear’s picture

Zgear’s picture

Status: Needs work » Needs review
yoroy’s picture

Status: Needs review » Reviewed & tested by the community

I think I dare RTBC this just from reading the patch. Thank you Zgear!

chx’s picture

I am fine either way.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Makes sense to me, 1000 seems more common than 1 000. Committed/pushed to 8.x.

catch’s picture

pillarsdotnet’s picture

Do we need a change notification for this, as well?

catch’s picture

Title: The thousand_separator for numeric fields should default to '' (nothing) instead of ' ' (space). » Change notification for: The thousand_separator for numeric fields should default to '' (nothing) instead of ' ' (space).
Category: bug » task
Status: Fixed » Active

Hmm, 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.

xjm’s picture

Issue tags: +Needs change record

Tagging.

willmoy’s picture

Status: Active » Needs review

Draft 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?

xjm’s picture

Title: Change notification for: The thousand_separator for numeric fields should default to '' (nothing) instead of ' ' (space). » The thousand_separator for numeric fields should default to '' (nothing) instead of ' ' (space).
Status: Needs review » Fixed

Thanks, the change notice looks good to me. Maybe a followup issue for the other bit?

Status: Fixed » Closed (fixed)

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

catch’s picture

Version: 8.x-dev » 7.x-dev
Status: Closed (fixed) » Patch (to be ported)
Issue tags: +Needs backport to D7

Just ran into Drupal 7, I think this is probably backportable.

cck’s picture

Assigned: Zgear » cck
Status: Patch (to be ported) » Needs review
FileSize
1.04 KB

Drupal 7 patch ported from thousand_separator-1275978-12.patch

dcam’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
8.5 KB
9.46 KB

I 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.

before.png

after.png

I'm marking this as RTBC.

David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs review

Is this sentence in the change notification correct?

Similarly if your site or theme relies on the separator being a space, you may need to adjust it after upgrading.

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?

David_Rothstein’s picture

Issue summary: View changes

Trivial grammar correction: moved a comma.

mgifford’s picture

Assigned: cck » Unassigned
Issue summary: View changes

Great 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.

dcam’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs change record

I 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.

David_Rothstein’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +7.40 release notes, +7.40 release announcement

Committed 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.

David_Rothstein’s picture

Status: Fixed » Closed (fixed)

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