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
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pwolanin’s picture

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

Here's a 1st pass

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun 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
CreditAttribution: sun 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 CreditAttribution: 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 CreditAttribution: pwolanin commented
Issue summary: View changes
Status: Needs work » Needs review
FileSize
increment.txt3.21 KB
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 CreditAttribution: pwolanin commented
Status: Needs work » Needs review
FileSize
increment.txt833 bytes
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
CreditAttribution: sun 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 CreditAttribution: 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 CreditAttribution: pwolanin commented
FileSize
2219009-12.patch47.94 KB

pretty trivial conflict in CoreServiceProvider.php

Here's a re-roll.

pwolanin’s picture

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

pwolanin CreditAttribution: 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 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

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

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
Status: Fixed » Needs review
FileSize
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
CreditAttribution: ianthomas_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 🇧🇪🇪🇺
CreditAttribution: Wim Leers 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 🇨🇦
CreditAttribution: webchick 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
CreditAttribution: fgm 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 🇨🇦
CreditAttribution: webchick 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 CreditAttribution: 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 CreditAttribution: ParisLiakos commented

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