Fix some method signatures so that arguments with default values are at the end of the argument list (see D12418).
Details
Details
- Reviewers
hach-que epriestley - Group Reviewers
Blessed Reviewers - Commits
- Restricted Diffusion Commit
rPf3d382cec4d3: Fix some method signatures
Eyeballed the callsites.
Diff Detail
Diff Detail
- Repository
- rP Phabricator
- Branch
- master
- Lint
Lint Passed - Unit
Tests Passed - Build Status
Buildable 5932 Build 5952: [Placeholder Plan] Wait for 30 Seconds
Event Timeline
| src/applications/drydock/blueprint/DrydockBlueprintImplementation.php | ||
|---|---|---|
| 355–356 | These nulls are required to allow null to be passed into the first two parameters, otherwise you'll get an exception. It's probably better to just make $message nullable. | |
| src/applications/phragment/storage/PhragmentFragment.php | ||
| 80 | Again, optional null is required here due to type checks. | |
| src/applications/phragment/storage/PhragmentFragment.php | ||
|---|---|---|
| 80 | I don't quite follow... from what I can tell this function is always called with > 2 arguments. | |
| src/applications/phragment/storage/PhragmentFragment.php | ||
|---|---|---|
| 80 | It's always called with two arguments, but the second argument may be null (as per the $file === null check for directories). In PHP if you specify the type of a parameter, and it does not have an optional specifier, then that argument can not be null. If you pass null into it, you'll get a type check failure (because null is not of the required type). To allow null values to be passed into type checked parameters you need to add the optional specifier = null. | |
| src/applications/phragment/storage/PhragmentFragment.php | ||
|---|---|---|
| 80 | Ah. I did not know this. Based on this, I wonder if ArcanistXHPASTLinter::LINT_DEFAULT_PARAMETER is valid? Maybe it should be relaxed. | |
| src/applications/phragment/storage/PhragmentFragment.php | ||
|---|---|---|
| 80 | Yeah this PHP syntax caught me out a few times, especially since KDevelop (the IDE I use) highlights non-optional parameters after optional parameters as an error (even though it's perfectly fine in PHP). | |
Comment Actions
I think it's fine/desirable to require all arguments to have defaults after the first argument with a default. The handful of cases where we violate this are bizarre and probably better written differently.
| src/applications/people/storage/PhabricatorUserLog.php | ||
|---|---|---|
| 89 | Does this definitely have no callsites where the parameter is null? | |
| src/applications/people/storage/PhabricatorUserLog.php | ||
|---|---|---|
| 89 | Hmm, it possibly does actually... I guess $request->getUser() could return null | |