From 726b148afc9836f8cccc6800e99c494cf51dd3b2 Mon Sep 17 00:00:00 2001 From: Aviv Eyal Date: Sat, 12 Aug 2023 08:41:07 -0700 Subject: [PATCH 01/12] Rebrand: Add "path" entries to PlatformSymbols Summary: Ref T15006. Required for D25343 Test Plan: See D25343. Reviewers: O1 Blessed Committers, valerio.bozzolan Reviewed By: O1 Blessed Committers, valerio.bozzolan Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno Maniphest Tasks: T15006 Differential Revision: https://we.phorge.it/D25349 --- src/platform/PlatformSymbols.php | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/platform/PlatformSymbols.php b/src/platform/PlatformSymbols.php index 1b02b775..2529780e 100644 --- a/src/platform/PlatformSymbols.php +++ b/src/platform/PlatformSymbols.php @@ -11,6 +11,14 @@ final class PlatformSymbols return 'Phorge'; } + public static function getPlatformClientPath() { + return 'arcanist/'; + } + + public static function getPlatformServerPath() { + return 'phorge/'; + } + public static function getProductNames() { return array( self::getPlatformClientName(), From 6832afc30026064e9bee3ac09b8b852c3ba4a323 Mon Sep 17 00:00:00 2001 From: Aviv Eyal Date: Sat, 12 Aug 2023 08:43:26 -0700 Subject: [PATCH 02/12] Fix jshint tests Summary: The linter's behavior was changed in https://github.com/jshint/jshint/issues/3444: "warnings" are no longer counted for `maxerr`. Updating the test to match... Test Plan: `arc unit src/lint/linter/__tests__/ArcanistJSHintLinterTestCase.php` with a recent-ish (2.13.6) jshint. 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/D25355 --- src/lint/linter/__tests__/jshint/too-many-errors.lint-test | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/lint/linter/__tests__/jshint/too-many-errors.lint-test b/src/lint/linter/__tests__/jshint/too-many-errors.lint-test index e7b936dd..bc33c0dd 100644 --- a/src/lint/linter/__tests__/jshint/too-many-errors.lint-test +++ b/src/lint/linter/__tests__/jshint/too-many-errors.lint-test @@ -1,5 +1,6 @@ /* jshint maxerr: 1 */ -console.log('foobar') +console.log( +{ ~~~~~~~~~~ -disabled:2:22:E043 -warning:2:22:W033 +disabled:3:1:E043 +error:3:1:E019 From df6c315ace5feccd99e073c68d4f82f11724954f Mon Sep 17 00:00:00 2001 From: bob Date: Mon, 14 Aug 2023 10:54:37 +0200 Subject: [PATCH 03/12] Fix a PHP 8.1/8.2 deprecated use of strlen deprecated call with a NULL argument Summary: This strlen call triggering an exception if an user tried to call the patch command without an authentication token Indeed, strlen() was used in Phabricator to check if a generic value is a non-empty string. This behavior is deprecated since PHP 8.1, we use phutil_nonempty_string() as a replacement. Fix T15599 Test Plan: Remove your arcanist authentication token file (if you have one) and try to call the patch command in a repository. You should get an error message suggesting you to call the install-certificate command instead of an exception. Reviewers: O1 Blessed Committers, Matthew Reviewed By: O1 Blessed Committers, Matthew Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno Maniphest Tasks: T15599 Differential Revision: https://we.phorge.it/D25383 --- src/workflow/ArcanistWorkflow.php | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/workflow/ArcanistWorkflow.php b/src/workflow/ArcanistWorkflow.php index 3a855025..d0863772 100644 --- a/src/workflow/ArcanistWorkflow.php +++ b/src/workflow/ArcanistWorkflow.php @@ -479,8 +479,8 @@ abstract class ArcanistWorkflow extends Phobject { // If we have `token`, this server supports the simpler, new-style // token-based authentication. Use that instead of all the certificate // stuff. - $token = idx($credentials, 'token', ''); - if (strlen($token)) { + $token = idx($credentials, 'token'); + if (phutil_nonempty_string($token)) { $conduit = $this->getConduit(); $conduit->setConduitToken($token); @@ -2244,8 +2244,8 @@ abstract class ArcanistWorkflow extends Phobject { protected function getModernUnitDictionary(array $map) { $map = $this->getModernCommonDictionary($map); - $details = idx($map, 'userData', ''); - if (strlen($details)) { + $details = idx($map, 'userData'); + if (phutil_nonempty_string($details)) { $map['details'] = (string)$details; } unset($map['userData']); From 8b907d7716617989d9d30ddbb3152307560cd36d Mon Sep 17 00:00:00 2001 From: sten Date: Fri, 18 Aug 2023 10:01:00 +0100 Subject: [PATCH 04/12] Fix PHP 8.1 arc patch strlen(null) binary file error Summary: Fix issue in arcanist whereby when doing an arc patch involving adding or removing a binary file, it falls over with strlen(null) errors. Fixes T15617 Test Plan: arc patch Dxxxx Reviewers: O1 Blessed Committers, avivey Reviewed By: O1 Blessed Committers, avivey Subscribers: avivey, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno Maniphest Tasks: T15617 Differential Revision: https://we.phorge.it/D25409 --- src/parser/ArcanistBundle.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/parser/ArcanistBundle.php b/src/parser/ArcanistBundle.php index 4617c9b6..62deca5e 100644 --- a/src/parser/ArcanistBundle.php +++ b/src/parser/ArcanistBundle.php @@ -762,7 +762,7 @@ final class ArcanistBundle extends Phobject { $old_data = $this->getBlob($old_phid, $name); } - $old_length = strlen($old_data); + $old_length = phutil_nonempty_string($old_data) ? strlen($old_data) : 0; // Here, and below, the binary will be emitted with base85 encoding. This // encoding encodes each 4 bytes of input in 5 bytes of output, so we may @@ -795,7 +795,7 @@ final class ArcanistBundle extends Phobject { $new_data = $this->getBlob($new_phid, $name); } - $new_length = strlen($new_data); + $new_length = phutil_nonempty_string($new_data) ? strlen($new_data) : 0; $this->reserveBytes($new_length * 5 / 4); if ($new_data === null) { From d343be59269a03d6345c80bc0b56414031dcabd8 Mon Sep 17 00:00:00 2001 From: Aviv Eyal Date: Thu, 31 Aug 2023 08:12:23 -0700 Subject: [PATCH 05/12] Error handling: send Deprecation messages as explicit Event Summary: Ref T15554. When a deprecation warning is captured here, mark it as such and send using the same channel as error messages. Error Handlers will generally ignore it now, so they'll need to be updated, e.g. D25386 Test Plan: Hitting a `strlen(null)` before This Change: - Web: - PhutilAggregateException - white boxes with red border. - Daemons: - trace in daemon log, task fails. Daemon sleeps for 5 seconds. - Arcanist and Scripts in phorge/bin/ and phorge/scripts: - execution blows up with error trace. - SSH server-side scripts (ssh-auth and ssh-exec): - trace in configured log, execution fails - SSH client-side scripts (ssh-connect): - execution blows up with error trace. After this change: - Web: - Before `D25386`: Nothing on screen, errors show in log. - With `D25386`: logs + dark console. - Daemons: - trace in daemon log, task completes successfully. - Arcanist and Scripts in phorge/bin/ and phorge/scripts/ : - Error trace printed to STDERR, execution continues. - SSH server-side scripts (ssh-auth and ssh-exec): - trace in configured log, execution continues. - SSH client-side scripts (ssh-connect): - Error trace printed to STDERR, execution continues. Reviewers: O1 Blessed Committers, valerio.bozzolan Reviewed By: O1 Blessed Committers, valerio.bozzolan Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno Maniphest Tasks: T15554 Differential Revision: https://we.phorge.it/D25387 --- src/error/PhutilErrorHandler.php | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/error/PhutilErrorHandler.php b/src/error/PhutilErrorHandler.php index 63d5b492..4439bfaf 100644 --- a/src/error/PhutilErrorHandler.php +++ b/src/error/PhutilErrorHandler.php @@ -203,12 +203,17 @@ final class PhutilErrorHandler extends Phobject { if (($num === E_USER_ERROR) || ($num === E_USER_WARNING) || - ($num === E_USER_NOTICE)) { + ($num === E_USER_NOTICE) || + ($num === E_DEPRECATED)) { + + // See T15554 - we special-case E_DEPRECATED because we don't want them + // to kill the process. + $level = ($num === E_DEPRECATED) ? self::DEPRECATED : self::ERROR; $trace = debug_backtrace(); array_shift($trace); self::dispatchErrorMessage( - self::ERROR, + $level, $str, array( 'file' => $file, @@ -380,6 +385,7 @@ final class PhutilErrorHandler extends Phobject { $timestamp = date('Y-m-d H:i:s'); switch ($event) { + case self::DEPRECATED: case self::ERROR: $default_message = sprintf( '[%s] ERROR %d: %s at [%s:%d]', From 35e127da57a8a94799e55dfdf9640b4a165cd9a7 Mon Sep 17 00:00:00 2001 From: sten Date: Mon, 11 Sep 2023 11:06:33 +0100 Subject: [PATCH 06/12] Fix rendering of cowsay sheep.cow Summary: In the templates of the external cowsay library, most replaceable tokens are specified as $var_name. However, the sheep.cow and flaming-sheep.cow use the ${eyes} syntax instead. This is not recognised by PhutilCowsay.php resulting in incorrect rendering of the template. This change updates PhutilCowsay.php to handle ${var_name} tokens as well as $var_name cowsay(cow='sheep'){{{My eyes, my eyes!}}} Test Plan: In a Remarkup comment or document, add ``` cowsay(cow='sheep'){{{How do my eyes look now?}}} ``` When testing in differential, you don't even need to submit the comment. Reviewers: O1 Blessed Committers, avivey Reviewed By: O1 Blessed Committers, avivey Subscribers: avivey, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno Differential Revision: https://we.phorge.it/D25435 --- src/utils/PhutilCowsay.php | 19 ++++++++++++------- src/utils/__tests__/cowsay/sheep.expect | 11 +++++++++++ src/utils/__tests__/cowsay/sheep.test | 13 +++++++++++++ 3 files changed, 36 insertions(+), 7 deletions(-) create mode 100644 src/utils/__tests__/cowsay/sheep.expect create mode 100644 src/utils/__tests__/cowsay/sheep.test diff --git a/src/utils/PhutilCowsay.php b/src/utils/PhutilCowsay.php index ccbeccf4..ca0290b9 100644 --- a/src/utils/PhutilCowsay.php +++ b/src/utils/PhutilCowsay.php @@ -71,14 +71,19 @@ final class PhutilCowsay extends Phobject { $template); } - $template = preg_replace_callback( + $token_patterns = array( '/\\$([a-z]+)/', - array($this, 'replaceTemplateVariable'), - $template); - if ($template === false) { - throw new Exception( - pht( - 'Failed to replace template variables while rendering cow!')); + '/\\${([a-z]+)}/', + ); + foreach ($token_patterns as $token_pattern) { + $template = preg_replace_callback( + $token_pattern, + array($this, 'replaceTemplateVariable'), + $template); + if ($template === false) { + throw new Exception( + pht('Failed to replace template variables while rendering cow!')); + } } $lines = $this->text; diff --git a/src/utils/__tests__/cowsay/sheep.expect b/src/utils/__tests__/cowsay/sheep.expect new file mode 100644 index 00000000..ee5d1c08 --- /dev/null +++ b/src/utils/__tests__/cowsay/sheep.expect @@ -0,0 +1,11 @@ + __________________ +< How are my eyes? > + ------------------ + \ + \ + __ + UooU\.'@@@@@@`. + \__/(@@@@@@@@@@) + (@@@@@@@@) + `YY~~~~YY' + || || diff --git a/src/utils/__tests__/cowsay/sheep.test b/src/utils/__tests__/cowsay/sheep.test new file mode 100644 index 00000000..abd54dfc --- /dev/null +++ b/src/utils/__tests__/cowsay/sheep.test @@ -0,0 +1,13 @@ + $thoughts + $thoughts + __ + U${eyes}U\.'@@@@@@`. + \__/(@@@@@@@@@@) + (@@@@@@@@) + `YY~~~~YY' + || || +~~~~~~~~~~ +{ + "text": "How are my eyes?", + "eyes": "oo" +} From ba42b63704b2f84a378f51ec4b45df32dc86790f Mon Sep 17 00:00:00 2001 From: sten Date: Wed, 27 Sep 2023 10:06:20 +0100 Subject: [PATCH 07/12] Update ArcanistComposerLinter.php to check content-hash instead of hash Summary: The 'hash' key in composer.json files was removed by composer version 1.3.0 in December 2016, in favour of the 'content-hash' key. Update the code to validate the content-hash instead. Fixes T15647 Test Plan: * Update a composer.json file, without running 'composer update' * Run 'arc lint' and confirm it warns you that the composer.lock file is out of date * Run 'composer update' * Run 'arc lint' and confirm it returns OKAY Reviewers: O1 Blessed Committers, avivey Reviewed By: O1 Blessed Committers, avivey Subscribers: tobiaswiese, valerio.bozzolan, Matthew, Cigaryno Maniphest Tasks: T15647 Differential Revision: https://we.phorge.it/D25442 --- src/lint/linter/ArcanistComposerLinter.php | 69 ++++++++++++++++++- .../__tests__/xml/languages-6.lint-test | 2 +- 2 files changed, 68 insertions(+), 3 deletions(-) diff --git a/src/lint/linter/ArcanistComposerLinter.php b/src/lint/linter/ArcanistComposerLinter.php index a9eded3e..c46657e0 100644 --- a/src/lint/linter/ArcanistComposerLinter.php +++ b/src/lint/linter/ArcanistComposerLinter.php @@ -37,11 +37,12 @@ final class ArcanistComposerLinter extends ArcanistLinter { } private function lintComposerJson($path) { - $composer_hash = md5(Filesystem::readFile(dirname($path).'/composer.json')); + $composer_hash = self::getContentHash( + Filesystem::readFile(dirname($path).'/composer.json')); $composer_lock = phutil_json_decode( Filesystem::readFile(dirname($path).'/composer.lock')); - if ($composer_hash !== $composer_lock['hash']) { + if ($composer_hash !== $composer_lock['content-hash']) { $this->raiseLintAtPath( self::LINT_OUT_OF_DATE, pht( @@ -52,4 +53,68 @@ final class ArcanistComposerLinter extends ArcanistLinter { } } + /** + * Returns the md5 hash of the sorted content of the composer.json file. + * + * This function copied from + * https://github.com/ + * composer/composer/blob/1.5.2/src/Composer/Package/Locker.php + * and has the following license: + * + * Copyright (c) Nils Adermann, Jordi Boggiano + * + * Permission is hereby granted, free of charge, to any person obtaining a + * copy of this software and associated documentation files (the "Software"), + * to deal in the Software without restriction, including without limitation + * the rights to use, copy, modify, merge, publish, distribute, sublicense, + * and/or sell copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER + * DEALINGS IN THE SOFTWARE. + * + * + * @param string $composer_file_contents The contents of the composer file. + * + * @return string + */ + public static function getContentHash($composer_file_contents) { + $content = json_decode($composer_file_contents, true); + + $relevant_keys = array( + 'name', + 'version', + 'require', + 'require-dev', + 'conflict', + 'replace', + 'provide', + 'minimum-stability', + 'prefer-stable', + 'repositories', + 'extra', + ); + + $relevant_content = array(); + + foreach (array_intersect($relevant_keys, array_keys($content)) as $key) { + $relevant_content[$key] = $content[$key]; + } + if (isset($content['config']['platform'])) { + $relevant_content['config']['platform'] = $content['config']['platform']; + } + + ksort($relevant_content); + + return md5(json_encode($relevant_content)); + } + } diff --git a/src/lint/linter/__tests__/xml/languages-6.lint-test b/src/lint/linter/__tests__/xml/languages-6.lint-test index f652fbb8..7f14c97f 100644 --- a/src/lint/linter/__tests__/xml/languages-6.lint-test +++ b/src/lint/linter/__tests__/xml/languages-6.lint-test @@ -3,5 +3,5 @@ ~~~~~~~~~~ -error:3:16:XML76:LibXML Error +error:3:12:XML76:LibXML Error error:4:1:XML5:LibXML Error From 98d16d27cf3e13003fd5782f928acc22971b7a84 Mon Sep 17 00:00:00 2001 From: Aviv Eyal Date: Thu, 12 Oct 2023 19:20:37 +0300 Subject: [PATCH 08/12] only update cache file if something changed Summary: See Q77. When installing in a read-only location, updating the file is both redundant (nothing changed) and fails. Make sure to only save the updated file if anything changed. Test Plan: Run `arc lint` somewhere, make `.phutil_module_cache` and `src/` read-only, run `arc lint` again - it should avoid crashing. Reviewers: fgaz, O1 Blessed Committers, valerio.bozzolan Reviewed By: fgaz, O1 Blessed Committers, valerio.bozzolan Subscribers: tobiaswiese, valerio.bozzolan, Matthew, Cigaryno Differential Revision: https://we.phorge.it/D25446 --- src/moduleutils/PhutilLibraryMapBuilder.php | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/moduleutils/PhutilLibraryMapBuilder.php b/src/moduleutils/PhutilLibraryMapBuilder.php index f51fc464..df05050a 100644 --- a/src/moduleutils/PhutilLibraryMapBuilder.php +++ b/src/moduleutils/PhutilLibraryMapBuilder.php @@ -469,10 +469,12 @@ EOPHP; $this->fileSymbolMap = $symbol_map; - // We're done building the cache, so write it out immediately. Note that - // we've only retained entries for files we found, so this implicitly cleans - // out old cache entries. - $this->writeSymbolCache($symbol_map, $source_map); + if ($futures) { + // We're done building/updating the cache, so write it out immediately. + // Note that we've only retained entries for files we found, so this + // implicitly cleans out old cache entries. + $this->writeSymbolCache($symbol_map, $source_map); + } // Our map is up to date, so either show it on stdout or write it to disk. $this->librarySymbolMap = $this->buildLibraryMap($symbol_map); From 5bc53cfe53d0afe813b19f28d6151273e7b86499 Mon Sep 17 00:00:00 2001 From: sten Date: Tue, 12 Sep 2023 17:01:45 +0100 Subject: [PATCH 09/12] Update PhutilCowsay.php to work for small cows Summary: Update PhutilCowsay.php to work for small cows. In doing so, we also simplify the code to just use multiline regexps rather than trying to parse a line at a time. cowsay(cow='small'){{{What about me?}}} Test Plan: Check cowsay works for the built in non-perl cow: ``` cowsay(cow='companion'){{{Built in}}} ``` Test all the perl cows: ``` cowsay(cow='bunny'){{{Testing bunny}}} cowsay(cow='cower'){{{Testing cower}}} cowsay(cow='daemon'){{{Testing daemon}}} cowsay(cow='default'){{{Testing default}}} cowsay(cow='dragon-and-cow'){{{Testing dragon-and-cow}}} cowsay(cow='dragon'){{{Testing dragon}}} cowsay(cow='elephant'){{{Testing elephant}}} cowsay(cow='eyes'){{{Testing eyes}}} cowsay(cow='flaming-sheep'){{{Testing flaming-sheep}}} cowsay(cow='head-in'){{{Testing head-in}}} cowsay(cow='kitty'){{{Testing kitty}}} cowsay(cow='koala'){{{Testing koala}}} cowsay(cow='meow'){{{Testing meow}}} cowsay(cow='moofasa'){{{Testing moofasa}}} cowsay(cow='moose'){{{Testing moose}}} cowsay(cow='mutilated'){{{Testing mutilated}}} cowsay(cow='satanic'){{{Testing satanic}}} cowsay(cow='sheep'){{{Testing sheep}}} cowsay(cow='skeleton'){{{Testing skeleton}}} cowsay(cow='small'){{{Testing small}}} cowsay(cow='squirrel'){{{Testing squirrel}}} cowsay(cow='stegosaurus'){{{Testing stegosaurus}}} cowsay(cow='supermilker'){{{Testing supermilker}}} cowsay(cow='surgery'){{{Testing surgery}}} cowsay(cow='turkey'){{{Testing turkey}}} cowsay(cow='turtle'){{{Testing turtle}}} cowsay(cow='tux'){{{Testing tux}}} cowsay(cow='www'){{{Testing www}}} ``` Reviewers: O1 Blessed Committers, valerio.bozzolan Reviewed By: O1 Blessed Committers, valerio.bozzolan Subscribers: tobiaswiese, valerio.bozzolan, Matthew, Cigaryno Differential Revision: https://we.phorge.it/D25436 --- src/utils/PhutilCowsay.php | 41 ++++++++++------------- src/utils/__tests__/cowsay/cube_perl.test | 3 +- src/utils/__tests__/cowsay/small.expect | 7 ++++ src/utils/__tests__/cowsay/small.test | 12 +++++++ 4 files changed, 39 insertions(+), 24 deletions(-) create mode 100644 src/utils/__tests__/cowsay/small.expect create mode 100644 src/utils/__tests__/cowsay/small.test diff --git a/src/utils/PhutilCowsay.php b/src/utils/PhutilCowsay.php index ca0290b9..aeabf145 100644 --- a/src/utils/PhutilCowsay.php +++ b/src/utils/PhutilCowsay.php @@ -41,34 +41,29 @@ final class PhutilCowsay extends Phobject { $template = $this->template; // Real ".cow" files are Perl scripts which define a variable called - // "$the_cow". We aren't going to interpret Perl, so strip all this stuff - // (and any comments in the file) away. - $template = phutil_split_lines($template, true); - $keep = array(); - $is_perl_cowfile = false; - foreach ($template as $key => $line) { - if (preg_match('/^#/', $line)) { - continue; - } - if (preg_match('/^\s*\\$the_cow/', $line)) { - $is_perl_cowfile = true; - continue; - } - if (preg_match('/^\s*EOC\s*$/', $line)) { - continue; - } - $keep[] = $line; - } - $template = implode('', $keep); + // "$the_cow". We aren't going to interpret Perl, so just get everything + // between the EOC (End Of Cow) tokens. The initial EOC might be in + // quotes, and might have a semicolon. + // We apply regexp modifiers + // * 's' to make . match newlines within the EOC ... EOC block + // * 'm' so we can use ^ to match start of line within the multiline string + $matches = null; + if ( + preg_match('/\$the_cow/', $template) && + preg_match('/EOC[\'"]?;?.*?^(.*?)^EOC/sm', $template, $matches) + ) { + $template = $matches[1]; - // Original .cow files are perl scripts which contain escaped sequences. - // We attempt to unescape here by replacing any character preceded by a - // backslash/escape with just that character. - if ($is_perl_cowfile) { + // Original .cow files are perl scripts which contain escaped sequences. + // We attempt to unescape here by replacing any character preceded by a + // backslash/escape with just that character. $template = preg_replace( '/\\\\(.)/', '$1', $template); + } else { + // Text template. Just strip away comments. + $template = preg_replace('/^#.*$/', '', $template); } $token_patterns = array( diff --git a/src/utils/__tests__/cowsay/cube_perl.test b/src/utils/__tests__/cowsay/cube_perl.test index d9582d2b..a950e588 100644 --- a/src/utils/__tests__/cowsay/cube_perl.test +++ b/src/utils/__tests__/cowsay/cube_perl.test @@ -1,5 +1,5 @@ # test case for original perl-script cowfile -$the_cow = +$the_cow = < + ------------------ + \ ,__, + \ (oo)____ + (__) )\ + ||--|| * diff --git a/src/utils/__tests__/cowsay/small.test b/src/utils/__tests__/cowsay/small.test new file mode 100644 index 00000000..bc3e08e4 --- /dev/null +++ b/src/utils/__tests__/cowsay/small.test @@ -0,0 +1,12 @@ +$eyes = ".." unless ($eyes); +$the_cow = < Date: Fri, 24 Nov 2023 18:58:36 +0200 Subject: [PATCH 10/12] PhutilErrorHandler: support multiple error listeners Summary: Ref T15554. The plan is to add a new listener that will only listen to DEPRECATED events, and do something useful with them. Test Plan: Test script in P26 shows registering 2 handlers and getting both invoked. Reviewers: O1 Blessed Committers, Matthew Reviewed By: O1 Blessed Committers, Matthew Subscribers: Sten, speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno Maniphest Tasks: T15554 Differential Revision: https://we.phorge.it/D25388 --- src/error/PhutilErrorHandler.php | 21 +++++++++++++++------ src/error/phlog.php | 2 +- src/filesystem/PhutilErrorLog.php | 2 +- 3 files changed, 17 insertions(+), 8 deletions(-) diff --git a/src/error/PhutilErrorHandler.php b/src/error/PhutilErrorHandler.php index 4439bfaf..2fc8fe7d 100644 --- a/src/error/PhutilErrorHandler.php +++ b/src/error/PhutilErrorHandler.php @@ -6,7 +6,7 @@ * * This class takes over the PHP error and exception handlers when you call * ##PhutilErrorHandler::initialize()## and forwards all debugging information - * to a listener you install with ##PhutilErrorHandler::setErrorListener()##. + * to a listener you install with ##PhutilErrorHandler::addErrorListener()##. * * To use PhutilErrorHandler, which will enhance the messages printed to the * PHP error log, just initialize it: @@ -16,7 +16,7 @@ * To additionally install a custom listener which can print error information * to some other file or console, register a listener: * - * PhutilErrorHandler::setErrorListener($some_callback); + * PhutilErrorHandler::addErrorListener($some_callback); * * For information on writing an error listener, see * @{function:phutil_error_listener_example}. Providing a listener is optional, @@ -31,7 +31,7 @@ */ final class PhutilErrorHandler extends Phobject { - private static $errorListener = null; + private static $errorListeners = array(); private static $initialized = false; private static $traps = array(); @@ -68,8 +68,15 @@ final class PhutilErrorHandler extends Phobject { * @return void * @task config */ + public static function addErrorListener($listener) { + self::$errorListeners[] = $listener; + } + + /** + * Deprecated - use `addErrorListener`. + */ public static function setErrorListener($listener) { - self::$errorListener = $listener; + self::addErrorListener($listener); } @@ -438,7 +445,7 @@ final class PhutilErrorHandler extends Phobject { break; } - if (self::$errorListener) { + if (self::$errorListeners) { static $handling_error; if ($handling_error) { error_log( @@ -447,7 +454,9 @@ final class PhutilErrorHandler extends Phobject { return; } $handling_error = true; - call_user_func(self::$errorListener, $event, $value, $metadata); + foreach (self::$errorListeners as $error_listener) { + call_user_func($error_listener, $event, $value, $metadata); + } $handling_error = false; } } diff --git a/src/error/phlog.php b/src/error/phlog.php index 105e0740..52dc6af9 100644 --- a/src/error/phlog.php +++ b/src/error/phlog.php @@ -41,7 +41,7 @@ function phlog($value/* , ... */) { /** * Example @{class:PhutilErrorHandler} error listener callback. When you call - * `PhutilErrorHandler::setErrorListener()`, you must pass a callback function + * `PhutilErrorHandler::addErrorListener()`, you must pass a callback function * with the same signature as this one. * * NOTE: @{class:PhutilErrorHandler} handles writing messages to the error diff --git a/src/filesystem/PhutilErrorLog.php b/src/filesystem/PhutilErrorLog.php index d652d854..a9ec08a4 100644 --- a/src/filesystem/PhutilErrorLog.php +++ b/src/filesystem/PhutilErrorLog.php @@ -83,7 +83,7 @@ final class PhutilErrorLog } public function onError($event, $value, array $metadata) { - // If we've set "error_log" to a real file, so messages won't be output to + // If we've set "error_log" to a real file, messages won't be output to // stderr by default. Copy them to stderr. if ($this->logPath === null) { From 16a412b1080290d798b5f903e8b5cb2df1e957d3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bartosz=20Dziewo=C5=84ski?= Date: Sat, 19 Aug 2023 23:45:37 +0200 Subject: [PATCH 11/12] Fix ArcanistExternalLinter on Windows Summary: When using `proc_open()` with `'bypass_shell' => true` on Windows, file extensions other than .exe will not be resolved. Various linters therefore don't work, such as `jshint`, which is actually `jshint.cmd`. The problem was already observed and fixed in some places (e.g. ArcanistGitAPI trying to run `git`), but not in ArcanistExternalLinter. Changes: * Fix `Filesystem::resolveBinary()` to actually only resolve executable files on Windows, and not any other files with no extension or with an extension listed in %PATHEXT%. Those files can be executed by typing their name in the cmd.exe shell, but not directly by low-level Windows functions, and we're using `'bypass_shell'` to bypass the shell. * Fix `ArcanistExternalLinter::getBinary()` to call `Filesystem::resolveBinary()` on Windows. Test Plan: Run `arc lint` on the Phorge repository while on Windows. Observe no errors related to jshint. Reviewers: O1 Blessed Committers, avivey Reviewed By: O1 Blessed Committers, avivey Subscribers: aklapper, avivey, speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno Maniphest Tasks: T15544 Differential Revision: https://we.phorge.it/D25341 --- src/filesystem/Filesystem.php | 17 ++++++++++++++--- src/lint/linter/ArcanistExternalLinter.php | 14 +++++++++++++- 2 files changed, 27 insertions(+), 4 deletions(-) diff --git a/src/filesystem/Filesystem.php b/src/filesystem/Filesystem.php index 9cd52268..981d4feb 100644 --- a/src/filesystem/Filesystem.php +++ b/src/filesystem/Filesystem.php @@ -1103,12 +1103,23 @@ final class Filesystem extends Phobject { } return null; } - $stdout = head($stdout); + + // These are the only file extensions that can be executed directly + // when using proc_open() with 'bypass_shell'. + $executable_extensions = ['exe', 'bat', 'cmd', 'com']; + + foreach ($stdout as $line) { + $path = trim($line); + $ext = pathinfo($path, PATHINFO_EXTENSION); + if (in_array($ext, $executable_extensions)) { + return $path; + } + } + return null; } else { list($err, $stdout) = exec_manual('which %s', $binary); + return $err === 0 ? trim($stdout) : null; } - - return $err === 0 ? trim($stdout) : null; } diff --git a/src/lint/linter/ArcanistExternalLinter.php b/src/lint/linter/ArcanistExternalLinter.php index a394528f..8c35e440 100644 --- a/src/lint/linter/ArcanistExternalLinter.php +++ b/src/lint/linter/ArcanistExternalLinter.php @@ -133,7 +133,19 @@ abstract class ArcanistExternalLinter extends ArcanistFutureLinter { * @task bin */ final public function getBinary() { - return coalesce($this->bin, $this->getDefaultBinary()); + $bin = coalesce($this->bin, $this->getDefaultBinary()); + if (phutil_is_windows()) { + // On Windows, we use proc_open with 'bypass_shell' option, which will + // resolve %PATH%, but not %PATHEXT% (unless the extension is .exe). + // Therefore find the right binary ourselves. + // If we can't find it, leave it unresolved, as this string will be + // used in some error messages elsewhere. + $resolved = Filesystem::resolveBinary($bin); + if ($resolved) { + return $resolved; + } + } + return $bin; } /** From e46025f7a9146f9918bab9d6fbdf6ed1816db5b5 Mon Sep 17 00:00:00 2001 From: Andre Klapper Date: Mon, 4 Dec 2023 19:27:47 -0800 Subject: [PATCH 12/12] Fix PHP 8.1 "urlencode(null)" exception blocking account registration redirect for custom OAuth provider Summary: It seems that a `tokenSecret` is not always passed at this stage, and that PHP's `urlencode()` does not accept passing a `null` string since PHP 8.1 (I could not find any upstream note about this but bug reports across the web seem to confirm this). Thus do not try to `urlencode($this->tokenSecret)` if it is `null`. ``` EXCEPTION: (RuntimeException) urlencode(): Passing null to parameter #1 ($string) of type string is deprecated at [/src/error/PhutilErrorHandler.php:261] arcanist(), ava(), phorge(), wmf-ext-misc() #0 <#2> PhutilErrorHandler::handleError(integer, string, string, integer) called at [/src/error/PhutilErrorHandler.php:261] #1 <#2> urlencode(NULL) called at [/src/future/oauth/PhutilOAuth1Future.php:232] ``` Closes T15589 Test Plan: * As an admin, set up custom "MediaWiki" OAuth provider from from https://gitlab.wikimedia.org/-/ide/project/repos/phabricator/extensions/edit/wmf/stable/-/src/oauth/ * As an admin, apply D25373 * As a user, go to `/auth/login/mediawiki:whatever/` * Select login button Redirect now works as expected: The URL redirect to allow access on http://mediawiki.localhost/index.php?title=Special%3AOAuth%2Fauthorize&oauth_token=1234567890abcdef1234567890abcdef&oauth_consumer_key=1234567890abcdef1234567890abcdef works as expected, instead of showing a raw error page about `urlencode()` not accepting passing `null`. (After allowing authorization there are more issues in Phorge code but they are out of scope for this Arcanist patch.) Reviewers: O1 Blessed Committers, valerio.bozzolan, speck Reviewed By: O1 Blessed Committers, valerio.bozzolan, speck Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno Maniphest Tasks: T15589 Differential Revision: https://we.phorge.it/D25374 --- src/future/oauth/PhutilOAuth1Future.php | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/future/oauth/PhutilOAuth1Future.php b/src/future/oauth/PhutilOAuth1Future.php index 8edd6c26..f73b9db0 100644 --- a/src/future/oauth/PhutilOAuth1Future.php +++ b/src/future/oauth/PhutilOAuth1Future.php @@ -229,7 +229,10 @@ final class PhutilOAuth1Future extends FutureProxy { $consumer_secret = $this->consumerSecret->openEnvelope(); } - $key = urlencode($consumer_secret).'&'.urlencode($this->tokenSecret); + $key = urlencode($consumer_secret).'&'; + if ($this->tokenSecret !== null) { + $key .= urlencode($this->tokenSecret); + } switch ($this->signatureMethod) { case 'HMAC-SHA1':