Updated: Comment #6

Problem/Motivation

Settings::getSingleton()->get('foo'); is far uglier and less intuitive than it could be.

Proposed resolution

Replace with:
Settings::get('foo');

Similarly provide Settings::getAll(); and rename the ->get() instance method.

Remaining tasks

convert existing code

User interface changes

N/A

API changes

changes the method for getting settings - remove settings() function and only use static methods on the class.

CommentFileSizeAuthor
#17 2219009-17.patch617 bytespwolanin
#12 2219009-12.patch47.94 KBpwolanin
#8 2219009-8.patch48.03 KBpwolanin
#8 increment.txt833 bytespwolanin
#6 2219009-6.patch48.03 KBpwolanin
#6 increment.txt3.21 KBpwolanin
#1 2219009-1.patch47.76 KBpwolanin

Comments

pwolanin’s picture

pwolanin commented
Status: Active » Needs review
Issue tags: +DX (Developer Experience)
StatusFileSize
new 2219009-1.patch47.76 KB

Here's a 1st pass

sun’s picture

sun
Primary language German
Location Karlsruhe
commented 
  1. +++ b/core/lib/Drupal/Component/Utility/Settings.php
@@ -34,9 +34,37 @@
-  public static function getSingleton() {
+  public static function getInstance() {

    +1 — Thanks a ton for fixing this method name.

  2. +++ b/core/lib/Drupal/Component/Utility/Settings.php
@@ -34,9 +34,37 @@
   }
+  /**

    Minor: Missing newline :) 

  3. +++ b/core/lib/Drupal/Component/Utility/Settings.php
@@ -64,7 +92,7 @@ function __construct(array $settings) {
-  public function get($name, $default = NULL) {
+  public function getSetting($name, $default = NULL) {

@@ -74,7 +102,7 @@ public function get($name, $default = NULL) {
-  public function getAll() {
+  public function getAllSettings() {

    Let's make those methods private now.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Related issues: +#2199795: Make the Settings class prevent serialization of actual settings

Status: Needs review » Needs work

The last submitted patch, 1: 2219009-1.patch, failed testing.

pwolanin’s picture

pwolanin commented

@sun - the instance methods are still used in the unit test. Should we use the static methods there instead?

pwolanin’s picture

pwolanin commented
Issue summary: View changes
Status: Needs work » Needs review
StatusFileSize
new increment.txt3.21 KB
new 2219009-6.patch48.03 KB

This should fix the test fatal (missing use) and suns suggestions.

Status: Needs review » Needs work

The last submitted patch, 6: 2219009-6.patch, failed testing.

pwolanin’s picture

pwolanin commented
Status: Needs work » Needs review
StatusFileSize
new increment.txt833 bytes
new 2219009-8.patch48.03 KB

Hmm, turns out the instance is injected into a bunch of core services (e.g. string_translator.custom_strings)

Let's see if it passes with those methods left public.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Reviewed & tested by the community

Looks awesome to me.

FWIW, one idea raised in #2199795: Make the Settings class prevent serialization of actual settings is to no longer expose Settings as a service (because it cannot be serialized as part of other service instances), so those instances might go away. Just mentioning for completeness.

pwolanin’s picture

pwolanin commented

Yes, seems like there is more cleanup that could be tackled after this - having it as a service doesn't seem to make much sense as we are doing it: it's not generally accessed that way, so you can't swap it.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 8: 2219009-8.patch, failed testing.

pwolanin’s picture

pwolanin commented
StatusFileSize
new 2219009-12.patch47.94 KB

pretty trivial conflict in CoreServiceProvider.php

Here's a re-roll.

pwolanin’s picture

pwolanin commented
Status: Needs work » Needs review
pwolanin’s picture

pwolanin commented
Status: Needs review » Reviewed & tested by the community

ok, back to rtbc since it's passing all tests again.

  • Commit 000966e on 8.x by alexpott: 
    Issue #2219009 by pwolanin: Improve DX of Settings class.
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed 000966e and pushed to 8.x. Thanks!

pwolanin’s picture

pwolanin commented
Status: Fixed » Needs review
StatusFileSize
new 2219009-17.patch617 bytes

stupidly missed the use in the catch in index.php

catch (Exception $e) {
  $message = 'If you have just changed code (for example deployed a new module or moved an existing one) read <a href="http://drupal.org/documentation/rebuild">http://drupal.org/documentation/rebuild</a>';
  if (settings()->get('rebuild_access', FALSE)) {
    $rebuild_path = $GLOBALS['base_url'] . '/rebuild.php';
    $message .= " or run the <a href=\"$rebuild_path\">rebuild script</a>";
  }
ianthomas_uk’s picture

ianthomas_uk
he/him
Primary language English
Location Brighton, UK
commented

This needs CRs to be updated, especially https://drupal.org/node/1882698.
I'm working on them now.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Reviewed & tested by the community

Most trivial patch of the month award goes to pwolanin! :)

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed
Issue tags: +Needs followup

Since testbot obviously isn't testing this code path, I think it's fine to commit this before it's back. However, we should make a major issue to somehow add test coverage for this, because that's very un-good.

Meanwhile, committed and pushed to 8.x. Thanks!

fgm’s picture

fgm
Primary language French
Location Paris, France
commented

This commit does not seem to have made it to Git, even online https://drupal.org/node/3060/commits

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

Weird. Take 2!

  • Commit b9ef6be on 8.x by webchick: 
    Issue #2219009 follow-up by pwolanin: Replace missed call to settings().
damiankloip’s picture

damiankloip commented

So, just out of interest, how is it this normal task gets committed so quickly when other issues have been waiting much longer? :)

Status: Fixed » Closed (fixed)

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

ParisLiakos’s picture

ParisLiakos commented

follow up #2250491: Remove duplicate methods from Settings class
i dont really see the point of duplicating the methods