Add different storage status error codes managed by StoragedNotAvailableExc… #20729

jmaciasportela · 2015-11-25T11:00:53Z

…eption

related to #20599

Be able to know which kind of error when StoragedNotAvailableException is thrown

mention-bot · 2015-11-25T11:00:57Z

By analyzing the blame information on this pull request, we identified @icewind1991, @bartv2 and @bantu to be potential reviewers.

PVince81 · 2015-11-25T11:05:44Z

Cool 👍

Check it out @icewind1991 @Xenopathic

icewind1991 · 2015-11-25T13:35:03Z

apps/files_external/controller/storagescontroller.php

-				$e->getMessage()
-			);
+			switch ($e->getCode()) {
+				case 1:


Please use the constants in the case statements to

Just thought it might be good to have these constants directly in the StorageNotAvailableException. This way the apps/storages that want to throw the exception can directly use these constants while throwing.

Yes that would be better, also since OC_Mount_Config is slowly being depricated

jmaciasportela · 2015-11-25T15:48:45Z

working on it. Thank you all for your comments 👍

jmaciasportela · 2015-11-25T16:58:27Z

@PVince81 @icewind1991 @Xenopathic Please note that new exceptions has fixed exception code to be used directly on storage->setStatus method. I 'm not sure if this is the right way

icewind1991 · 2015-11-25T16:59:58Z

apps/files_external/controller/storagescontroller.php

@@ -244,7 +244,27 @@ protected function updateStorageStatus(StorageConfig &$storage) {
 		} catch (StorageNotAvailableException $e) {


Since the exceptions below extend this should this catch be at the bottom or otherwise this block catches all of them?

Yes! My fault ...

The good thing about deriving exceptions is that it allows us to do the logic within the exceptions themselves, rather than have multiple near-duplicate handlers. All of these catches are the same, so merge them all together into the StorageNotAvailableException catch, where the current hard-coded STATUS_ERROR is replaced with $e->getCode(). Since each exception has a different code (as you've defined) the correct status will be returned. And with less duplicate code 😄

Also, no need to translate a string containing only a placeholder. Just pass the string through directly.

jmaciasportela · 2015-11-26T07:27:55Z

Thank you @Xenopathic , very good explanations

RobinMcCorkell · 2015-11-26T11:45:25Z

@jmaciasportela NICE!

Just move the existing STATUS_* constants from OC_Mount_Config to StorageNotAvailableException, and this'll be good to go 😄

jmaciasportela · 2015-11-26T16:53:36Z

@Xenopathic it feels little strange for me, something like this:

$newStorage->setStatus(StorageNotAvailableException::STATUS_SUCCESS);

RobinMcCorkell · 2015-11-26T21:49:02Z

@jmaciasportela The alternative is to put the STATUS constants in \OCP\Files\Storage, the storage interface itself. That might be a better idea?

PVince81 · 2015-11-30T13:04:27Z

Failing test:

± % ./autotest-external.sh sqlite sftp-atmoz                                                                                                                                                                                         !10121
Using database oc_autotest
Setup environment for sqlite testing ...
Installing ....
ownCloud is not installed - only a limited number of commands are available
creating sqlite db
ownCloud was successfully installed
{"reqId":"fA5I7mMMR0WkAHJ08Vv6","remoteAddr":"","app":"PHP","message":"touch(): Unable to create file \/srv\/www\/htdocs\/owncloud\/data\/owncloud.log because Permission denied at \/srv\/www\/htdocs\/owncloud\/lib\/private\/log\/owncloud.php#53","level":3,"time":"2015-11-30T13:03:43+00:00"}
Testing with sqlite ...
Run only sftp-atmoz ...
files_external enabled
No coverage
PHPUnit 4.3.5 by Sebastian Bergmann.

Configuration read from /srv/www/htdocs/owncloud/tests/phpunit-autotest-external.xml

.............PHP Fatal error:  Cannot use OCP\Files\StorageNotAvailableException as StorageNotAvailableException because the name is already in use in /srv/www/htdocs/owncloud/apps/files_external/controller/storagescontroller.php on line 41
PHP Stack trace:
PHP   1. {main}() /home/vincent/.composer/vendor/phpunit/phpunit/phpunit:0
PHP   2. PHPUnit_TextUI_Command::main() /home/vincent/.composer/vendor/phpunit/phpunit/phpunit:56
PHP   3. PHPUnit_TextUI_Command->run() /home/vincent/.composer/vendor/phpunit/phpunit/src/TextUI/Command.php:138
PHP   4. PHPUnit_TextUI_TestRunner->doRun() /home/vincent/.composer/vendor/phpunit/phpunit/src/TextUI/Command.php:186
PHP   5. PHPUnit_Framework_TestSuite->run() /home/vincent/.composer/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:423
PHP   6. PHPUnit_Framework_TestSuite->run() /home/vincent/.composer/vendor/phpunit/phpunit/src/Framework/TestSuite.php:751
PHP   7. PHPUnit_Framework_TestCase->run() /home/vincent/.composer/vendor/phpunit/phpunit/src/Framework/TestSuite.php:751
PHP   8. PHPUnit_Framework_TestResult->run() /home/vincent/.composer/vendor/phpunit/phpunit/src/Framework/TestCase.php:711
PHP   9. PHPUnit_Framework_TestCase->runBare() /home/vincent/.composer/vendor/phpunit/phpunit/src/Framework/TestResult.php:643
PHP  10. OCA\Files_external\Tests\Controller\GlobalStoragesControllerTest->setUp() /home/vincent/.composer/vendor/phpunit/phpunit/src/Framework/TestCase.php:771
PHP  11. spl_autoload_call() /home/vincent/.composer/vendor/phpunit/phpunit/src/Framework/TestCase.php:40
PHP  12. OC\Autoloader->load() /home/vincent/.composer/vendor/phpunit/phpunit/src/Framework/TestCase.php:40
PHP  13. require_once() /srv/www/htdocs/owncloud/lib/autoloader.php:185
PHP  14. spl_autoload_call() /srv/www/htdocs/owncloud/lib/autoloader.php:41
PHP  15. OC\Autoloader->load() /srv/www/htdocs/owncloud/lib/autoloader.php:41

RobinMcCorkell · 2015-11-30T15:36:33Z

apps/files_external/controller/storagescontroller.php

@@ -38,6 +38,7 @@
 use \OCP\Files\StorageNotAvailableException;
 use \OCA\Files_External\Lib\InsufficientDataForMeaningfulAnswerException;
 use \OCA\Files_External\Service\BackendService;
+use \OCP\Files\StorageNotAvailableException;


Duplicate (causes the unit test errors @PVince81 saw)

PVince81 · 2015-12-01T16:03:05Z

Is this still WIP ? If not, please rename the ticket.

Seems the unit test ran now.

Ready for review ?

PVince81 · 2015-12-01T16:03:51Z

lib/public/files/extstorageconnectionexception.php

+namespace OCP\Files;
+
+/**
+ * External Storage authentication exception


PHP doc doesn't match

PVince81 · 2015-12-01T16:04:40Z

I'd probably have called the exceptions "StorageXXXException" instead of "ExtStorageXXException". But fine by me.

Please fix the exception's PHPDoc then this is good to do.

👍

jmaciasportela · 2015-12-01T16:28:11Z

@PVince81 fixed and exceptions was renamed.

PVince81 · 2015-12-01T16:34:58Z

Thanks. Let's see what CI says

PVince81 · 2015-12-02T09:59:22Z

👍

@icewind1991 @Xenopathic @DeepDiver1975 can this be merged ?

icewind1991 · 2015-12-02T12:42:23Z

👍 looks good

(needs a rebase)

RobinMcCorkell · 2015-12-02T13:27:52Z

👍 🚀

…eption

MorrisJobke · 2015-12-03T09:00:39Z

Let me fix this broken rebase.

MorrisJobke · 2015-12-03T09:02:36Z

Voila ... only the good parts of the failed rebase :)

jmaciasportela · 2015-12-03T09:03:31Z

Thank you @MorrisJobke but tell me how, I hope not to broke it again

MorrisJobke · 2015-12-03T09:55:16Z

@Xenopathic @icewind1991 @PVince81 Can one of you have a short look at the outcome. I compared the diff and it was identical but I want to be sure. Then this can be merged.

PVince81 · 2015-12-03T10:27:37Z

👍 looks good

Add different storage status error codes managed by StoragedNotAvailableExc…

DeepDiver1975 added the in progress label Nov 25, 2015

jmaciasportela added this to the 9.0-current milestone Nov 25, 2015

jmaciasportela mentioned this pull request Nov 25, 2015

Able to show different errors on storage exceptions #20599

Closed

jmaciasportela added the 3 - To Review label Nov 25, 2015

icewind1991 reviewed Nov 25, 2015
View reviewed changes

jmaciasportela added 2 - Developing and removed 3 - To Review labels Nov 25, 2015

icewind1991 reviewed Nov 25, 2015
View reviewed changes

jmaciasportela added 3 - To Review and removed 2 - Developing labels Nov 26, 2015

RobinMcCorkell reviewed Nov 30, 2015
View reviewed changes

jmaciasportela mentioned this pull request Dec 1, 2015

External Storage Interface: Trigering user username and password Entry #16302

Closed

PVince81 reviewed Dec 1, 2015
View reviewed changes

PVince81 changed the title ~~[WIP] Add different storage status error codes managed by StoragedNotAvailableExc…~~ Add different storage status error codes managed by StoragedNotAvailableExc… Dec 2, 2015

Jesús Macias added 8 commits December 3, 2015 09:57

Add different storage status codes managed by StoragedNotAvailableExc…

b17f268

…eption

Add new external storage exceptions for different errors

7cd13f4

Delete unnecessary storage status constans

332c674

Change exception catch order

9d1ec02

Fix code from PR comments

4ead2bc

Migrate storate status code to storagenotavailablexception

98f5c50

Fix duplicate use in storage controller

f4f53e3

Fix code from comments

9dd1109

MorrisJobke force-pushed the issue_20599 branch from ac49126 to 9dd1109 Compare December 3, 2015 09:02

jmaciasportela added 4 - To release and removed 3 - To Review in progress labels Dec 3, 2015

DeepDiver1975 added a commit that referenced this pull request Dec 3, 2015

Merge pull request #20729 from owncloud/issue_20599

e62b6c1

Add different storage status error codes managed by StoragedNotAvailableExc…

DeepDiver1975 merged commit e62b6c1 into master Dec 3, 2015

DeepDiver1975 deleted the issue_20599 branch December 3, 2015 11:30

lock bot locked as resolved and limited conversation to collaborators Aug 8, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add different storage status error codes managed by StoragedNotAvailableExc… #20729

Add different storage status error codes managed by StoragedNotAvailableExc… #20729

jmaciasportela commented Nov 25, 2015

mention-bot commented Nov 25, 2015

PVince81 commented Nov 25, 2015

icewind1991 Nov 25, 2015

PVince81 Nov 25, 2015

icewind1991 Nov 25, 2015

jmaciasportela commented Nov 25, 2015

jmaciasportela commented Nov 25, 2015

icewind1991 Nov 25, 2015

jmaciasportela Nov 25, 2015

RobinMcCorkell Nov 25, 2015

RobinMcCorkell Nov 25, 2015

jmaciasportela commented Nov 26, 2015

RobinMcCorkell commented Nov 26, 2015

jmaciasportela commented Nov 26, 2015

RobinMcCorkell commented Nov 26, 2015

PVince81 commented Nov 30, 2015

RobinMcCorkell Nov 30, 2015

PVince81 commented Dec 1, 2015

PVince81 Dec 1, 2015

PVince81 commented Dec 1, 2015

jmaciasportela commented Dec 1, 2015

PVince81 commented Dec 1, 2015

PVince81 commented Dec 2, 2015

icewind1991 commented Dec 2, 2015

RobinMcCorkell commented Dec 2, 2015

MorrisJobke commented Dec 3, 2015

MorrisJobke commented Dec 3, 2015

jmaciasportela commented Dec 3, 2015

MorrisJobke commented Dec 3, 2015

PVince81 commented Dec 3, 2015

		@@ -244,7 +244,27 @@ protected function updateStorageStatus(StorageConfig &$storage) {
		} catch (StorageNotAvailableException $e) {

Add different storage status error codes managed by StoragedNotAvailableExc… #20729

Add different storage status error codes managed by StoragedNotAvailableExc… #20729

Conversation

jmaciasportela commented Nov 25, 2015

mention-bot commented Nov 25, 2015

PVince81 commented Nov 25, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmaciasportela commented Nov 25, 2015

jmaciasportela commented Nov 25, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmaciasportela commented Nov 26, 2015

RobinMcCorkell commented Nov 26, 2015

jmaciasportela commented Nov 26, 2015

RobinMcCorkell commented Nov 26, 2015

PVince81 commented Nov 30, 2015

Choose a reason for hiding this comment

PVince81 commented Dec 1, 2015

Choose a reason for hiding this comment

PVince81 commented Dec 1, 2015

jmaciasportela commented Dec 1, 2015

PVince81 commented Dec 1, 2015

PVince81 commented Dec 2, 2015

icewind1991 commented Dec 2, 2015

RobinMcCorkell commented Dec 2, 2015

MorrisJobke commented Dec 3, 2015

MorrisJobke commented Dec 3, 2015

jmaciasportela commented Dec 3, 2015

MorrisJobke commented Dec 3, 2015

PVince81 commented Dec 3, 2015