When adding a content-type with an integer field, I got the following:

  • Notice: Undefined index: scale in number_field_formatter_settings_summary() (line 265 of modules/field/modules/number/number.module).
  • Notice: Undefined index: thousand_separator in number_field_formatter_settings_summary() (line 265 of modules/field/modules/number/number.module).
  • Notice: Undefined index: prefix_suffix in number_field_formatter_settings_summary() (line 266 of modules/field/modules/number/number.module).
  • Notice: Undefined index: thousand_separator in number_field_formatter_settings_form() (line 229 of modules/field/modules/number/number.module).
  • Notice: Undefined index: prefix_suffix in number_field_formatter_settings_form() (line 251 of modules/field/modules/number/number.module).
  • Notice: Undefined index: scale in number_field_formatter_settings_summary() (line 265 of modules/field/modules/number/number.module).
  • Notice: Undefined index: decimal_separator in number_field_formatter_settings_summary() (line 265 of modules/field/modules/number/number.module).
  • Notice: Undefined index: scale in number_field_formatter_settings_summary() (line 265 of modules/field/modules/number/number.module).
  • Notice: Undefined index: decimal_separator in number_field_formatter_settings_summary() (line 265 of modules/field/modules/number/number.module).
  • Notice: Undefined index: thousand_separator in number_field_formatter_settings_summary() (line 265 of modules/field/modules/number/number.module).
  • Notice: Undefined index: prefix_suffix in number_field_formatter_settings_summary() (line 266 of modules/field/modules/number/number.module).

The attached patch fixes the problem as follows:

  1. Adds the following function:
    /**
     * Default number settings
     *
     * @param string $number_type
     *   The type of this number field, such as
     *   'number_integer' or 'number_decimal'.
     *
     * @return
     *   An array of default field settings.
     *
     * @todo Should be locale-dependent.
     */
    function number_settings_defaults($number_type) {
      static $common_defaults = array(  
        'thousand_separator' => ' ',
        'decimal_separator' => '.',
        'prefix_suffix' => TRUE,
      );
      return $common_defaults + ($number_type == 'number_integer') ?
        array('scale' => 0 ) :
        array('scale' => 2, 'precision' => 10);
    }
    
  2. Ensure that the defaults are added where appropriate.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

The last submitted patch, number.module.patch, failed testing.

pillarsdotnet’s picture

Status: Needs work » Needs review
FileSize
4.39 KB

Corrected...

Status: Needs review » Needs work

The last submitted patch, number.module.patch, failed testing.

pillarsdotnet’s picture

Status: Needs work » Needs review

#2: number.module.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, number.module.patch, failed testing.

pillarsdotnet’s picture

Status: Needs work » Needs review
FileSize
4.39 KB

If at first you don't succeed...

pillarsdotnet’s picture

FileSize
4.42 KB

Missed one; corrected.

klausi’s picture

I cannot reproduce this when I add a content type and an integer field. On what page do the notices occur?

pillarsdotnet’s picture

Happened when I add a content type an an integer field. Will try to reproduce on minimal install.

pillarsdotnet’s picture

Doesn't happen on minimal install with only "Field UI" and "Number" modules enabled.

Perhaps this is an entity bug? Still testing...

pillarsdotnet’s picture

Status: Needs review » Closed (works as designed)

Okay, I give up. After backing out the above patch, I can't reproduce either.

Will reopen if I find a reproducible test case.

VeeLin’s picture

If it helps I got this error in the following situation with :
1. go to Manage Display and change from Default to Unformatted under Format.
2. save
3. change back to default and the error disappearers.

Jarode’s picture

FileSize
103.13 KB
Notice : Undefined index: scale dans number_field_formatter_settings_summary() (ligne 265 dans /var/www/vhosts/myrmecofourmis.com/httpdocs/site/modules/field/modules/number/number.module).
Notice : Undefined index: decimal_separator dans number_field_formatter_settings_summary() (ligne 265 dans /var/www/vhosts/myrmecofourmis.com/httpdocs/site/modules/field/modules/number/number.module).
Notice : Undefined index: thousand_separator dans number_field_formatter_settings_summary() (ligne 265 dans /var/www/vhosts/myrmecofourmis.com/httpdocs/site/modules/field/modules/number/number.module).
Notice : Undefined index: prefix_suffix dans number_field_formatter_settings_summary() (ligne 266 dans /var/www/vhosts/myrmecofourmis.com/httpdocs/site/modules/field/modules/number/number.module).
yched’s picture

