[Core: Config] Add type declarations to function signatures #4127
Conversation
johnsaigle
added
Category: Cleanup
php/libraries/NDB_Config.class.inc
Outdated
| 'While attempting to get Subproject config settings, caught ' . | ||
| "DatabaseException: `$e`" | ||
| ); | ||
| return array(); |
There was a problem hiding this comment.
This return is unreachable.. but other than that, what's the point of this change?
There was a problem hiding this comment.
I'll remove the unreachable return.
I thought it would be better not to silence the DB exception because it probably means something went wrong. Returning null to me makes it seem like there was no entry for $subprojectID in the table -- but in that case there shouldn't be an exception thrown; pselectRow should just return null.
There was a problem hiding this comment.
In that case why not just remove the try/catch? If the database throws an exception and it isn't caught the stack trace will tell you where it came from.
| * @return mixed The value from the database, or null if nothing found. | ||
| */ | ||
| function getSettingFromDB($name, $id=null) | ||
| function getSettingFromDB(string $name, ?int $id = null) |
There was a problem hiding this comment.
Given the description in the return doc, couldn't the return type be stated as ?array ?
There was a problem hiding this comment.
I can't remember when I added the FIXME above but no, this can return array, string or null
There was a problem hiding this comment.
the doc block can be updated to indicate a sum type even if the function signature can't, phan might be able to use the slightly tighter bounds to track down a few more bugs from the doc than "mixed".. but I'll merge this as is, since it's relatively minor.
php/libraries/NDB_Config.class.inc
Outdated
| * @return NDB_Config object | ||
| */ | ||
| static function &singleton($configFile = null) | ||
| static function &singleton(?string $configFile = null) |
There was a problem hiding this comment.
is there a NDB_Config object class that can be declared in the function declared type here? e.g. : \NDB_Config
php/libraries/NDB_Config.class.inc
Outdated
| * @param string $name The name of the XML node to retrieve. | ||
| * | ||
| * @return string The value from the config.xml | ||
| * @return mixed The value from the config.xml |
There was a problem hiding this comment.
instead of mixed, would it be better to state the potential types that can be returned?
johnsaigle
added
the
State: Needs work
johnsaigle
removed
the
State: Needs work
5 participants
See #3878 for background