-
Notifications
You must be signed in to change notification settings - Fork 824
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
FIX Handle calling Deprecation::notice() before manifests are available #10556
FIX Handle calling Deprecation::notice() before manifests are available #10556
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.
Not a big fan of controlling code executions based on the error message attached to an exception, but I can live with it.
I would like us to explicitly catch BadMethodCallException
.
@@ -42,7 +44,7 @@ public function getManifest() | |||
); | |||
} | |||
if (empty($this->manifests)) { | |||
throw new BadMethodCallException("No injector manifests available"); | |||
throw new BadMethodCallException(self::NO_MANIFESTS_AVAILABLE); |
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.
Seems like this would be a good place to throw a custom exception. Maybe, we can look at doing something like in CMS5/CMS6.
// this can happen when calling Deprecation::notice() before manifests are available, i.e. | ||
// some of the code involved in creating the manifests calls Deprecation::notice() | ||
} else { | ||
throw $e; |
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.
Not sure about this, but I'm assuming that every exception rethrown here, will appear to be coming from Deprecation::notice()
.
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.
I don't believe so, I think it just throws the original $e
unmodified
php -r '
class A {
function f() {
throw new \Exception();
}
}
class B {
function f() {
try {
(new A)->f();
} catch (\Exception $e) {
throw $e;
}
}
}
class C {
function f() {
try {
(new B)->f();
} catch (\Exception $e) {
var_dump($e->getLine());
var_dump($e->getTraceAsString());
}
}
}
(new C)->f();
'
int(4)
string(108) "#0 Command line code(10): A->f()
#1 Command line code(19): B->f()
#2 Command line code(26): C->f()
#3 {main}"
a3fe1d1
to
897f990
Compare
@maxime-rainville Have made update, CI is green |
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.
I'm assuming since this is related to the manifest, it's something that can't really be tested.
Issue silverstripe/silverstripe-assets#521