Change singleton and strong_create to use dependency injector, with additional fixes to existing classes to behave correctly #487
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…viously a bug/feature in singleton, where it would pass null,true as params to strong_create, which would then get passed through as params to Versioned's constructor, meant that the code still executed fine (as was set to something that wasn't an array, so the null and true were instead taken as args). The fact that the usage of singleton(Versioned) never really used the classes code, purely for value lookup, meant that this never propagated errors. I've now switched singleton() to use the injector for retrieving values, which means these dud values are no longer passed through CHANGE Given that Config::inst is an implementation of the singleton pattern itself, I've removed the extra call to singleton(). A side effect of this is that it gets around a possibly nasty circular reference with the dependency injector (which relies on the config object); in future, this dependency structure should really be structured from the DI directly. MINOR Change singleton and strong_create to use dependency injector BUGFIX: Provide default constructor values for classes (fixes issues when used in 'singleton' scenario during dev/build in particular) MINOR Clear out injector state when resetting db schema during tests (a follow on from changing singleton() calls to use the injector underneath)
sminnee
added a commit
that referenced
this pull request
May 24, 2012
Change singleton and strong_create to use dependency injector, with additional fixes to existing classes to behave correctly
bergice
pushed a commit
to open-sausages/silverstripe-framework
that referenced
this pull request
Nov 18, 2018
…/fix-loading-height FIX Ensure FormBuilder loading indicator has a minimum height of the image
sabina-talipova
pushed a commit
to creative-commoners/silverstripe-framework
that referenced
this pull request
Oct 18, 2022
…ump-intervention DEP Bump minimum version of intervention/image
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a followup to the previous pull request with additional default constructor values added; seems that running the tests in a slightly different config weren't executing the same code paths (running an individual test case will pass on its own, but as part of /all it'll fail - something to do with the build bootstrapping process - go figure). I have seen a couple of failures running tests/all (to do with grid field I think it was) but these are failing before I applied the below patch, so I'm assuming those errors are exclusive of this code.
BUGFIX Versioned's constructor doesn't provide suitable defaults. Previously a bug/feature in singleton, where it would pass null,true as params to strong_create, which would then get passed through as params to Versioned's constructor, meant that the code still executed fine (as was set to something that wasn't an array, so the null and true were instead taken as args). The fact that the usage of singleton(Versioned) never really used the classes code, purely for value lookup, meant that this never propagated errors. I've now switched singleton() to use the injector for retrieving values, which means these dud values are no longer passed through
CHANGE Given that Config::inst is an implementation of the singleton pattern itself, I've removed the extra call to singleton(). A side effect of this is that it gets around a possibly nasty circular reference with the dependency injector (which relies on the config object); in future, this dependency structure should really be structured from the DI directly.
MINOR Change singleton and strong_create to use dependency injector
BUGFIX: Provide default constructor values for classes (fixes issues when used in 'singleton' scenario during dev/build in particular)
MINOR Clear out injector state when resetting db schema during tests (a follow on from changing singleton() calls to use the injector underneath)