Summary:
Explicitly cast `$minutes` to int to avoid an error in PHP 8.1 and later.
```
ERROR 8192: Implicit conversion from float to int loses precision at [/var/www/html/phorge/arcanist/src/unit/renderer/ArcanistUnitConsoleRenderer.php:86]
```
Closes T15968
Test Plan: Run `bin/arc unit --everything`.
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15968
Differential Revision: https://we.phorge.it/D25848
Summary:
Followup to rARC99e57a70. This patch should cover all remaining issues now that PHPStan covers it (instead of the previous trial-and-error approach).
Implicitly nullable parameter declarations are deprecated in PHP 8.4:
https://php.watch/versions/8.4/implicitly-marking-parameter-type-nullable-deprecated
The proposed syntax was introduced in PHP 7.1 and Phorge requires PHP 7.2 now.
Test Plan: Run static code analysis.
Reviewers: O1 Blessed Committers, avivey
Reviewed By: O1 Blessed Committers, avivey
Subscribers: tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Differential Revision: https://we.phorge.it/D25831
Summary:
Implicitly nullable parameter declarations are deprecated in PHP 8.4:
https://php.watch/versions/8.4/implicitly-marking-parameter-type-nullable-deprecated
The proposed syntax was introduced in PHP 7.1.
Refs T15935
Test Plan: Try to successfully run `./bin/storage upgrade` with PHP 8.4
Reviewers: O1 Blessed Committers, chris
Reviewed By: O1 Blessed Committers, chris
Subscribers: tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15935
Differential Revision: https://we.phorge.it/D25813
Summary:
Recently this unit test was always failing for some specific users.
I was able to reproduce the issue with an Italian unix environment:
arc unit src/parser/__tests__/ArcanistBundleTestCase.php
Obtaining:
counterexample
EXCEPTION (Exception): Diff Parse Exception: Expected '\ No newline at end of file'.
After this change, the involved unit test always work also in my Italian environment.
Note that the `LC_ALL=C` means that all localization should be deactivated,
and we should instead use C-sourced strings (not translated in any language). Nice!
Closes T15927
Test Plan: Run the unit test. It does not fail anymore if you are Italian of French or whatever.
Reviewers: O1 Blessed Committers, mainframe98, 20after4
Reviewed By: O1 Blessed Committers, mainframe98, 20after4
Subscribers: mainframe98, aklapper, tobiaswiese, Matthew, Cigaryno
Maniphest Tasks: T15927
Differential Revision: https://we.phorge.it/D25809
Summary:
`ArcanistHardpointFutureList` does not have a constructor and must be instantiated without any parameters.
As the code checks `if ($result instanceof Future)` and tries to pass `$result` as a parameter, the intention seems to be calling `newFromFutures($result)` on the new `ArcanistHardpointFutureList` instance.
Closes T15836
Test Plan: Read the code in `ArcanistHardpointFutureList`.
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15836
Differential Revision: https://we.phorge.it/D25708
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_string()` as a replacement.
Note: this may highlight other absurd input values that might be worth correcting
instead of just ignoring. If phutil_nonempty_string() throws an exception in your
instance, report it to Phorge to evaluate and fix that specific corner case.
```
ERROR 8192: urlencode(): Passing null to parameter #1 ($string) of type string is deprecated at [/var/www/html/phorge/arcanist/src/future/oauth/PhutilOAuth1Future.php:232]
```
Closes T15929
Test Plan: Run `arcanist/bin/arc unit --everything` on a PHP >= 8.1 system.
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15929
Differential Revision: https://we.phorge.it/D25811
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_string()` as a replacement.
Note: this may highlight other absurd input values that might be worth correcting
instead of just ignoring. If phutil_nonempty_string() throws an exception in your
instance, report it to Phorge to evaluate and fix that specific corner case.
```
ERROR 8192: strlen(): Passing null to parameter #1 ($string) of type string is deprecated at [/var/www/html/phorge/arcanist/src/future/http/status/HTTPFutureHTTPResponseStatus.php:16]
```
Closes T15930
Test Plan: Run `arcanist/bin/arc unit --everything` on a PHP >= 8.1 system.
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15930
Differential Revision: https://we.phorge.it/D25810
Summary:
Per https://github.com/php-fig/fig-standards/blob/master/proposed/phpdoc.md the valid keyword is `$this`.
Thus replace `this` to make output of static code analysis slightly less noisy.
Test Plan: Read the docs.
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/D25818
Summary:
When `$u` is `null`, `null` is passed to `preg_match()` which is deprecated behavior since PHP 8.1.
Thus first check if `$u === null`.
```
ERROR 8192: preg_match(): Passing null to parameter #2 ($subject) of type string is deprecated at [/var/www/html/phorge/arcanist/src/parser/xhpast/api/XHPASTNode.php:243]
```
Refs T15926
Test Plan: Either read the code, or probably run something like `arc unit` on D25797?
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15926
Differential Revision: https://we.phorge.it/D25801
Summary:
Make the PHPDoc @return say so when the method can also return null instead of an, array, string, or int.
(In case of `getCommandHelp()`, return an empty string as child implementations do return strings.)
Test Plan: Read the code; run static code analysis.
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/D25805
Summary:
Static code analysis can detect `Unreachable statement - code above always terminates.`
The vast majority of occurrences in the Arcanist codebase are due to an unreachable `break` within a `case` after a `return` or after an all-covering `if/else`.
All this noise makes it harder to spot real logic issues (there are some!), thus fix these trivial cases.
Test Plan: Examine the code; run static code analysis.
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/D25803
Summary: This private function was added in https://secure.phabricator.com/rPHU71e8d7a4cf8e9f56b1427c27b3684ae17a3ea7c7 and has never been used.
Test Plan: Grep the code; run static code analysis.
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/D25782
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_string()` as a replacement.
Note: this may highlight other absurd input values that might be worth correcting
instead of just ignoring. If phutil_nonempty_string() throws an exception in your
instance, report it to Phorge to evaluate and fix that specific corner case.
```
ERROR 8192: strlen(): Passing null to parameter #1 ($string) of type string is deprecated at [/var/www/html/phorge/arcanist/src/parser/xhpast/__tests__/PHPASTParserTestCase.php:85]
```
Refs T15926
Test Plan: Either read the code, or probably run something like `arc unit` on D25797?
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15926
Differential Revision: https://we.phorge.it/D25800
Summary:
`parse_ini_string()` has been available since PHP 5.3.0: https://www.php.net/manual/en/function.parse-ini-string.php
`INI_SCANNER_RAW` was introduced in PHP 5.3.0 (and received several bug fixes on the way to PHP 5.4.10): https://www.php.net/ChangeLog-5.php
Phorge requires PHP 7.2 nowadays; before rP7d8c84a7bdc8fb43674341b97c36c9d8ae1d894a Phorge already required PHP 5.5.
Thus remove this outdated check.
Test Plan: Read the docs.
Reviewers: O1 Blessed Committers, speck
Reviewed By: O1 Blessed Committers, speck
Subscribers: tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Differential Revision: https://we.phorge.it/D25808
Summary:
Future developers may want to clean up some code after bumping required versions, so explicitly state that `JSON_UNESCAPED_SLASHES` was introduced in PHP 5.4.0 and that the PHP JSON extension is a core PHP extension since PHP 8.0.0 and cannot be disabled anymore, to save time looking up stuff.
https://www.php.net/ChangeLog-5.php#5.4.0https://www.php.net/manual/en/json.installation.php
Test Plan: Read the docs.
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/D25807
Summary:
Add variable names (`$varname` in `@param type $varname explanation`) to PHPDoc method headers, for fun and profit.
Closes T15923
Test Plan:
* Read the method signatures and their corresponding PHPDoc headers at your fireplace
* Still run `./bin/diviner generate` without explosions (though it is very lenient anyway?)
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15923
Differential Revision: https://we.phorge.it/D25799
Summary:
The question mark in `@param type? $foo Desc` is a custom notation not consistently applied across the codebase and not necessarily obvious to the reader (because custom and not mentioned in https://github.com/php-fig/fig-standards/blob/master/proposed/phpdoc.md ).
Instead, explicitly state "optional" in the parameter description for clarity.
Closes T15925.
Test Plan: Run PHPStan, see no `PHPDoc @param has invalid value (type? [...]` style output anymore.
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15925
Differential Revision: https://we.phorge.it/D25798
Summary:
This revision fixes a `trim(null)` exception when creating a diff with an empty
field.
Fixes: T15868
Test Plan:
1. Create a commit with an empty "Differential Revision:" field
2. Run `arc diff`
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: avivey, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Differential Revision: https://we.phorge.it/D25703
Summary: `PhutilTestCase::tryTestCases()` returns void. Thus `PhutilTestCase::tryTestCaseMap()` should behave the same way as `PhutilTestCase::assertException()`: return void.
Test Plan: Read the code; run static code analysis and don't get "Result of method PhutilTestCase::tryTestCases() (void) is used" anymore.
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/D25791
Summary: `$boot_length = strlen($boot_sequence->toString())` returns an `int` and `strlen()` expects a `string` as a parameter. Thus calling `if (strlen($boot_length) > 8192)` afterwards to get the string length of an integer makes no sense.
Test Plan: Read the code.
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/D25770
Summary:
PHPStan complains that `Method PhutilChannelChannel::readBytes() should return string but return statement is missing.` and `Method PhutilChannelChannel::writeBytes() should return int but return statement is missing.`
All other subclasses of `PhutilChannel` implementing these two methods either provide a return value or directly throw an exception.
`PhutilChannelChannel` throws an exception for both methods but hadn't explicitly declared that as a `return` statement.
Test Plan: Run static code analysis; read/grep the code.
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/D25754
Summary:
rARCbd9769ba92df63d0429f74ec7fb8b00a4989f28b added and declared the three private properties `$id`, `$saveAs`, `$show`.
However rARC21e80a635d798c5be2c6e5c272497b3170c1e079 removed their declarations while still initializing these properties in `didParseArguments()`.
Test Plan: Run PHPStan static code analysis; grep the code in this class.
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/D25713
Summary:
`dropSymbolCache()` does not return anything, and its only call in `rebuild-map.php` does not expect a return value either.
Thus replace `@return this` with `@return void`.
Test Plan: Read/grep the code.
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/D25720
Summary:
`ArcanistAliasEffect::EFFECT_CONFIGURATION` does not exist.
Probably `ArcanistAliasEffect::EFFECT_MISCONFIGURATION` was meant, given the message it sets.
Test Plan: Carefully read the code.
Reviewers: O1 Blessed Committers, avivey
Reviewed By: O1 Blessed Committers, avivey
Subscribers: tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Differential Revision: https://we.phorge.it/D25652
Summary: Upstream version of https://gerrit.wikimedia.org/r/c/phabricator/translations/+/1047593
Test Plan:
Set any languages and observe untranslated strings display proper PLURAL rules
With the downstream "translations" extension installed, set the language to traditional Chinese and see Simplified Chinese rather than English translations if they exist, like "Foo added/removed a project"
Reviewers: O1 Blessed Committers, avivey
Reviewed By: O1 Blessed Committers, avivey
Subscribers: avivey, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Differential Revision: https://we.phorge.it/D25695
Summary: See my comment at T15815#17864. I don't think the translation extractor has any consumers other than the downstream translations extension, and I'd prefer to handle this issue there rather than upstream.
Test Plan: Read the code.
Reviewers: O1 Blessed Committers, aklapper
Reviewed By: O1 Blessed Committers, aklapper
Subscribers: aklapper, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Differential Revision: https://we.phorge.it/D25684
Summary: `PhutilFileLockException` does not exist. Per https://we.phorge.it/source/arcanist/browse/master/src/filesystem/PhutilFileLock.php$54-66 , if lock acquisition fails it is supposed to throw a `PhutilLockException` instead.
Test Plan:
Grep the code base.
Or, run this, from arcanist, from two different terminals:
php ./support/unit/lock.php asd.txt
After this change, the exception is correctly managed.
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/D25641
Summary: `ArcanistWorkingCopyPath::getDescription()` is undefined. Given its use in the exception message, the path is supposed to be used, so call `ArcanistWorkingCopyPath::getPath()` instead.
Test Plan: Carefully read the code and check for existing methods in the class.
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/D25657
Summary:
rARC21e80a635d798c5be2c6e5c272497b3170c1e079 introduced a call to non-existing `Filesystem::removePath()`.
Presumably, `remove()` in `src/filesystem/Filesystem.php` was meant, so use that function instead.
Test Plan: Unknown, apart from reading and grep'ing the code base.
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/D25640
Summary:
When the `Content-Type` HTTP header is empty or missing, `null` is passed to `preg_match()` which is deprecated behavior since PHP 8.1. Thus only call `preg_match()` when the value is set.
```
ERROR 8192: preg_match(): Passing null to parameter #2 ($subject) of type string is deprecated at [$HOME/arcanist/src/future/http/status/HTTPFutureHTTPResponseStatus.php:24]
```
Closes T15821
Test Plan: Visit something using `HTTPFutureHTTPResponseStatus`, like, a profile image. No crashes.
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15821
Differential Revision: https://we.phorge.it/D25632
Summary: Improve PHPDoc of id()
Test Plan: Check the types that are returned by id
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: avivey, speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Differential Revision: https://we.phorge.it/D25576
Summary:
Code checking if the needle string `$value` is somewhere in the haystack `"\0"` makes no sense for a single byte (if it did, then `strcmp` instead of `strpos` should have been used) and the created exception output implies that it's supposed to check that a string does not contain NULL bytes.
Thus switch the order of arguments passed to `strpos()` to be correct.
```
EXCEPTION: (RuntimeException) strpos(): Non-string needles will be interpreted as strings in the future. Use an explicit chr() call to preserve the current behavior at [<arcanist>/src/error/PhutilErrorHandler.php:261]
#0 <#2> PhutilErrorHandler::handleError(integer, string, string, integer, array) called at [<arcanist>/src/error/PhutilErrorHandler.php:261]
#1 <#2> strpos(string, integer) called at [<arcanist>/src/utils/PhutilSortVector.php:33]
```
Closes T15755
Test Plan: Read the surrounding code carefully.
Reviewers: O1 Blessed Committers, valerio.bozzolan, speck
Reviewed By: O1 Blessed Committers, valerio.bozzolan, speck
Subscribers: tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15755
Differential Revision: https://we.phorge.it/D25557
Summary:
Looks like this is all that's needed?
Ref T15751
Test Plan:
R12 has some scenarios for testing this.
Also ran `arc liberate --clean` on arc and phorge repos, and the generated map did not change.
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15751
Differential Revision: https://we.phorge.it/D25551
Summary:
This change updates PhpunitTestEngine.php and ArcanistPhpunitTestResultParser.php to use phpunit's junit output rather than json.
--log-json was deprecated in PHPUnit 5.7, and removed in PHPUnit 6.0.0 (2017-02-03).
Fixes T15667
Test Plan:
Download an example PHP repo, with arcconfig set to 'PhpunitTestEngine'. Example:
```
git clone https://github.com/campbsb/example-phorge-php-project.git
```
Install phpunit using composer, and test it
```
cd example-phorge-php-project
php ~/composer.phar update
phpunit
```
In a PHP phorge repo, set the following in .arcconfig:
```
"unit.engine": "PhpunitTestEngine",
"phpunit_config": "phpunit.xml",
```
Make a non-breaking change to src/Service.php (eg add a space), and run 'arc unit'
Reviewers: O1 Blessed Committers, avivey, speck
Reviewed By: O1 Blessed Committers, avivey, speck
Subscribers: avivey, Ekubischta, speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15667
Differential Revision: https://we.phorge.it/D25472
Summary:
As of February 2024, phurl.io (which is not run by Phorge.it) shows HTTP 503 errors instead of redirecting to the target URIs.
Thus replace any URIs pointing to phurl.io with the corresponding target URIs, based on the mapping listed on https://secure.phabricator.com/phurl/.
Closes T15746
Test Plan: Carefully read the diff.
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15746
Differential Revision: https://we.phorge.it/D25542
Summary:
Ran into `strlen`/`strpos` issues while using Arcanist on Windows:
- Running `arc version` when not in a project/repository directory blew up (may not be specific to Windows).
- Determining mime type of binary file in repository fails.
Refs T15064
Test Plan: Ran diff including a binary file. Ran `arc version` when not in a project/repository directory.
Reviewers: O1 Blessed Committers, avivey
Reviewed By: O1 Blessed Committers, avivey
Subscribers: avivey, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15064
Differential Revision: https://we.phorge.it/D25534
Summary:
Strip a leading UTF-8 Byte Order Mark to avoid silently dropping the first file in a manually uploaded patch.
This change only strips the UTF-8 BOM as UTF-8 is acceptable input.
(Probably non-existing) handling of any other BOMs as first bytes in a diff remains unchanged.
Closes T15452
Test Plan:
Go to `/differential/diff/create/` and upload the test case in T15452 via `Raw Diff From File`.
See two files listed on resulting page `/differential/diff/1/` instead of previously only one file.
Optionally, confirm that byte length of `$diff` is three bytes less now (via `strlen($diff)`).
Reviewers: O1 Blessed Committers, speck
Reviewed By: O1 Blessed Committers, speck
Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15452
Differential Revision: https://we.phorge.it/D25514
Summary:
Premising that "arc commit" is a beautiful Workflow dedicated to svn repositories,
I tried it at work, causing the usual PHP 8.1 deprecation warning:
$ arc diff
$ arc commit
ERROR 8192: preg_replace(): Passing null to parameter #3 ($subject) of type array|string is deprecated at [arcanist/src/workflow/ArcanistWorkflow.php:1520]
arcanist(head=master, ref.master=e46025f7a914)
#0 preg_replace(string, string, NULL) called at [<arcanist>/src/workflow/ArcanistWorkflow.php:1520]
#1 ArcanistWorkflow::normalizeRevisionID(NULL) called at [<arcanist>/src/workflow/ArcanistCommitWorkflow.php:68]
#2 ArcanistCommitWorkflow::run() called at [<arcanist>/scripts/arcanist.php:427]
Usage Exception: Unable to identify the revision in the working copy. Use '--revision <revision_id>' to select a revision.
This bug happens at least when Arcanist does not find any related Revision ID.
It seems there is a method that always normalizes the Revision ID, but sometime that is unknown (null).
And so, NULL ends inside a preg_replace(). It's probably OK to have a normalize method that accept wild
things, including NULL. So, fixed that specific method.
Closes T15693
Test Plan:
This revision was tested in production in my company.
Take a random Subversion repository. Edit a line. Run "arc diff". Then run "arc commit". No warnings.
Reviewers: O1 Blessed Committers, aklapper
Reviewed By: O1 Blessed Committers, aklapper
Subscribers: tobiaswiese, Matthew, Cigaryno
Maniphest Tasks: T15693
Differential Revision: https://we.phorge.it/D25498
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 [<arcanist>/src/error/PhutilErrorHandler.php:261]
arcanist(), ava(), phorge(), wmf-ext-misc()
#0 <#2> PhutilErrorHandler::handleError(integer, string, string, integer) called at [<arcanist>/src/error/PhutilErrorHandler.php:261]
#1 <#2> urlencode(NULL) called at [<arcanist>/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
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
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
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
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
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
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
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
