Problem/Motivation

#2219009: Improve DX of Settings class introduced some new static methods to the settings class to allow usage like Settings::get() rather than having to getInstance() all the time. However, these static methods just call out the instance methods so we have duplication that is not necessary. With a get, getSetting, getAll, and getAllSettings methods - all doing the same thing. Basically it looks like a mess to me.

Adding duplicate methods doing the same thing is not really a DX improvement :)

Proposed resolution

static class methods can still be used by the instance, so just remove the other dupe methods and simplify the class considerably.

Remaining tasks

User interface changes

None

API changes

Removal of dupe settings methods. Stick with the newer/better naming.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

damiankloip’s picture

Issue summary: View changes
dawehner’s picture

+++ b/core/lib/Drupal/Core/Site/Settings.php
@@ -29,6 +29,17 @@
+  function __construct(array $settings) {

public ... protected choose what you want.

damiankloip’s picture

FileSize
2.9 KB
461 bytes

Sure, thanks!

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

thanks damian, thats exactly what i meant, yes :)
damiankloip++

dawehner’s picture

This basically is like the original version but with static methods using the singleton.
Before this patch it was possible to reuse the settings class to some extend, now it is certainly not longer the case. The question is whether we are okay with that.

Previous issue. #2219009: Improve DX of Settings class

ParisLiakos’s picture

i left a comment to #2219009: Improve DX of Settings class lets wait a bit before committing this in case we missed something

sun’s picture

Issue tags: +API clean-up

Looks good to me.

The approach taken here is essentially the same as the one I used for the Site class in #1757536: Move settings.php to /settings directory, fold sites.php into settings.php

The Settings class is architecturally in a really sad state right now, because it's neither a proper singleton (as it originally intended to be), nor is it a proper service (because it is a dependency of the kernel; i.e., Settings is required to boot a kernel, and only the kernel builds a service container).

However, this patch just removes some unnecessary ballast and doesn't make the overall situation any better or worse.


FWIW, meanwhile my ultimate plan is to remove the singleton concept entirely from Settings, and instead, make all front controllers properly inject a Settings instance into DrupalKernel.

The full dependency graph is: Site ← Settings ← DrupalKernel

sun’s picture

sun’s picture

Title: Untangle duplicate mess in Drupal\Tests\Core\Site\Settings » Remove duplicate methods from Settings class

Less aggro title ;-)

damiankloip’s picture

#5 what do you mean? This patch means the class can be used in the same way as before. Just without the duplicate methods.

sun’s picture

I also wasn't able to make sense of #5. Re-using the Settings class was never possible, because that's the primary (intended) constraint of the singleton pattern. Even though the pattern is not fully/correctly implemented, that constraint does exist for Settings.

Needless to say, there are much better classes available for re-use/abuse; e.g., Symfony's ParameterBag - which is Settings in green, sans singleton.

dawehner’s picture

@sun
Here is the previous code:

<?php

/**
 * @file
 * Contains \Drupal\Component\Utility\Settings.
 */

namespace Drupal\Component\Utility;

/**
 * Read only settings that are initialized with the class.
 */
final class Settings {

  /**
   * Array with the settings.
   *
   * @var array
   */
  private $storage = array();

  /**
   * Singleton instance.
   *
   * @var \Drupal\Component\Utility\Settings
   */
  private static $instance;

  /**
   * Returns the settings instance.
   *
   * A singleton is used because this class is used before the container is
   * available.
   *
   * @return \Drupal\Component\Utility\Settings
   */
  public static function getSingleton() {
    return self::$instance;
  }

  /**
   * Constructor.
   *
   * @param array $settings
   *   Array with the settings.
   */
  function __construct(array $settings) {
    $this->storage = $settings;
    self::$instance = $this;
  }

  /**
   * Returns a setting.
   *
   * Settings can be set in settings.php in the $settings array and requested
   * by this function. Settings should be used over configuration for read-only,
   * possibly low bootstrap configuration that is environment specific.
   *
   * @param string $name
   *   The name of the setting to return.
   * @param mixed $default
   *   (optional) The default value to use if this setting is not set.
   *
   * @return mixed
   *   The value of the setting, the provided default if not set.
   */
  public function get($name, $default = NULL) {
    return isset($this->storage[$name]) ? $this->storage[$name] : $default;
  }

  /**
   * Returns all the settings. This is only used for testing purposes.
   *
   * @return array
   *   All the settings.
   */
  public function getAll() {
    return $this->storage;
  }

}

That was nearly reusable, compared to not at all now.

damiankloip’s picture

Why do we need this class to be re usable? Where would you re use this? :)

ParisLiakos’s picture

well, lets face it, this class is not reusable. its final and its storage its private. and also a pain in phpunit tests:P

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

  • Commit 367e1a8 on 8.x by catch:
    Issue #2250491 by damiankloip: Remove duplicate methods from Settings...

Status: Fixed » Closed (fixed)

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