Status: Needs work » Closed (works as designed)

The 'Unformatted' formatter doesn't have any settings and therefore shouldn't return any 'settings summary'.

The correct fix is to wrap a if ($display['type'] == 'number_decimal' || $display['type'] == 'number_float') { check around the following lines in number_field_formatter_settings_summary() :

  $summary[] = number_format(1234.1234567890, $settings['scale'], $settings['decimal_separator'], $settings['thousand_separator']);
  if ($settings['prefix_suffix']) {
    $summary[] = t('Display with prefix and suffix.');
  }

Additionally, the same if ($display['type'] == ... check already exists in number_field_formatter_settings_form() but it needs to be moved just above the $options = array( line.

I'm not where I can roll a patch right now. Anyone ?

yched’s picture

Status: Closed (works as designed) » Needs work
uiexp’s picture

Status: Closed (works as designed) » Needs review

#7: number.module.patch queued for re-testing.

pillarsdotnet’s picture

Version: 7.x-dev » 8.x-dev
Status: Needs review » Needs work
Issue tags: +Needs backport to D7

#14 needs to be done for 8.x first, then backported.

Anonymous’s picture

I tried making the changes that yched suggested in #14, but that didn't solve the problem for me. However, the patch from #7 did solve the problem.

Maybe these are two separate issues that display the same behavior? For me, it is coming up when I use Views to display an integer.

amfis’s picture

For D7 that helped:

from (line: 264, number.module):

$summary[] = number_format(1234.1234567890, $settings['scale'], $settings['decimal_separator'], $settings['thousand_separator']);

  if ($settings['prefix_suffix']) {
    $summary[] = t('Display with prefix and suffix.');
  }

to:

  if (isset($settings['scale']) && isset($settings['decimal_separator']) && isset($settings['thousand_separator'])) {
    $summary[] = number_format(1234.1234567890, $settings['scale'], $settings['decimal_separator'], $settings['thousand_separator']);
  }
  
  if (isset($settings['prefix_suffix'])) {
    $summary[] = t('Display with prefix and suffix.');
  }
Rhodungeon’s picture

#19 woked perfectly in D7

pillarsdotnet’s picture

Status: Needs work » Needs review
FileSize
5.83 KB

Added ysched's suggestion in #14 to patch in #7, cleaned up slightly and re-rolled.

pillarsdotnet’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests

Incidentally, if anyone can point me to the discussion that led to the current decision to make the thousand_separator default to a space, I'd love to read it.

EDIT: Opened #1275978: The thousand_separator for numeric fields should default to '' (nothing) instead of ' ' (space).

yched’s picture

Status: Needs review » Needs work

- I don't favor the introduction of the number_settings_defaults() helper function.
- This function happily mixes field settings ('scale'...) and formatter settings ('thousands separator'), and adds settings that do not make sense ('scale' for integer fields ?)

I think #17 outlines the correct fixes, which are quite simple.

pillarsdotnet’s picture

Status: Needs work » Needs review
FileSize
3.12 KB

@ysched: Sure, whatever; you're obviously smarter than I am, but @linclark said he tried your fix and it didn't work.

Also, the change you suggest makes it impossible to specify a thousand_separator for integer numbers. Is that the intent?

Conversely, if we disallow thousand_separator for integer and unformatted numbers, shouldn't we also disallow prefix_suffix as well?

yched’s picture

@pillardotsnet : sorry for #23 sounding a bit harsh, I typed it quickly from my phone browser. Thanks for keeping up pounding on this issue.

Actually there's another error in number_field_formatter_settings_[form|summary]() : they reason on formatter being either 'number_decimal' or 'number_float' - and the latter doesn't exist, it should be 'number_integer' instead.
Attached patch is the same as #24, plus this additional fix.

Those fixes are the correct ones for me, and with those applied I haven't been able to reproduce the errors/warnings @linclark mentions in #18 while displaying an integer field in Views - nor do I see where they would come from. So, until @linclark or someone else provides more info on those, I'll stick to that approach.

I cannot RTBC this myself anymore, though :-/

pillarsdotnet’s picture

So in your proposed fix, $element['decimal_separator'] and $element['scale'] are only defined for type 'number_decimal':

+    if ($display['type'] == 'number_decimal') {
+      $element['decimal_separator'] = array(
+        '#type' => 'select',
+        '#title' => t('Decimal marker'),
+        '#options' => array('.' => t('Decimal point'), ',' => t('Comma')),
+        '#default_value' => $settings['decimal_separator'],
+      );
+      $element['scale'] = array(
+        '#type' => 'select',
+        '#title' => t('Scale'),
+        '#options' => drupal_map_assoc(range(0, 10)),
+        '#default_value' => $settings['scale'],
+        '#description' => t('The number of digits to the right of the decimal.'),
+      );
+    }

But they are referenced for both types 'number_decimal' and 'number_integer':

+  if ($display['type'] == 'number_decimal' || $display['type'] == 'number_integer') {
+    $summary[] = number_format(1234.1234567890, $settings['scale'], $settings['decimal_separator'], $settings['thousand_separator']);

Apologies for my ignorance, but please explain to me how this will not result in an undefined index error.

Also,

they reason on formatter being either 'number_decimal' or 'number_float' - and the latter doesn't exist, it should be 'number_integer' instead.

After applying your patch:

bobvin@bowie:~/www/8.x$ grep -rn number_float modules/
modules/field/modules/number/number.install:22:    case 'number_float' :
modules/field/modules/number/number.module:41:    'number_float' => array(
modules/field/modules/number/number.module:77:  if ($field['type'] == 'number_decimal' || $field['type'] == 'number_float') {
modules/field/modules/number/number.module:196:      'field types' => array('number_decimal', 'number_float'),
modules/field/modules/number/number.module:206:      'field types' => array('number_integer', 'number_decimal', 'number_float'),
modules/field/modules/number/number.module:316:      'field types' => array('number_integer', 'number_decimal', 'number_float'),
modules/field/modules/number/number.module:327:  if ($field['type'] == 'number_decimal' || $field['type'] == 'number_float') {

If 'number_float' doesn't exist, there is certainly a lot of cruft that needs cleaning up.

pillarsdotnet’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

Also, since our mutually contradictory patches are passing all tests, whatever fix is decided upon needs a corresponding test added.

yched’s picture

So in your proposed fix, $element['decimal_separator'] and $element['scale'] are only defined for type 'number_decimal':

No, 'decimal_separator' and 'scale' settings are defined for the 'number_integer' formatter too (see number_field_formatter_info()), they just have no UI in the form, so they will be present with their default values. Thus, referencing them will generate no warnings, they exist, just not changeable (at least not in the UI). I reckon this is not exactly intuitive, but this allows the formatter_view() and formatter_summary() code to be the same regardless of the field type (integer, decimal, float).

If 'number_float' doesn't exist, there is certainly a lot of cruft that needs cleaning up.

You're mixing field types and formatters. The 'number_float' field type exists, but there is no formatter named 'number_float'. float fields use the 'number_decimal' formatter.

So yes, the patch is correct. Updated patch just adds a comment in number_field_formatter_info() to make that a little clearer.

pillarsdotnet’s picture

Looks good; the only remaining task is to roll a test. I'll try to do that.

yched’s picture

The 'formatter settings' UI is not easily testable currently. That's because it is triggered by an image button, and those do not play well with our testing framework.

I guess this could be tested by programmatically creating a integer field and instance with the number_integer formatter, and just displaying the "Manage display" page (the test will pass by just triggering no warnings), but to be honest, I'm not sure a test is worth it.

pillarsdotnet’s picture

to be honest, I'm not sure a test is worth it.

Nevertheless, I'm writing one, and learning a lot in the process.

pillarsdotnet’s picture

Test fails without the patch; passes with it.

pillarsdotnet’s picture

yched’s picture

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

Getting back at this after a couple weeks away from my coding env.

Thanks for the tests ! Just slightly adjusted those (explicitly test setting the formatter to 'number_integer', so that the test does not depend on it being the default formatter).

@pillarsdotet validated the code, I validate the tests (with the minor adjustment above), so I guess this is RTBC.

chx’s picture

I guess so too :)

pillarsdotnet’s picture

Issue tags: -Needs tests

Removing "needs tests" tag.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x and 7.x. Thanks!

Status: Fixed » Closed (fixed)

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

acb’s picture

I am still seeing this error flooding watchdog. Has there been a regression somewhere?