1
0
Fork 0
mirror of https://we.phorge.it/source/arcanist.git synced 2024-11-08 16:02:39 +01:00
Commit graph

2426 commits

Author SHA1 Message Date
Andre Klapper
4b7ee1985b PHPDoc: Replace "@return this" with "@return $this"
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
2024-09-06 13:01:40 +02:00
Andre Klapper
a094105908 Fix PHP 8.1 "preg_match(null)" exception in XHPASTNode
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
2024-09-06 10:28:24 +02:00
Andre Klapper
c01198d55f Correct PHPDoc @return value of methods that can return null
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
2024-09-05 14:54:54 +02:00
Andre Klapper
b888963f6f Remove trivial cases of unreachable code
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
2024-09-04 12:16:05 +02:00
Andre Klapper
201fc1d981 Correct parameter name of PhpunitTestEngine::parseTestResults()
Summary: Followup to rARC7c5e607e9752a5e5f1e3ddd9bd1907077acb9253

Test Plan: Compare PHPDoc @param name and method signature.

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/D25804
2024-09-04 11:46:46 +02:00
Andre Klapper
9f9413edd2 Remove unused PhutilConsoleProgressSink::getWidth()
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
2024-09-04 11:45:57 +02:00
Andre Klapper
f974927377 Fix PHP 8.1 "strlen(null)" exception in PHPASTParserTestCase
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
2024-09-03 12:08:10 +02:00
Andre Klapper
995072b31f Remove unused ArcanistGitLocalState::getDisplayStashRef()
Summary: This private function is unused.

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/D25783
2024-09-02 08:55:45 +02:00
Andre Klapper
3f893c1484 Remove an outdated PHP 5.3 check in utils
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
2024-08-26 16:36:54 +02:00
Andre Klapper
9f66bff5f6 Clarify comment in PhutilJSON about required PHP version
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.0
https://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
2024-08-26 16:36:24 +02:00
Andre Klapper
04e3e250f7 Add missing variable names to PHPDoc @param of methods
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
2024-08-23 18:52:13 +02:00
Andre Klapper
901f060771 Drop question mark suffix from optional PHPDoc @param types
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
2024-08-23 12:50:14 +02:00
jkim
0c9c94748b Fix PHP 8.1 "trim(null)" exception when creating a diff with an empty "Differential Revision:" field
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
2024-08-22 15:32:59 +01:00
Andre Klapper
0d5f437970 Fix return value of PhutilTestCase::tryTestCaseMap()
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
2024-08-18 09:42:09 +02:00
Andre Klapper
4752567130 Fix $boot_length comparison in PhagePHPAgentBootloader
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
2024-08-15 20:24:47 +02:00
Andre Klapper
06028fad3c Add return statements for PhutilChannelChannel::readBytes()/writeBytes()
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
2024-08-14 14:44:51 +02:00
Andre Klapper
84210cedc6 Declare missing class properties in ArcanistDownloadWorkflow
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
2024-07-10 10:06:35 +02:00
Andre Klapper
76d22d70e4 Correct PHPDoc of dropSymbolCache()
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
2024-07-10 10:06:03 +02:00
Valerio Bozzolan
72e59da804 Fix arc diff in Subversion for non-English languages
Summary:
When using `arc diff`, this crash should not fire anymore on non-English shells:

    Undefined array key "Repository UUID"

The error message comes to this line:

https://we.phorge.it/source/arcanist/browse/master/src/repository/api/ArcanistSubversionAPI.php;f7fcf31c7e23475e345cb3cd4abf0474ba6fd9cf$606

Look at the parser of `svn info` that expects English language:

https://we.phorge.it/source/arcanist/browse/master/src/repository/api/ArcanistSubversionAPI.php;f7fcf31c7e23475e345cb3cd4abf0474ba6fd9cf$326-375

The historical behavior of `ExecFuture` is to inherit environment variables. It may have sense, to have the user home, etc.

So, the approach is to just clear the incompatible specific environment variable that alters the output language.

Closes T15855

Test Plan:
- clone a random a Subversion repository
- have your operating system in a non-English language
- (e.g. `export LANGUAGE=en_US:it_IT`)
- run 'arc diff'

No crash anymore. You are able to submit the patch, just like in git.

Reviewers: O1 Blessed Committers, l2dy, avivey

Reviewed By: O1 Blessed Committers, l2dy, avivey

Subscribers: l2dy, aklapper, tobiaswiese, Matthew, Cigaryno

Maniphest Tasks: T15855

Differential Revision: https://we.phorge.it/D25691
2024-07-08 10:03:32 +02:00
Andre Klapper
331b255b15 Declare missing class properties
Summary: Code in these classes tries to access undefined properties. Thus define these properties.

