The way things were set up now in the `phpunit` job, no matter whether tests passed or failed, the workflow would always continue.
I suspect this may have been set-up this way to make sure that all variations of test runs will actually be run ?
The downside is that, while you will see a ❌ for the individual build in the workflow summary, the workflow will not be marked as failed, nor will the individual test builds be marked as failed.
If we look back at the last time the workflow was run completely, this can be seen in the annotations below the summary, which show that the Windows test runs all failed.
https://github.com/phpDocumentor/ReflectionDocBlock/actions/runs/370378454
I'm proposing to change this now by:
* Removing the `continue-on-error` for the test run.
* Adding the `fail-fast` key and setting it to `false`.
By default this key is set to `true`, which means that if any individual build within the job fails, all other builds within the job will be cancelled.
By setting it to `false`, all builds in the matrix will still be run, but if any of them fail, the workflow will be marked as "failed".
The original php method preg_split might return `string[][]` in some
situations, however this should never be the case for this library. By
adding an extra assert, we should be more safe.
As discussed in the PR:
> > Argument 4 of preg_split expects 0|1|2|3|4|5|6|7, parent type int provided (see https://psalm.dev/193)
>
> I believe this issue is for the `phpDocumentor\Reflection\Utils` class and expects the `pregSplit()` method to apply input validation to the value received for `$flags` before passing it off to the PHP native `preg_split()` function.
>
> IMO that's taking things a little too far as PHP will handle this internally without errors.
> See: https://3v4l.org/NdDRK
The current version of Psalm flags the following issues:
```
ERROR: InvalidReturnType - src\Utils.php:44:16 - The declared return type 'array<array-key, string>' for phpDocumentor\Reflection\Utils::pregSplit is incorrect, got 'list<list<int|string>|string>' (see https://psalm.dev/011)
* @return string[] Returns an array containing substrings of subject split along boundaries matched by pattern
ERROR: InvalidReturnStatement - src\Utils.php:55:16 - The inferred type 'list<list<int|string>|string>' does not match the declared return type 'array<array-key, string>' for phpDocumentor\Reflection\Utils::pregSplit (see https://psalm.dev/128)
return $parts;
```
I'm suggest ignoring this as `list` isn't an officially supported type.
Psalm flags this condition as redundant:
```
ERROR: RedundantCondition - src/DocBlock/Tags/Return_.php:62:48 - "" can never contain non-empty-lowercase-string (see https://psalm.dev/122)
return $type . ($description !== '' ? ($type !== '' ? ' ' : '') . $description : '');
```
Based on the statement in the line above - `$type = $this->type ? '' . $this->type : 'mixed';` -, Psalm is correct and the `$type` variable can never be an empty string.
Psalm flags these type casts as redundant:
```
ERROR: RedundantCastGivenDocblockType - src/DocBlock/Tags/Author.php:80:23 - Redundant cast to string given docblock-provided type (see https://psalm.dev/263)
$authorName = (string) $this->authorName;
ERROR: RedundantCastGivenDocblockType - src/DocBlock/Tags/Example.php:150:21 - Redundant cast to string given docblock-provided type (see https://psalm.dev/263)
$filePath = (string) $this->filePath;
ERROR: RedundantCastGivenDocblockType - src/DocBlock/Tags/Link.php:74:17 - Redundant cast to string given docblock-provided type (see https://psalm.dev/263)
$link = (string) $this->link;
ERROR: RedundantCastGivenDocblockType - src/DocBlock/Tags/Method.php:228:23 - Redundant cast to string given docblock-provided type (see https://psalm.dev/263)
$methodName = (string) $this->methodName;
```
I have verified each and can confirm that these are redundant. They are probably a left-over from the time when the `__construct()` method in these classes did not yet have type declarations.
Correctly flagged by Psalm:
```
ERROR: PossiblyNullArgument - src\Utils.php:50:53 - Argument 3 of preg_split cannot be null, possibly null value provided (see https://psalm.dev/078)
$parts = php_preg_split($pattern, $subject, $limit, $flags);
```
The `$limit` argument of the PHP native `preg_split()` function is not nullable.
Ref: https://www.php.net/manual/en/function.preg-split
This switches the installation method for Psalm from Phive to Composer, while still using a Phar file for running Psalm.
Includes:
* Removing Psalm from the Phive configuration.
* Adding Psalm to the Composer configuration. Includes upgrading from version `3.11.2` to version `4.8.1`.
* Adjusting the script used in the `Makefile`.
👉 Please verify and test this as things work differently on different OS-es and this should work for you.
* Adjusting the GH Actions script to use the Composer installed version of Psalm.
Note: due to the committed `composer.lock` file, Psalm will not automatically upgrade when newer versions are available.
Refs:
* https://github.com/vimeo/psalm/releases
* https://github.com/psalm/phar/releases
* Fix the name and description to prevent confusion between the project ruleset and the organisation ruleset.
* Set the minimum PHP version for the PHPCompatibility standard.
* Don't require property type declarations.
* Ensure special characters used as literals in an exclude pattern are escaped.
PHPStan flags the code within the `Source::__toString()` method:
```
------ -------------------------------------------------------------------------------------------
Line DocBlock\Tags\Source.php
------ -------------------------------------------------------------------------------------------
111 Result of || is always true.
114 Result of || is always true.
114 Result of || is always true.
------ -------------------------------------------------------------------------------------------
```
I have investigated this and can confirm that these flags are correct.
1. `$this->startingLine` is cast to an integer in the `__construct()` method (line 45) and subsequently cast to a string in `__toString()` (line 105).
This means that it can only ever be a non-empty ("truthy") string or the string '0', so the `$startingLine || $startingLine === '0'` condition used in two places is redundant.
2. `$this->lineCount` is either an integer or `null` after the `__construct()` method (line 46).
In the `__toString()` method, if the `lineCount` is an integer, it is effectively cast to a string by the concatenation with an empty string on line 107, while if the `lineCount` was `null`, it is turned into an empty string.
By changing the concatenation from concatenating with an empty string to concatenating with a one-space string, we can remove the ternary in the `return` statement checking for `$lineCount` being empty.
The existing unit tests already cover this code and still pass after this change.
... or rather add the missing `@coversNothing` as this is an integration test.
This prevents the test from being marked as "risky" due to the missing tag on PHPUnit 9.x:
```
There was 1 risky test:
1) phpDocumentor\Reflection\DocblockSeeTagResolvingTest::testResolvesSeeFQSENOfInlineTags
This test does not have a @covers annotation but is expected to have one
```
... to prevent it from being considered "risky" and being listed as "risky" below each test run.
```
There was 1 risky test:
1) phpDocumentor\Reflection\ModifyBackTraceSafeTest::testBackTraceModificationDoesNotImpactFunctionArguments
This test did not perform any assertions
/home/runner/work/ReflectionDocBlock/ReflectionDocBlock/tests/integration/ModifyBackTraceSafeTest.php:15
```
Triggering a workflow for a branch manually is not supported by default in GH Actions, but has to be explicitly allowed.
This is useful if, for instance, an external action script or composer dependency has broken.
Once a fix is available, failing builds for open PRs can be retriggered manually instead of having to be re-pushed to retrigger the workflow.
Ref: https://github.blog/changelog/2020-07-06-github-actions-manual-triggers-with-workflow_dispatch/
Jaap van Otterdijk
dependabot[bot]
jrfnl