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.
Comment | File | Size | Author |
---|---|---|---|
#3 | interdiff-2250491-3.txt | 461 bytes | damiankloip |
#3 | 2250491-3.patch | 2.9 KB | damiankloip |
settings-untangle.patch | 2.89 KB | damiankloip | |
Comments
Comment #1
damiankloip CreditAttribution: damiankloip commentedComment #2
dawehnerpublic ... protected choose what you want.
Comment #3
damiankloip CreditAttribution: damiankloip commentedSure, thanks!
Comment #4
ParisLiakos CreditAttribution: ParisLiakos commentedthanks damian, thats exactly what i meant, yes :)
damiankloip++
Comment #5
dawehnerThis 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
Comment #6
ParisLiakos CreditAttribution: ParisLiakos commentedi left a comment to #2219009: Improve DX of Settings class lets wait a bit before committing this in case we missed something
Comment #7
sunLooks 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.phpThe
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 aSettings
instance intoDrupalKernel
.The full dependency graph is:
Site ← Settings ← DrupalKernel
Comment #8
sunComment #9
sunLess aggro title ;-)
Comment #10
damiankloip CreditAttribution: damiankloip commented#5 what do you mean? This patch means the class can be used in the same way as before. Just without the duplicate methods.
Comment #11
sunI 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 forSettings
.Needless to say, there are much better classes available for re-use/abuse; e.g., Symfony's
ParameterBag
- which isSettings
in green, sans singleton.Comment #12
dawehner@sun
Here is the previous code:
That was nearly reusable, compared to not at all now.
Comment #13
damiankloip CreditAttribution: damiankloip commentedWhy do we need this class to be re usable? Where would you re use this? :)
Comment #14
ParisLiakos CreditAttribution: ParisLiakos commentedwell, lets face it, this class is not reusable. its final and its storage its private. and also a pain in phpunit tests:P
Comment #15
catchCommitted/pushed to 8.x, thanks!