Test Plan: Run PHPStan static code analysis; grep the code in each 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/D25712
2024-07-03 09:05:34 +02:00
Andre Klapper
21fbc806e4 Fix call to non-existing ArcanistAliasEffect::EFFECT_CONFIGURATION in ArcanistAliasEngine
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
2024-06-23 10:03:01 +02:00
Pppery
0af89f7d32 Add fallback languages for locale files
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
2024-06-21 13:10:22 -04:00
Pppery
f7fcf31c7e Remove PhutilPhtTestCase::getDateTranslations()
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
2024-06-07 15:10:18 -04:00
Andre Klapper
6250296648 Correct call to non-existing PhutilFileLockException in support/unit/lock.php
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
2024-05-30 14:36:03 +02:00
Andre Klapper
3cb117684f Fix calls to non-existing getDescription() in ArcanistWorkingCopyPath
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
2024-05-19 10:28:16 +02:00
Andre Klapper
5477568274 Correct call to non-existing Filesystem::removePath() in ArcanistDownloadWorkflow.php
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
2024-05-18 21:38:57 +02:00
Andre Klapper
7f28d7266f Fix PHP 8.1 "preg_match(null)" exception for missing Content-Type
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
2024-05-10 18:12:59 +02:00
Tim Schumacher
6718b32a64
Improve PHPDoc of id()
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
2024-04-15 16:39:21 +02:00
Andre Klapper
ef73b12b58 Fix "strpos(): Non-string needles will be interpreted as strings" in PhutilSortVector
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
2024-03-22 13:34:55 +01:00
Aviv Eyal
f6261dc614 Arc liberate: support traits
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
2024-03-15 19:58:41 +02:00
sten
7c5e607e97 Update PhpunitTestEngine.php and ArcanistPhpunitTestResultParser.php to use junit output rather than json.
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
2024-03-15 16:09:16 +00:00
Andre Klapper
174bf094ef Replace all phurl.io short URIs with target URIs
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
2024-02-28 14:59:26 +01:00
Christopher Speck
8ef1ead6ac T15064: Several arcanist PHP 8.1 compat issues on Windows
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
2024-02-06 06:56:39 -05:00
Andre Klapper
6c7caf3572 Correct manual upload of Differential patch with a leading BOM
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
2024-01-13 22:05:47 +01:00
Valerio Bozzolan
6142fcd526 Fix Subversion "commit" support in PHP 8.1
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
2023-12-18 11:37:41 +01:00
Andre Klapper
e46025f7a9 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 [<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
2023-12-04 19:28:25 -08:00
Bartosz Dziewoński
16a412b108 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
2023-12-04 22:44:29 +01:00
Aviv Eyal
25611ba24a 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
2023-11-27 20:32:12 +02:00
sten
5bc53cfe53 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
2023-11-14 15:05:33 +00:00
Aviv Eyal
98d16d27cf 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
2023-10-12 19:21:56 +03:00
sten
ba42b63704 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
2023-09-28 09:12:29 +01:00
sten
35e127da57 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
2023-09-12 15:57:15 +01:00
Aviv Eyal
d343be5926 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
2023-08-31 08:14:16 -07:00
sten
8b907d7716 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
2023-08-18 14:57:54 +01:00
bob
df6c315ace 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
2023-08-14 10:58:01 +02:00
Aviv Eyal
6832afc300 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
2023-08-12 08:43:46 -07:00
Aviv Eyal
726b148afc 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
2023-08-12 08:41:53 -07:00
Andre Klapper
788098096e 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
2023-07-29 12:20:40 +03:00
Aviv Eyal
6c6f47bf9c (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
<?php

require_once './arcanist/scripts/__init_script__.php';

$valid_inputs = array(
  'foo',
  null,
  new PhutilURI('http://0@domain.com/'),
);

$invalid_inputs = array(
  9,
  new Filesystem(),
  array(),
);

echo "Valid inputs: \n";
foreach ($valid_inputs as $v) {
  echo("Checking ".gettype($v)." \n");
  PhutilURI::checkHrefType($v);
}

echo "\nError inputs: \n";
foreach ($invalid_inputs as $v) {
  echo("Checking ".gettype($v)." \n");
  PhutilURI::checkHrefType($v);
}

echo "\n";
```

Reviewers: O1 Blessed Committers, valerio.bozzolan

Reviewed By: O1 Blessed Committers, valerio.bozzolan

Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno

Maniphest Tasks: T15316

Differential Revision: https://we.phorge.it/D25356
2023-07-24 12:33:47 -07:00
Aviv Eyal
8a2cb75d63 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
2023-07-21 09:45:55 -07:00