@RobinX, having getUser() return null is expected/legal when there is no logged in user (ex: public page)
However in that specific situation, there's a bug causing the user to not be defined but no exception is thrown. So having a check there would be nice. Cheers, Vincent On 02/04/2015 01:23 PM, Bernhard Posselt wrote: > Also try to stick with one return type or null. Some methods Ive seen > return booleans, objects and null which is just insane and would never > work with a statically typed language > > On 02/04/2015 01:21 PM, Robin McCorkell wrote: >> Hi, >> >> I feel as though we are in limbo with regards to exceptions. Some code uses >> exceptions to report an error, while others return null or false. I suspect >> this is one of the reasons why these situations have occurred. >> >> It is my opinion that a function should only return values that make sense. >> If a function is named getUser, it should only return the user, and if it >> can't that is clearly an error situation where an exception should be >> thrown. On the other hand, a function like getListOfThings could viably >> return false if there is nothing to get (unless those things are expected >> to be there!). >> >> A change to a fully exception based system won't happen overnight, but it'd >> be nice to see it promoted more in the code base. >> >> Just my thoughts, >> Robin McCorkell >> On 4 Feb 2015 12:14, "Vincent Petry" <pvinc...@owncloud.com> wrote: >> >>> Hello, >>> >>> After debugging several bugs, I recently found that many pieces of the >>> core code is assuming that a function call will always return the >>> expected value. >>> >>> There were many scenarios where a given bug was causing \OCP::getUser() >>> to return null but the code would carry on working with that null value, >>> and cause side-effects: >>> >>> https://github.com/owncloud/core/blob/stable7/apps/files_encryption/hooks/hooks.php#L438 >>> >>> I suggest that from now on we try and code more defensively. >>> This means that we want to check for correct values more often. >>> For example when the current user is needed, check for "null" afterwards >>> and throw an exception / log a warning instead of letting the code >>> continue. >>> >>> Here is another example: https://github.com/owncloud/core/issues/13862 >>> And here when `getRelativePath()` fails, it is not a normal situation, >>> we need to throw an exception instead of continuing: >>> >>> https://github.com/owncloud/core/blob/master/lib/private/files/view.php#L792 >>> >>> What do you think ? >>> >>> I have raised https://github.com/owncloud/core/issues/13885 with a list >>> of cases that can be fixed. If you know of other places in the code that >>> can be improved that way, please add them in the checklist (and/or >>> submit a PR to fix them) >>> >>> Thanks, >>> >>> Vincent >>> >>> >>> _______________________________________________ >>> Devel mailing list >>> Devel@owncloud.org >>> http://mailman.owncloud.org/mailman/listinfo/devel >>> >>> >> >> >> _______________________________________________ >> Devel mailing list >> Devel@owncloud.org >> http://mailman.owncloud.org/mailman/listinfo/devel > > _______________________________________________ > Devel mailing list > Devel@owncloud.org > http://mailman.owncloud.org/mailman/listinfo/devel >
signature.asc
Description: OpenPGP digital signature
_______________________________________________ Devel mailing list Devel@owncloud.org http://mailman.owncloud.org/mailman/listinfo/devel
