Skip to content

Commit ce3d484

Browse files
neandrakeneandrake
committed
Addressing some PHP8 incompatibilities - Diffusion & Differential
Summary: Running through setting up and using Diffusion repositories and addressing PHP8 issues that come up. Test Plan: I set up a hosted mercurial repository and pushed commits to it over ssh. I set up a hosted git repository and pushed commits to it over ssh. I set up a mirrored mercurial repository over ssh. I set up a mirrored git repository over ssh. I created a diff on a git repository and landed it. I created a diff on a mercurial repository and landed it. I cloned and pushed a commit to a mercurial repo over http. I cloned and pushed a commit to a git repo over http. Reviewers: #blessed_reviewers, epriestley Reviewed By: #blessed_reviewers, epriestley Subscribers: Korvin, epriestley Differential Revision: https://secure.phabricator.com/D21864
1 parent 5899526 commit ce3d484

File tree

51 files changed

+151
-83
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

51 files changed

+151
-83
lines changed

‎scripts/ssh/ssh-auth.php

+2-2
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@
3636

3737
$authstruct = null;
3838

39-
if (strlen($authstruct_raw)) {
39+
if ($authstruct_raw !== null && strlen($authstruct_raw)) {
4040
try {
4141
$authstruct = phutil_json_decode($authstruct_raw);
4242
} catch (Exception $ex) {
@@ -135,7 +135,7 @@
135135

136136
$cmd = csprintf('%s %Ls', $bin, $key_argv);
137137

138-
if (strlen($instance)) {
138+
if ($instance !== null && strlen($instance)) {
139139
$cmd = csprintf('PHABRICATOR_INSTANCE=%s %C', $instance, $cmd);
140140
}
141141

‎src/applications/almanac/util/AlmanacKeys.php

+1-1
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,7 @@ public static function getLiveDevice() {
5858

5959
public static function getClusterSSHUser() {
6060
$username = PhabricatorEnv::getEnvConfig('diffusion.ssh-user');
61-
if (strlen($username)) {
61+
if ($username !== null && strlen($username)) {
6262
return $username;
6363
}
6464

‎src/applications/conduit/query/PhabricatorConduitSearchEngine.php

+1-1
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ public function buildQueryFromSavedQuery(PhabricatorSavedQuery $saved) {
3939
$query->withIsInternal(false);
4040

4141
$contains = $saved->getParameter('nameContains');
42-
if (strlen($contains)) {
42+
if ($contains !== null && strlen($contains)) {
4343
$query->withNameContains($contains);
4444
}
4545

‎src/applications/config/controller/module/PhabricatorConfigModuleController.php

+1-1
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ public function handleRequest(AphrontRequest $request) {
99

1010
$all_modules = PhabricatorConfigModule::getAllModules();
1111

12-
if (!strlen($key)) {
12+
if ($key === null || !strlen($key)) {
1313
$key = head_key($all_modules);
1414
}
1515

‎src/applications/differential/conduit/DifferentialGetCommitMessageConduitAPIMethod.php

+2-2
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@ protected function execute(ConduitAPIRequest $request) {
5656
// show "Field:" templates for some fields even if they are empty.
5757
$edit_mode = $request->getValue('edit');
5858

59-
$is_any_edit = (bool)strlen($edit_mode);
59+
$is_any_edit = $edit_mode !== null && (bool)strlen($edit_mode);
6060
$is_create = ($edit_mode == 'create');
6161

6262
$field_list = DifferentialCommitMessageField::newEnabledFields($viewer);
@@ -115,7 +115,7 @@ protected function execute(ConduitAPIRequest $request) {
115115

116116
$is_title = ($field_key == $key_title);
117117

118-
if (!strlen($value)) {
118+
if ($value === null || !strlen($value)) {
119119
if ($is_template) {
120120
$commit_message[] = $label.': ';
121121
}

‎src/applications/differential/customfield/DifferentialBranchField.php

+8-1
Original file line numberDiff line numberDiff line change
@@ -39,13 +39,20 @@ private function getBranchDescription(DifferentialDiff $diff) {
3939
$branch = $diff->getBranch();
4040
$bookmark = $diff->getBookmark();
4141

42+
if ($branch === null) {
43+
$branch = '';
44+
}
45+
if ($bookmark === null) {
46+
$bookmark = '';
47+
}
48+
4249
if (strlen($branch) && strlen($bookmark)) {
4350
return pht('%s (bookmark) on %s (branch)', $bookmark, $branch);
4451
} else if (strlen($bookmark)) {
4552
return pht('%s (bookmark)', $bookmark);
4653
} else if (strlen($branch)) {
4754
$onto = $diff->loadTargetBranch();
48-
if (strlen($onto) && ($onto !== $branch)) {
55+
if ($onto !== null && strlen($onto) && ($onto !== $branch)) {
4956
return pht(
5057
'%s (branched from %s)',
5158
$branch,

‎src/applications/differential/field/DifferentialCommitMessageField.php

+1-1
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,7 @@ public function readFieldValueFromObject(DifferentialRevision $revision) {
6060
}
6161

6262
public function renderFieldValue($value) {
63-
if (!strlen($value)) {
63+
if ($value === null || !strlen($value)) {
6464
return null;
6565
}
6666

‎src/applications/differential/field/DifferentialRevisionIDCommitMessageField.php

+1-1
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,7 @@ public function readFieldValueFromConduit($value) {
7272
}
7373

7474
public function renderFieldValue($value) {
75-
if (!strlen($value)) {
75+
if ($value === null || !strlen($value)) {
7676
return null;
7777
}
7878

‎src/applications/differential/storage/DifferentialChangeset.php

+1-1
Original file line numberDiff line numberDiff line change
@@ -325,7 +325,7 @@ public function getDiff() {
325325

326326
public function getOldStatePathVector() {
327327
$path = $this->getOldFile();
328-
if (!strlen($path)) {
328+
if ($path === null || !strlen($path)) {
329329
$path = $this->getFilename();
330330
}
331331

‎src/applications/differential/view/DifferentialRevisionUpdateHistoryView.php

+1
Original file line numberDiff line numberDiff line change
@@ -251,6 +251,7 @@ public function render() {
251251
$content = phabricator_form(
252252
$this->getUser(),
253253
array(
254+
'method' => 'GET',
254255
'action' => '/D'.$revision_id.'#toc',
255256
),
256257
array(

‎src/applications/diffusion/conduit/DiffusionBranchQueryConduitAPIMethod.php

+2-2
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ protected function getGitResult(ConduitAPIRequest $request) {
3030
$repository = $drequest->getRepository();
3131

3232
$contains = $request->getValue('contains');
33-
if (strlen($contains)) {
33+
if ($contains !== null && strlen($contains)) {
3434

3535
// See PHI958 (and, earlier, PHI720). If "patterns" are provided, pass
3636
// them to "git branch ..." to let callers test for reachability from
@@ -80,7 +80,7 @@ protected function getMercurialResult(ConduitAPIRequest $request) {
8080
->setRepository($repository);
8181

8282
$contains = $request->getValue('contains');
83-
if (strlen($contains)) {
83+
if ($contains !== null && strlen($contains)) {
8484
$query->withContainsCommit($contains);
8585
}
8686

‎src/applications/diffusion/conduit/DiffusionBrowseQueryConduitAPIMethod.php

+9-4
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ protected function getGitResult(ConduitAPIRequest $request) {
3737
$repository = $drequest->getRepository();
3838

3939
$path = $request->getValue('path');
40-
if (!strlen($path) || $path === '/') {
40+
if ($path === null || !strlen($path) || $path === '/') {
4141
$path = null;
4242
}
4343

@@ -282,8 +282,13 @@ protected function getMercurialResult(ConduitAPIRequest $request) {
282282

283283
$results = array();
284284

285-
$match_against = trim($path, '/');
286-
$match_len = strlen($match_against);
285+
if ($path !== null) {
286+
$match_against = trim($path, '/');
287+
$match_len = strlen($match_against);
288+
} else {
289+
$match_against = '';
290+
$match_len = 0;
291+
}
287292

288293
// For the root, don't trim. For other paths, trim the "/" after we match.
289294
// We need this because Mercurial's canonical paths have no leading "/",
@@ -295,7 +300,7 @@ protected function getMercurialResult(ConduitAPIRequest $request) {
295300
if (strncmp($path, $match_against, $match_len)) {
296301
continue;
297302
}
298-
if (!strlen($path)) {
303+
if ($path === null || !strlen($path)) {
299304
continue;
300305
}
301306
$remainder = substr($path, $trim_len);

‎src/applications/diffusion/conduit/DiffusionHistoryQueryConduitAPIMethod.php

+4-5
Original file line numberDiff line numberDiff line change
@@ -45,16 +45,15 @@ protected function getGitResult(ConduitAPIRequest $request) {
4545
$repository = $drequest->getRepository();
4646
$commit_hash = $request->getValue('commit');
4747
$against_hash = $request->getValue('against');
48+
$offset = $request->getValue('offset');
49+
$limit = $request->getValue('limit');
4850

4951
$path = $request->getValue('path');
50-
if (!strlen($path)) {
52+
if ($path === null || !strlen($path)) {
5153
$path = null;
5254
}
5355

54-
$offset = $request->getValue('offset');
55-
$limit = $request->getValue('limit');
56-
57-
if (strlen($against_hash)) {
56+
if ($against_hash !== null && strlen($against_hash)) {
5857
$commit_range = "{$against_hash}..{$commit_hash}";
5958
} else {
6059
$commit_range = $commit_hash;

‎src/applications/diffusion/controller/DiffusionBranchTableController.php

+1-1
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ public function handleRequest(AphrontRequest $request) {
2626
);
2727

2828
$contains = $drequest->getSymbolicCommit();
29-
if (strlen($contains)) {
29+
if ($contains !== null && strlen($contains)) {
3030
$params['contains'] = $contains;
3131
}
3232

‎src/applications/diffusion/controller/DiffusionBrowseController.php

+7-3
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ public function handleRequest(AphrontRequest $request) {
2222
// list.
2323

2424
$grep = $request->getStr('grep');
25-
if (strlen($grep)) {
25+
if (phutil_nonempty_string($grep)) {
2626
return $this->browseSearch();
2727
}
2828

@@ -290,6 +290,11 @@ public function browseDirectory(
290290
$header = $this->buildHeaderView($drequest);
291291
$header->setHeaderIcon('fa-folder-open');
292292

293+
$title = '/';
294+
if ($drequest->getPath() !== null) {
295+
$title = nonempty(basename($drequest->getPath()), '/');
296+
}
297+
293298
$empty_result = null;
294299
$browse_panel = null;
295300
if (!$results->isValidResults()) {
@@ -303,7 +308,6 @@ public function browseDirectory(
303308
->setPaths($results->getPaths())
304309
->setUser($request->getUser());
305310

306-
$title = nonempty(basename($drequest->getPath()), '/');
307311
$icon = 'fa-folder-open';
308312
$browse_header = $this->buildPanelHeaderView($title, $icon);
309313

@@ -351,7 +355,7 @@ public function browseDirectory(
351355

352356
return $this->newPage()
353357
->setTitle(array(
354-
nonempty(basename($drequest->getPath()), '/'),
358+
$title,
355359
$repository->getDisplayName(),
356360
))
357361
->setCrumbs($crumbs)

‎src/applications/diffusion/controller/DiffusionCommitController.php

+6-3
Original file line numberDiff line numberDiff line change
@@ -27,8 +27,11 @@ public function handleRequest(AphrontRequest $request) {
2727

2828
// If this page is being accessed via "/source/xyz/commit/...", redirect
2929
// to the canonical URI.
30-
$has_callsign = strlen($request->getURIData('repositoryCallsign'));
31-
$has_id = strlen($request->getURIData('repositoryID'));
30+
$repo_callsign = $request->getURIData('repositoryCallsign');
31+
$has_callsign = $repo_callsign !== null && strlen($repo_callsign);
32+
$repo_id = $request->getURIData('repositoryID');
33+
$has_id = $repo_id !== null && strlen($repo_id);
34+
3235
if (!$has_callsign && !$has_id) {
3336
$canonical_uri = $repository->getCommitURI($commit_identifier);
3437
return id(new AphrontRedirectResponse())
@@ -922,7 +925,7 @@ private function renderAuditStatusView(
922925

923926
private function linkBugtraq($corpus) {
924927
$url = PhabricatorEnv::getEnvConfig('bugtraq.url');
925-
if (!strlen($url)) {
928+
if ($url === null || !strlen($url)) {
926929
return $corpus;
927930
}
928931

‎src/applications/diffusion/controller/DiffusionController.php

+4-1
Original file line numberDiff line numberDiff line change
@@ -265,7 +265,10 @@ protected function getRepositoryControllerURI(
265265

266266
protected function renderPathLinks(DiffusionRequest $drequest, $action) {
267267
$path = $drequest->getPath();
268-
$path_parts = array_filter(explode('/', trim($path, '/')));
268+
$path_parts = array();
269+
if ($path !== null && strlen($path)) {
270+
$path_parts = array_filter(explode('/', trim($path, '/')));
271+
}
269272

270273
$divider = phutil_tag(
271274
'span',

‎src/applications/diffusion/controller/DiffusionHistoryController.php

+3-2
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,8 @@ public function handleRequest(AphrontRequest $request) {
5050
// ancestors appropriately, but this would currrently be prohibitively
5151
// expensive in the general case.
5252

53-
$show_graph = !strlen($drequest->getPath());
53+
$show_graph = ($drequest->getPath() === null
54+
|| !strlen($drequest->getPath()));
5455
if ($show_graph) {
5556
$history_list
5657
->setParents($history_results['parents'])
@@ -98,7 +99,7 @@ private function buildHeader(DiffusionRequest $drequest) {
9899
$viewer = $this->getViewer();
99100
$repository = $drequest->getRepository();
100101

101-
$no_path = !strlen($drequest->getPath());
102+
$no_path = $drequest->getPath() === null || !strlen($drequest->getPath());
102103
if ($no_path) {
103104
$header_text = pht('History');
104105
} else {

‎src/applications/diffusion/controller/DiffusionRepositoryManagePanelsController.php

+1-1
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,7 @@ public function handleRequest(AphrontRequest $request) {
4040
}
4141

4242
$selected = $request->getURIData('panel');
43-
if (!strlen($selected)) {
43+
if ($selected === null || !strlen($selected)) {
4444
$selected = head_key($panels);
4545
}
4646

‎src/applications/diffusion/controller/DiffusionServeController.php

+13-5
Original file line numberDiff line numberDiff line change
@@ -183,11 +183,13 @@ private function serveRequest(AphrontRequest $request) {
183183
// won't prompt users who provide a username but no password otherwise.
184184
// See T10797 for discussion.
185185

186-
$have_user = strlen(idx($_SERVER, 'PHP_AUTH_USER'));
187-
$have_pass = strlen(idx($_SERVER, 'PHP_AUTH_PW'));
186+
$http_user = idx($_SERVER, 'PHP_AUTH_USER');
187+
$http_pass = idx($_SERVER, 'PHP_AUTH_PW');
188+
$have_user = $http_user !== null && strlen($http_user);
189+
$have_pass = $http_pass !== null && strlen($http_pass);
188190
if ($have_user && $have_pass) {
189-
$username = $_SERVER['PHP_AUTH_USER'];
190-
$password = new PhutilOpaqueEnvelope($_SERVER['PHP_AUTH_PW']);
191+
$username = $http_user;
192+
$password = new PhutilOpaqueEnvelope($http_pass);
191193

192194
// Try Git LFS auth first since we can usually reject it without doing
193195
// any queries, since the username won't match the one we expect or the
@@ -524,9 +526,15 @@ private function isReadOnlyRequest(
524526
break;
525527
case PhabricatorRepositoryType::REPOSITORY_TYPE_MERCURIAL:
526528
$cmd = $request->getStr('cmd');
529+
if ($cmd === null) {
530+
return false;
531+
}
527532
if ($cmd == 'batch') {
528533
$cmds = idx($this->getMercurialArguments(), 'cmds');
529-
return DiffusionMercurialWireProtocol::isReadOnlyBatchCommand($cmds);
534+
if ($cmds !== null) {
535+
return DiffusionMercurialWireProtocol::isReadOnlyBatchCommand(
536+
$cmds);
537+
}
530538
}
531539
return DiffusionMercurialWireProtocol::isReadOnlyCommand($cmd);
532540
case PhabricatorRepositoryType::REPOSITORY_TYPE_SVN:

‎src/applications/diffusion/controller/DiffusionTagListController.php

+3-1
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,9 @@ public function handleRequest(AphrontRequest $request) {
2525
'offset' => $pager->getOffset(),
2626
);
2727

28-
if (strlen($drequest->getSymbolicCommit())) {
28+
if ($drequest->getSymbolicCommit() !== null
29+
&& strlen($drequest->getSymbolicCommit())) {
30+
2931
$is_commit = true;
3032
$params['commit'] = $drequest->getSymbolicCommit();
3133
} else {

‎src/applications/diffusion/data/DiffusionCommitRef.php

+7
Original file line numberDiff line numberDiff line change
@@ -131,6 +131,13 @@ public function getSummary() {
131131
}
132132

133133
private function formatUser($name, $email) {
134+
if ($name === null) {
135+
$name = '';
136+
}
137+
if ($email === null) {
138+
$email = '';
139+
}
140+
134141
if (strlen($name) && strlen($email)) {
135142
return "{$name} <{$email}>";
136143
} else if (strlen($email)) {

‎src/applications/diffusion/document/DiffusionDocumentRenderingEngine.php

+1-1
Original file line numberDiff line numberDiff line change
@@ -87,7 +87,7 @@ protected function willRenderRef(PhabricatorDocumentRef $ref) {
8787
$ref->setSymbolMetadata($this->getSymbolMetadata());
8888

8989
$coverage = $drequest->loadCoverage();
90-
if (strlen($coverage)) {
90+
if ($coverage !== null && strlen($coverage)) {
9191
$ref->addCoverage($coverage);
9292
}
9393
}

‎src/applications/diffusion/engine/DiffusionCommitHookEngine.php

+1-1
Original file line numberDiff line numberDiff line change
@@ -1133,7 +1133,7 @@ private function newPushEvent() {
11331133
->setHookWait(phutil_microseconds_since($hook_start));
11341134

11351135
$identifier = $this->getRequestIdentifier();
1136-
if (strlen($identifier)) {
1136+
if ($identifier !== null && strlen($identifier)) {
11371137
$event->setRequestIdentifier($identifier);
11381138
}
11391139

‎src/applications/diffusion/query/DiffusionFileFutureQuery.php

+4-1
Original file line numberDiff line numberDiff line change
@@ -92,7 +92,10 @@ final protected function executeQuery() {
9292

9393
$drequest = $this->getRequest();
9494

95-
$name = basename($drequest->getPath());
95+
$name = '';
96+
if ($drequest->getPath() !== null) {
97+
$name = basename($drequest->getPath());
98+
}
9699
$relative_ttl = phutil_units('48 hours in seconds');
97100

98101
try {

0 commit comments

Comments
 (0)