Same as #2085867: Add a default parameter to keyvalue get() method, but for config. So we would have things like \Drupal::config('my_settings')->get('some_key', 'default value');

Files: 
CommentFileSizeAuthor
#3 2090751-3.patch2.39 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#3 interdiff-2090751-3.txt1.48 KBdamiankloip
#1 d8.config-get-default.patch2.35 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

Comments

damiankloip’s picture

FileSize
2.35 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

Status: Needs review » Needs work

The last submitted patch, d8.config-get-default.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
1.48 KB
2.39 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

Hmm, tis ok locally. Adding another assertion to test get() without any key too.

Berdir’s picture

Component: base system » configuration system

Why is this needed?

Status: Needs review » Needs work

The last submitted patch, 2090751-3.patch, failed testing.

damiankloip’s picture

Status: Needs work » Closed (won't fix)

THis has not good a good reception, Let's just close it.

xjm’s picture

Issue tags: +Configuration system
ianthomas_uk’s picture

Status: Closed (won't fix) » Active

Reopening for the discussion of how to handle defaults for config, on the assumption that defaults for state will be passed as a second parameter.

At the moment, defaults are often set using :?

\Drupal::config('my_settings')->get('some_key') ?: 'default value';

This will creates confusion for developers between state and config, as they will use different methods

\Drupal::config('my_settings')->get('some_key') ?: 'default value'; // correct
\Drupal::state()->get('some_key', 'default value');                         // correct
\Drupal::config('my_settings')->get('some_key', 'default value');   // No default supplied - will default to null

I believe this is also an unintentional change from variable_get in 7.x which fell back to the default using isset(). The PHP ternary operator uses empty(), which means you can't have a boolean default to TRUE.

\Drupal::config('my_settings')->get('some_key') ?: TRUE; // Will return TRUE whatever 'some_key' is set to.

Both of these could be fixed by enforcing schemas that supplied defaults (so defaults would never be set inline), but I don't think that's currently the plan.

tim.plunkett’s picture

Status: Active » Closed (won't fix)

Let's use real examples, because none of those are legitimate.

  1. core/modules/block/lib/Drupal/block/Tests/BlockTest.php
    181:    $default_theme = \Drupal::config('system.theme')->get('default') ?: 'stark';
    
  2. core/modules/color/color.module
    135:  return \Drupal::config('color.' . $theme)->get('palette') ?: $palette;
    
  3. core/modules/menu/lib/Drupal/menu/Tests/MenuTest.php
    397:    $default_theme = \Drupal::config('system.theme')->get('default') ?: 'stark';
    
  4. core/modules/node/node.module
    705:  $submitted = \Drupal::config('node.type.' . $node->bundle())->get('settings.node.submitted') ?: TRUE;
    
  5. core/modules/system/system.install
    1945:  $front_page = \Drupal::config('system.site')->get('page.front') ?: 'node';
    
  6. core/modules/system/system.module
    225:      'description' => \Drupal::config('system.theme')->get('admin') ?: t('This is only used when the site is configured to use a separate administration theme on the <a href="@appearance-url">Appearance</a> page.', array('@appearance-url' => url('admin/appearance'))),
    
  7. core/modules/user/user.module
    421:    $role_permissions[$rid] = \Drupal::config("user.role.$rid")->get('permissions') ?: array();
    

Those are all of the places we use a ternary with config().

1) This is a test, and a lazy one at that.
2) This already has a @todo to remove it
3) This is also a test
4) This is a bug, being fixed in #2053461: Node type settings such as published state, promoted state, create revision and author information cannot be turned off
5) This is in system_update_8046() for enabling the views module :)
6) This is cheating, it is casting TRUE to an empty string for the description
7) This is only used during updating (in maintenance mode!)

Every time you call config you should be sure that it exists already. Defaults must be provided. Anywhere we need this pattern, or would need a default parameter, is a bug or a hack.

alexpott’s picture

What is said in #9 is correct doing this would be a massive step backwards for CMI.

ianthomas_uk’s picture

Agreed. The confusion came because there are still places where defaults are supplied inline, and the documentation is not currently clear enough about how the configuration system should be used, particularly when the config key will be dynamic (e.g. adding additional configuration per-content type). I plan to improve the docs after I've learnt how it all works through #2102521: Finish converting menu.module to CMI

alexpott’s picture

And lets get rid of 3 of those ternaries see #2112233: Remove unnecessary ternaries in around config()->get()