-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Handle invalid storage when getting storage root id #29210
Conversation
return (int)$this->getStorage()->getCache()->getId(''); | ||
$storage = $this->getStorage(); | ||
if ($storage !== null && !$this->invalidStorage) { | ||
return (int)$storage->getCache()->getId(''); |
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.
test if cache is null???
7e9f8c5
to
9e9c96c
Compare
@DeepDiver1975 I've adjusted it: now it also checks if the cache is null. As a bonus, I also added unit tests and squashed. |
$cache = $storage->getCache(); | ||
|
||
if ($cache === null) { | ||
return -1; |
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.
Is it worthy to provide different value to mark different errors, such as -1 = storage not found, -2 invalid = storage? Use constants?
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 at this point... this method is only called in one location currently and doesn't care what the error is about
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.
Ok, fair enough.
Could you add the information in the PHPDoc? Otherwise -1 would be a valid storage id that wouldn't need special handling.
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've now adjusted the PHPDoc and also the one from IMountPoint to mention the -1 value
9e9c96c
to
e75950e
Compare
stable10: #29278 |
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Description
Whenever an invalid storage is present like a remote storage with invalid certificate, the mount point handling code will fail hard because the storage could not be instantiated.
Steps here: #28763 (comment)
Analysis here: #28763 (comment)
Related Issue
#28763
Motivation and Context
How Has This Been Tested?
See steps
Screenshots (if appropriate):
Types of changes
Checklist: