-
Notifications
You must be signed in to change notification settings - Fork 10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DEP Use stable version of silverstripe/supported-modules #246
DEP Use stable version of silverstripe/supported-modules #246
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, will merge when other PRs are all good to go
eb63b99
to
ccead6d
Compare
ccead6d
to
9c828fc
Compare
$task = UpdatePackageInfoTask::create(); | ||
$oldVersionProvider = Injector::inst()->get(VersionProvider::class); | ||
try { | ||
$task = UpdatePackageInfoTask::create(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is to fix a long standing broken unit test when running sink with a forked framework version
The new try/finally block is to reset Injector::inst()->registerService($versionProvider, VersionProvider::class);
back from the mock VersionProvider, to the original VersionProvder. Looking through the sapphire teststate code it's not immediately clear to me whether this is required or not (quite possibly it's not and it resets itself between tests), however I don't want to risk causing a regression later on in a subsequent unit test
Looking through the teststate stuff, it's not immediately clear to me whether or not resetting the
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM though should probably have been two very separate commits. Doesn't matter enough to ping pong it.
Issue silverstripe/silverstripe-framework#11428