From 97e163187418b74b94456cff3c2b680e97c45f0f Mon Sep 17 00:00:00 2001 From: Andre Klapper Date: Sat, 10 Jun 2023 16:34:58 +0200 Subject: [PATCH 01/10] Fix PHP 8.1 "strlen(null)" exception which blocks creating a project with an empty Description field Summary: `strlen()` was used in Phabricator to check if a generic value is a non-empty string. This behavior is deprecated since PHP 8.1. Phorge adopts `phutil_nonempty_stringlike()` as a replacement for string-alike variables. Note: this may highlight other absurd input values that might be worth correcting instead of just ignoring. If phutil_nonempty_stringlike() throws an exception in your instance, report it to Phorge to evaluate and fix that specific corner case. Closes first half of T15331 Test Plan: Applied these two changes (one in Arcanist, one in Phorge). Project with empty Description field was created and `/project/view/projectid/` rendered in web browser. Reviewers: O1 Blessed Committers, valerio.bozzolan, speck Reviewed By: O1 Blessed Committers, valerio.bozzolan, speck Subscribers: avivey, speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno Maniphest Tasks: T15331 Differential Revision: https://we.phorge.it/D25176 --- src/utils/utils.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/utils/utils.php b/src/utils/utils.php index 53e9f809..0d6dc2a1 100644 --- a/src/utils/utils.php +++ b/src/utils/utils.php @@ -900,7 +900,7 @@ function array_mergev(array $arrayv) { * NOTE: This function does not treat "\r" on its own as a newline because none * of SVN, Git or Mercurial do on any OS. * - * @param string Block of text to be split into lines. + * @param string|PhutilSafeHTML $corpus Block of text to be split into lines. * @param bool If true, retain line endings in result strings. * @return list List of lines. * @@ -908,7 +908,7 @@ function array_mergev(array $arrayv) { * @phutil-external-symbol function phutil_safe_html */ function phutil_split_lines($corpus, $retain_endings = true) { - if (!strlen($corpus)) { + if (!phutil_nonempty_stringlike($corpus)) { return array(''); } From 677e117b97fc3280d49e368c45b9accf6515fa8f Mon Sep 17 00:00:00 2001 From: Steve Campbell Date: Mon, 19 Jun 2023 14:55:33 +0100 Subject: [PATCH 02/10] Fix strlen() being passed a null in ArcanistUnitConsoleRenderer.php Summary: Fix the following error in ArcanistUnitConsoleRenderer.php under PHP 8.1: ``` strlen(): Passing null to parameter #1 ($string) of type string is deprecated ``` Stack trace: ``` EXCEPTION: (RuntimeException) strlen(): Passing null to parameter #1 ($string) of type string is deprecated at [/src/error/PhutilErrorHandler.php:261] arcanist(head=master, ref.master=97e163187418, custom=4) #0 PhutilErrorHandler::handleError(integer, string, string, integer) called at [/src/unit/renderer/ArcanistUnitConsoleRenderer.php:15] #1 ArcanistUnitConsoleRenderer::renderUnitResult(ArcanistUnitTestResult) called at [/src/workflow/ArcanistUnitWorkflow.php:190] #2 ArcanistUnitWorkflow::run() called at [/scripts/arcanist.php:427] ``` Ref T15187 Test Plan: arc unit Reviewers: O1 Blessed Committers, valerio.bozzolan Reviewed By: O1 Blessed Committers, valerio.bozzolan Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno Maniphest Tasks: T15187 Differential Revision: https://we.phorge.it/D25300 --- .../__tests__/ArcanistUnitTestResultTestCase.php | 13 +++++++++++++ src/unit/renderer/ArcanistUnitConsoleRenderer.php | 2 +- 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/src/unit/__tests__/ArcanistUnitTestResultTestCase.php b/src/unit/__tests__/ArcanistUnitTestResultTestCase.php index 7c4d43dd..7458af8f 100644 --- a/src/unit/__tests__/ArcanistUnitTestResultTestCase.php +++ b/src/unit/__tests__/ArcanistUnitTestResultTestCase.php @@ -40,4 +40,17 @@ final class ArcanistUnitTestResultTestCase extends PhutilTestCase { } } + public function testRenderer() { + $result = new ArcanistUnitTestResult(); + $result->setName('RendererTest'); + $result->setResult('pass'); + $result->setDuration(0.001); + $result->setUserData(''); + + $renderer = new ArcanistUnitConsoleRenderer(); + $output = $renderer->renderUnitResult($result); + $test_dscr = 'Renderer copes with null namespace'; + $this->assertTrue((bool)preg_match('/PASS/', $output), $test_dscr); + } + } diff --git a/src/unit/renderer/ArcanistUnitConsoleRenderer.php b/src/unit/renderer/ArcanistUnitConsoleRenderer.php index 729c4b8d..48a1d96f 100644 --- a/src/unit/renderer/ArcanistUnitConsoleRenderer.php +++ b/src/unit/renderer/ArcanistUnitConsoleRenderer.php @@ -12,7 +12,7 @@ final class ArcanistUnitConsoleRenderer extends ArcanistUnitRenderer { $test_name = $result->getName(); $test_namespace = $result->getNamespace(); - if (strlen($test_namespace)) { + if (phutil_nonempty_string($test_namespace)) { $test_name = $test_namespace.'::'.$test_name; } From 8130241a11ac6e65a43d1ca419f7b46cba3b58f2 Mon Sep 17 00:00:00 2001 From: Steve Campbell Date: Wed, 21 Jun 2023 19:56:41 +0100 Subject: [PATCH 03/10] Add resources/ssl/custom.pem to .gitignore Summary: When arcanist connects to a phorge site which uses an SSL certificate signed by a local CA, then the client needs to have a resources/ssl/custom.pem file as per resources/ssl/README As this is client specific it should be in the .gitignore file. Also update resources/ssl/README to replace [Pp]habricator with [Pp]horge. Test Plan: ``` touch resources/ssl/custom.pem git status ``` The 'git status' should show no changes. Reviewers: O1 Blessed Committers, valerio.bozzolan Reviewed By: O1 Blessed Committers, valerio.bozzolan Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno Differential Revision: https://we.phorge.it/D25304 --- .gitignore | 1 + resources/ssl/README | 8 ++++---- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/.gitignore b/.gitignore index 0e13f861..31f4ebb7 100644 --- a/.gitignore +++ b/.gitignore @@ -11,6 +11,7 @@ # User extensions /externals/includes/* /src/extensions/* +/resources/ssl/custom.pem # XHPAST /support/xhpast/*.a diff --git a/resources/ssl/README b/resources/ssl/README index 4d5057f5..d6c303f7 100644 --- a/resources/ssl/README +++ b/resources/ssl/README @@ -34,11 +34,11 @@ If "custom.pem" is present, that file will be used instead of "default.pem". If you receive errors using your "custom.pem" file, you can test it directly with `curl` by running a command like this: - curl -v --cacert path/to/your/custom.pem https://phabricator.example.com/ + curl -v --cacert arcanist/resources/ssl/custom.pem https://phorge.example.com/ -Replace "path/to/your/custom.pem" with the path to your "custom.pem" file, -and replace "https://phabricator.example.com" with the real URL of your -Phabricator install. +Replace "arcanist/resources/ssl/custom.pem" with the path to your "custom.pem" +file, and replace "https://phorge.example.com" with the real URL of your Phorge +install. The initial lines of output from `curl` should give you information about the SSL handshake and certificate verification, which may be helpful in resolving From 8426ebc053b3c9260ab5939a42281cf4256c1a23 Mon Sep 17 00:00:00 2001 From: Aviv Eyal Date: Sat, 24 Jun 2023 12:23:58 -0700 Subject: [PATCH 04/10] Fix `arc browse` for php 8.x Summary: Ref T15064 Test Plan: Run `arc browse `, get a browser and no exception. Reviewers: O1 Blessed Committers, valerio.bozzolan, chris Reviewed By: O1 Blessed Committers, chris Subscribers: speck, tobiaswiese, Matthew, Cigaryno Maniphest Tasks: T15064 Differential Revision: https://we.phorge.it/D25309 --- src/ref/ArcanistRepositoryRef.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ref/ArcanistRepositoryRef.php b/src/ref/ArcanistRepositoryRef.php index b0122ab4..38ef327b 100644 --- a/src/ref/ArcanistRepositoryRef.php +++ b/src/ref/ArcanistRepositoryRef.php @@ -66,7 +66,7 @@ final class ArcanistRepositoryRef )); foreach ($params as $key => $value) { - if (!strlen($value)) { + if ($value === null || !strlen($value)) { unset($params[$key]); } } From 152ba1f02a31b7e48ed2923b611e66f951c9fdf0 Mon Sep 17 00:00:00 2001 From: Aviv Eyal Date: Mon, 3 Jul 2023 13:42:25 -0700 Subject: [PATCH 05/10] Add explicit tests for phutil_string_cast Summary: Test (and document) what phutil_string_cast actually does to some things. Test Plan: `arc unit` Reviewers: O1 Blessed Committers, speck Reviewed By: O1 Blessed Committers, speck Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno Differential Revision: https://we.phorge.it/D25326 --- src/utils/__tests__/PhutilUtilsTestCase.php | 46 +++++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/src/utils/__tests__/PhutilUtilsTestCase.php b/src/utils/__tests__/PhutilUtilsTestCase.php index 0d4c898f..d7213685 100644 --- a/src/utils/__tests__/PhutilUtilsTestCase.php +++ b/src/utils/__tests__/PhutilUtilsTestCase.php @@ -1070,4 +1070,50 @@ final class PhutilUtilsTestCase extends PhutilTestCase { } } + + public function testStringCasting() { + $cases = array( + array(123, '123', 'number'), + array(null, '', 'null to empty string'), + array('text', 'text', 'string'), + array(17.4, '17.4', 'float'), + array(true, '1', 'boolean true (well done php?)'), + array(false, '', 'boolean false (to empty string)'), + array(0, '0', 'zero (int)'), + array(0.0, '0', 'zero (float'), + array( + new PhutilURI('http://www.example.com'), + 'http://www.example.com', + 'Object with toString()', + ), + ); + + $exception_cases = array( + array(array(), 'array'), + ); + + foreach ($cases as $test_case) { + list($input, $expected_output, $test_name) = $test_case; + + $actual = phutil_string_cast($input); + + $this->assertEqual($expected_output, $actual, $test_name); + } + + + $expect_exceptions = array('Exception'); + foreach ($exception_cases as $test_case) { + list($input, $test_name) = $test_case; + + try { + phutil_string_cast($input); + } catch (Exception $ex) { + $caught = $ex; + } catch (Throwable $ex) { + $caught = $ex; + } + + $this->assertCaught($expect_exceptions, $caught, $test_name); + } + } } From 6e4947b55f09d58b80856fc1b54dccbffe3ecbc0 Mon Sep 17 00:00:00 2001 From: Aviv Eyal Date: Fri, 7 Jul 2023 02:02:12 -0700 Subject: [PATCH 06/10] Improve arc-browse for php 8.1 Summary: Ref T15064. Test Plan: ran some `arc browse` commands in php 8.1, got no exceptions. Reviewers: O1 Blessed Committers, valerio.bozzolan, Sten Reviewed By: O1 Blessed Committers, valerio.bozzolan, Sten Subscribers: Sten, speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno Maniphest Tasks: T15064 Differential Revision: https://we.phorge.it/D25339 --- .../query/ArcanistBrowseObjectNameURIHardpointQuery.php | 4 ++-- src/browse/workflow/ArcanistBrowseWorkflow.php | 4 ++-- src/ref/ArcanistRepositoryRef.php | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/browse/query/ArcanistBrowseObjectNameURIHardpointQuery.php b/src/browse/query/ArcanistBrowseObjectNameURIHardpointQuery.php index c2ff847e..20b1d00e 100644 --- a/src/browse/query/ArcanistBrowseObjectNameURIHardpointQuery.php +++ b/src/browse/query/ArcanistBrowseObjectNameURIHardpointQuery.php @@ -15,7 +15,7 @@ final class ArcanistBrowseObjectNameURIHardpointQuery $token_set = array(); foreach ($refs as $key => $ref) { $token = $ref->getToken(); - if (!strlen($token)) { + if (!phutil_nonempty_string($token)) { continue; } @@ -41,7 +41,7 @@ final class ArcanistBrowseObjectNameURIHardpointQuery continue; } - $uri = idx($object, 'uri'); + $uri = idx($object, 'uri', ''); if (!strlen($uri)) { continue; } diff --git a/src/browse/workflow/ArcanistBrowseWorkflow.php b/src/browse/workflow/ArcanistBrowseWorkflow.php index ffc41654..2198d50e 100644 --- a/src/browse/workflow/ArcanistBrowseWorkflow.php +++ b/src/browse/workflow/ArcanistBrowseWorkflow.php @@ -142,7 +142,7 @@ EOTEXT if ($many_hits) { foreach ($many_hits as $ref) { $token = $ref->getToken(); - if (strlen($token)) { + if (phutil_nonempty_string($token)) { $message = pht('Argument "%s" is ambiguous.', $token); } else { $message = pht('Default behavior is ambiguous.'); @@ -167,7 +167,7 @@ EOTEXT foreach ($many_hits as $ref) { $token_display = $ref->getToken(); - if (!strlen($token)) { + if (!phutil_nonempty_string($token)) { $token_display = pht(''); } diff --git a/src/ref/ArcanistRepositoryRef.php b/src/ref/ArcanistRepositoryRef.php index 38ef327b..c345c7d3 100644 --- a/src/ref/ArcanistRepositoryRef.php +++ b/src/ref/ArcanistRepositoryRef.php @@ -79,7 +79,7 @@ final class ArcanistRepositoryRef $params = $params + $defaults; - $uri_base = $this->browseURI; + $uri_base = coalesce($this->browseURI, ''); $uri_base = rtrim($uri_base, '/'); $uri_branch = phutil_escape_uri_path_component($params['branch']); From b996b4799b024de19796404d085650abd24a8797 Mon Sep 17 00:00:00 2001 From: Valerio Bozzolan Date: Tue, 18 Jul 2023 12:39:36 +0200 Subject: [PATCH 07/10] phutil_nonempty_scalar(): don't throw when receiving a boolean scalar Summary: Note that booleans are scalars. Full stop. | is_scalar($v) | Result | |------------------|--------| | `"foo"` | true | | `""` | true | | `null` | true | | `0` | true | | `0.5` | true | | `true` | true | | `false` | true | | `new stdclass()` | false | | `array()` | false | Note that phutil_nonempty_scalar() was designed just to tell whenever a scalar is "empty" or not. So it must not explode when receiving a valid scalar. So the question is not whenever a boolean is a scalar or not, but whenever is empty or not. But also this is a clear fact: | `$v` | `is_scalar($v)` | `!is_empty($v)` | `if(strlen($v))`| |---------|-----------------|-----------------|-----------------| | `true` | `true` | `true` | `true` | | `false` | `true` | `false` | `false` | In short, now the function does not explode anymore with bool values. Instead, it says whenever are empty or not. In bold the exact changes: | Value |`phutil_nonempty_scalar($v)`| |-------------------|----------------------------| | `"foo"` | true | | `""` | false | | `null` | false | | `0` | true | | `0.5` | true | |`obj` with tostring| true | |`obj` withno tostr.| Exception | | `array()` | Exception | | `true` | ~~Exception~~ **true** | | `false` | ~~Exception~~ **false** | Closes T15239 Test Plan: - check if it makes sense to you - check the few usages Reviewers: O1 Blessed Committers, avivey Reviewed By: O1 Blessed Committers, avivey Subscribers: speck, tobiaswiese, Matthew, Cigaryno Maniphest Tasks: T15239 Differential Revision: https://we.phorge.it/D25117 --- src/utils/__tests__/PhutilUtilsTestCase.php | 9 ++++++++- src/utils/utils.php | 11 +++++++++++ 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/src/utils/__tests__/PhutilUtilsTestCase.php b/src/utils/__tests__/PhutilUtilsTestCase.php index d7213685..1e088f5c 100644 --- a/src/utils/__tests__/PhutilUtilsTestCase.php +++ b/src/utils/__tests__/PhutilUtilsTestCase.php @@ -1004,11 +1004,18 @@ final class PhutilUtilsTestCase extends PhutilTestCase { $uri = new PhutilURI('http://example.org/'); + // Each test is defined in this way: + // 0: subject $value + // 1: expected result from phutil_nonempty_string($value) + // 2: expected result from phutil_nonempty_stringlike($value) + // 3: expected result from phutil_nonempty_scalar($value) + // 4: human test name $map = array( array(null, false, false, false, 'literal null'), array('', false, false, false, 'empty string'), array('x', true, true, true, 'nonempty string'), - array(false, null, null, null, 'bool'), + array(false, null, null, false, 'false bool'), + array(true, null, null, true, 'true bool'), array(1, null, null, true, 'integer'), array($uri, null, true, true, 'uri object'), array(2.5, null, null, true, 'float'), diff --git a/src/utils/utils.php b/src/utils/utils.php index 0d6dc2a1..0b86cd59 100644 --- a/src/utils/utils.php +++ b/src/utils/utils.php @@ -2187,6 +2187,9 @@ function phutil_nonempty_stringlike($value) { * string other than the empty string, integers, and floats are considered * scalar. * + * Note that booleans are also valid scalars, where false is considered empty, + * and true is non-empty since if you cast true to string, it's non-empty. + * * This method raises an exception if passed any other value. * * @param Value to test. @@ -2205,6 +2208,14 @@ function phutil_nonempty_scalar($value) { return true; } + // Booleans are also valid scalars by PHP. Inventing the opposite can be + // too much esoteric and problematic. + // false: empty, because casted to string becomes '' (empty) + // true: non-empty, because casted to string becomes '1' (non-empty) + if ($value === false || $value === true) { + return $value; + } + if (is_object($value)) { try { $string = phutil_string_cast($value); From 8a2cb75d63081772fa537bb7aa6c69cfef870bea Mon Sep 17 00:00:00 2001 From: Aviv Eyal Date: Fri, 21 Jul 2023 09:45:01 -0700 Subject: [PATCH 08/10] Fix tab complete in php 8 Summary: Ref T15064 Test Plan: run `arc shell-complete`; `arc shell-complete arc shell`; Should suggest completions, not crash. Reviewers: O1 Blessed Committers, valerio.bozzolan Reviewed By: O1 Blessed Committers, valerio.bozzolan Subscribers: mainframe98, speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno Maniphest Tasks: T15064 Differential Revision: https://we.phorge.it/D25350 --- src/toolset/workflow/ArcanistShellCompleteWorkflow.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/toolset/workflow/ArcanistShellCompleteWorkflow.php b/src/toolset/workflow/ArcanistShellCompleteWorkflow.php index 9c2fcf9a..307231c8 100644 --- a/src/toolset/workflow/ArcanistShellCompleteWorkflow.php +++ b/src/toolset/workflow/ArcanistShellCompleteWorkflow.php @@ -92,7 +92,7 @@ EOTEXT $argv = $this->getArgument('argv'); $is_generate = $this->getArgument('generate'); - $is_shell = (bool)strlen($this->getArgument('shell')); + $is_shell = phutil_nonempty_string($this->getArgument('shell')); $is_current = $this->getArgument('current'); if ($argv) { From 6c6f47bf9cf6f6df116454254872332b65dfed12 Mon Sep 17 00:00:00 2001 From: Aviv Eyal Date: Mon, 24 Jul 2023 06:16:40 -0700 Subject: [PATCH 09/10] (arc) Unify type-checking for `setHref()` type methods Summary: Introduce `PhutilURI::checkHrefType` to unify type-check of some PHUI objects (See D25357). Ref T15316 Test Plan: Run this script: ``` #!/usr/bin/env php Date: Sat, 29 Jul 2023 12:20:14 +0300 Subject: [PATCH 10/10] Fix PHP 8.0 ValueError calling mb_convert_encoding() with an invalid encoding Summary: Per https://www.php.net/manual/en/function.mb-convert-encoding.php, as of PHP 8.0.0, a ValueError is thrown if the value of `to_encoding` or `from_encoding` is an invalid encoding but a ValueError is not suppressed by the stfu operator ("@"). Origin of the function: https://secure.phabricator.com/rPHU72ad8fd0f05b0d84f7d8efd7db62ad0b3ba4431f Premising that Arcanist elevates warnings to exception, now we just try-catch. Closes T15423 Test Plan: On `/diffusion/edit/1/page/encoding/`, * enter a valid encoding, such as "7bit", successfully changed encoding * enter a valid encoding with random capitalization, such as "7biT", successfully changed encoding * enter a valid alias encoding, such as "ISO-10646-UCS-2", successfully changed encoding * enter a valid alias encoding with random capitalization, such as "isO-10646-uCS-2", successfully changed encoding * enter an invalid encoding, such as "whatever", get error message "Repository encoding "whatever" is not valid: String conversion from encoding 'UTF-8' to encoding 'whatever' failed: mb_convert_encoding(): Argument #2 ($to_encoding) must be a valid encoding, "whatever" given" In any case, no exception is shown anymore. Reviewers: O1 Blessed Committers, valerio.bozzolan, speck Reviewed By: O1 Blessed Committers, valerio.bozzolan, speck Subscribers: 0, speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno Maniphest Tasks: T15423 Differential Revision: https://we.phorge.it/D25249 --- src/utils/utf8.php | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/src/utils/utf8.php b/src/utils/utf8.php index 52d147e6..7ba7b11e 100644 --- a/src/utils/utf8.php +++ b/src/utils/utf8.php @@ -718,13 +718,10 @@ function phutil_utf8_convert($string, $to_encoding, $from_encoding) { 'mbstring')); } - $result = @mb_convert_encoding($string, $to_encoding, $from_encoding); - - if ($result === false) { - $message = error_get_last(); - if ($message) { - $message = idx($message, 'message', pht('Unknown error.')); - } + try { + $result = mb_convert_encoding($string, $to_encoding, $from_encoding); + } catch (Throwable $ex) { + $message = $ex->getMessage(); throw new Exception( pht( "String conversion from encoding '%s' to encoding '%s' failed: %s",