Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Scrutinizer Auto-Fixes #1

Merged
merged 1 commit into from
Jun 1, 2019
Merged

Scrutinizer Auto-Fixes #1

merged 1 commit into from
Jun 1, 2019

Conversation

scrutinizer-auto-fixer
Copy link
Collaborator

@hemberger requested this pull request.

It consists of patches automatically generated for this project on Scrutinizer:
https://scrutinizer-ci.com/g/smrealms/smr/

This commit consists of patches automatically generated for this project on https://scrutinizer-ci.com
@hemberger hemberger merged commit 13a5486 into misc-fixes Jun 1, 2019
@hemberger hemberger deleted the scrutinizer-patch-1 branch June 1, 2019 20:13
hemberger added a commit that referenced this pull request Apr 25, 2021
Fixes the phpstan warning:

> Method DummyPlayer::getShip() overrides method AbstractSmrPlayer::getShip() but misses parameter #1 $forceUpdate.
hemberger added a commit that referenced this pull request Jun 14, 2021
We need to be able to get the session var before we process any pages,
but it is during page processing where we set the session var. Thus,
we need to initialize the session var to an empty Page (as a kind of
bootstrapping strategy).

This fixes the following error:

> Error Message: ErrorException: Undefined array key "" in /smr/src/lib/Smr/Session.php:259
> Stack trace:
> #0 /smr/src/lib/Smr/Session.php(259): exception_error_handler(2, 'Undefined array...', '/smr/src/lib/Sm...', 259)
> #1 /smr/src/lib/Default/AbstractSmrPlayer.class.php(2169): Smr\Session->getCurrentVar()
> #2 /smr/src/tools/npc/npc.php(140): AbstractSmrPlayer->removeUnderAttack()
> #3 /smr/src/tools/npc/npc.php(91): NPCStuff()
> #4 {main}
hemberger added a commit that referenced this pull request Jun 14, 2021
The shop_hardware_processing.php script forwards the current LocationID
in the session var to the next page. This was not being set as part of
the preparation in npc.php (because the LocationID is not needed by
anything the NPC does, i.e. no subsequent display page). However, it is
no significant burden to provide this, so we pass along the sector ID
to the `doUNO` function.

Fixes the following error:

> Error Message: Exception: Could not find "LocationID" in var! in /smr/src/lib/Default/Page.class.php:166
> Stack trace:
> #0 /smr/src/engine/Default/shop_hardware_processing.php(64): Page->addVar('LocationID')
> #1 /smr/src/lib/Default/Page.class.php(231): require('/smr/src/engine...')
> #2 /smr/src/lib/Default/smr.inc.php(372): Page->process()
> #3 /smr/src/tools/npc/npc.php(352): do_voodoo()
> #4 /smr/src/tools/npc/npc.php(176): processContainer(Object(Page))
> #5 /smr/src/tools/npc/npc.php(91): NPCStuff()
> #6 {main}
hemberger added a commit that referenced this pull request Aug 20, 2021
In the `displayGrouped` and `displayMessage` functions, we now return
the constructed message (an array) instead of passing in the message
box by reference and appending to it inside the functions.

This fixes an issue with scout messages where the "GroupedMessages"
element did not yet exist, but we were passing it by reference to the
`displayMessage` function as if it were an array. This caused the
following error:

> TypeError: displayMessage(): Argument #1 ($messageBox) must be of type array, null given
hemberger added a commit that referenced this pull request Aug 21, 2021
* Return `$player` instead of returning it by reference. This change
  is needed because the value is `null` when it is passed in, which
  breaks the type requirement `AbstractSmrPlayer`. Fixes the error:

    > Unncaught TypeError: check_for_registration(): Argument #1
    > ($player) must be of type AbstractSmrPlayer, null given

* If the registration check fails, we return `false` instead of `true`
  so that we can use the union return type `AbstractSmrPlayer|false`.

* Use a `callable` for the `$callback` argument instead of a string
  representation that must be run with `eval`.
hemberger added a commit that referenced this pull request Sep 11, 2021
* Return `$player` instead of returning it by reference. This change
  is needed because the value is `null` when it is passed in, which
  breaks the type requirement `AbstractSmrPlayer`. Fixes the error:

    > Unncaught TypeError: check_for_registration(): Argument #1
    > ($player) must be of type AbstractSmrPlayer, null given

* If the registration check fails, we return `false` instead of `true`
  so that we can use the union return type `AbstractSmrPlayer|false`.

* Use a `callable` for the `$callback` argument instead of a string
  representation that must be run with `eval`.
hemberger added a commit that referenced this pull request Apr 11, 2022
Setting NPC_SCRIPT as a PHP constant made it difficult to test classes
that depended on it. This is because PHPUnit has no mechanism to change
the value of a PHP constant, apart from `@runInSeparateProcess`, and
then setting the constant to a different value in that process.

Unfortunately, `@runInSeparateProcess` interacted poorly with PHP-DI
in a way that I don't completely understand. When running tests using
this decorator, the following error would be triggered:

  PHPUnit\Framework\Exception: PHP Fatal error:  Uncaught Error: Using $this when not in object context in /smr/vendor/php-di/php-di/src/Compiler/Template.php:4
  Stack trace:
  #0 Standard input code(1154): require_once()
  #1 {main}
    thrown in /smr/vendor/php-di/php-di/src/Compiler/Template.php on line 4
  Fatal error: Uncaught Error: Using $this when not in object context in /smr/vendor/php-di/php-di/src/Compiler/Template.php:4
  Stack trace:
  #0 Standard input code(1154): require_once()
  #1 {main}
    thrown in /smr/vendor/php-di/php-di/src/Compiler/Template.php on line 4

This error always occurred when running the associated test files
directly, but in an upcoming class `SectorLock`, it also occurred when
running the entire test suite, which is obviously a blocking issue.

To fix this, we convert the PHP constant `NPC_SCRIPT` into a PHP-DI
variable with the same name. This allows us to change its value more
easily, and particularly at runtime instead of compile time. In some
sense this is an abuse of the DI container, since we're not actually
using it for DI -- rather just for global variable storage. Oh well.
hemberger added a commit that referenced this pull request Apr 11, 2022
Setting NPC_SCRIPT as a PHP constant made it difficult to test classes
that depended on it. This is because PHPUnit has no mechanism to change
the value of a PHP constant, apart from `@runInSeparateProcess`, and
then setting the constant to a different value in that process.

Unfortunately, `@runInSeparateProcess` interacted poorly with PHP-DI
in a way that I don't completely understand. When running tests using
this decorator, the following error would be triggered:

  PHPUnit\Framework\Exception: PHP Fatal error:  Uncaught Error: Using $this when not in object context in /smr/vendor/php-di/php-di/src/Compiler/Template.php:4
  Stack trace:
  #0 Standard input code(1154): require_once()
  #1 {main}
    thrown in /smr/vendor/php-di/php-di/src/Compiler/Template.php on line 4
  Fatal error: Uncaught Error: Using $this when not in object context in /smr/vendor/php-di/php-di/src/Compiler/Template.php:4
  Stack trace:
  #0 Standard input code(1154): require_once()
  #1 {main}
    thrown in /smr/vendor/php-di/php-di/src/Compiler/Template.php on line 4

This error always occurred when running the associated test files
directly, but in an upcoming class `SectorLock`, it also occurred when
running the entire test suite, which is obviously a blocking issue.

To fix this, we convert the PHP constant `NPC_SCRIPT` into a PHP-DI
variable with the same name. This allows us to change its value more
easily, and particularly at runtime instead of compile time. In some
sense this is an abuse of the DI container, since we're not actually
using it for DI -- rather just for global variable storage. Oh well.
hemberger added a commit that referenced this pull request Jul 5, 2022
We would like to be able to use an enum to narrow the type of sector
link directions, but cannot because they are used as array keys.

Restricting their possible values via a special PHPStan docstring type
turns out to be unhelpful, because the docstring would need to
propagate to many more function interfaces, and yet we still wouldn't
actually be able to enforce this restriction anywhere (since we would
ultimately need to assume that some input variable was of this type).

Therefore, we fall back to simply throwing an Exception if an invalid
value is used.

Fixes PHPStan warnings of the type:

> Parameter #1 $dir of method SmrSector::setLinkSector() expects 'Down'|'Left'|'Right'|'Up', string given.
hemberger added a commit that referenced this pull request Jul 5, 2022
> Parameter #1 $callback of function spl_autoload_register expects
> (callable(string): void)|null, Closure(string): bool given.

Two changes were needed to match the parameter type expected by
PHPStan:

1. Promote the 'get_class_loc' string to first-class callable syntax
   (see smrealms#1183).

2. Remove the boolean return value from `get_class_loc`. This was a
   "speculative" feature added in 3f6854f, but it doesn't conform to
   the expected interface nor is it needed for the autoloader to throw
   an Error if the class is not found.
hemberger added a commit that referenced this pull request Jul 5, 2022
Fixes PHPStan warning (and subsequent warnings):

> Parameter #1 $forces of method SmrCombatDrones::shootPlayerAsPort() expects SmrPort, $this(AbstractSmrPort) given.

This was the result of passing `$this` from within AbstractSmrPort.
hemberger added a commit that referenced this pull request Jul 5, 2022
These were added to prevent PHPStan unmatched type warnings, which was
helpful for Level 4, but at Level 5 these type requirements propagate
to the callers, which cannot always easily enforce these requirements.
Therefore, we switch to throwing in the default match arm instead.

Fixes the following PHPStan warnings:

> Parameter #1 $pieceID of static method Smr\Chess\ChessPiece::getLetterForPiece() expects 1|2|3|4|5|6, int given.
> Parameter #1 $letter of static method Smr\Chess\ChessPiece::getPieceForLetter() expects 'B'|'b'|'K'|'k'|'N'|'n'|'P'|'p'|'Q'|'q'|'R'|'r', string given.
hemberger added a commit that referenced this pull request Jul 10, 2022
Fixes PHPStan warning (and subsequent warnings):

> Parameter #1 $port of method SmrWeapon::shootPlayerAsPort() expects SmrPort, $this(AbstractSmrPort) given.

This was the result of passing `$this` from within AbstractSmrPort,
and is related to 399041c.
hemberger added a commit that referenced this pull request Aug 8, 2022
Since we are setting these var values in PHP code (rather than getting
them from user input), we don't need to validate their values.

Fixes PHPStan warnings:

> Parameter #1 $orderID of method AbstractSmrShip::moveWeaponUp() expects int, float|int|string given.
> Parameter #1 $orderID of method AbstractSmrShip::moveWeaponDown() expects int, float|int|string given.
hemberger added a commit that referenced this pull request Aug 8, 2022
Fixes PHPStan warning:

> Parameter #1 $num of function number_format expects float,
> float|int|string given.

By properly defining the Deposit/Withdrawal display value in the
engine file, we can unconditionally display its value in the template.
hemberger added a commit that referenced this pull request Aug 8, 2022
Fixes PHPStan warning:

> Parameter #1 $str of method Smr\Template::convertHtmlToAjaxXml()
> expects string, string|false given.

If output buffering is not turned on, then the template will not be
able to display anything.
hemberger added a commit that referenced this pull request Aug 8, 2022
We have a constraint on the return type that it must be the same as
the type of the input array key. We use the `@template` syntax to
achieve this.

Fixes PHPStan warnings of the type:

> Parameter #1 $buildingTypeID of method SmrPlanet::destroyBuilding()
> expects int, int|string given.
hemberger added a commit that referenced this pull request Jan 8, 2023
The return value of `fgets` can be false if an error occurred.

Fixes PHPStan warning:

> Parameter #1 $string of function trim expects string, string|false given.
hemberger added a commit that referenced this pull request Jan 8, 2023
The return value of `gzcompress` can be false if an error occurred.

Fixes PHPStan warning:

> Parameter #1 $binary of method Smr\Database::escapeBinary() expects string, string|false given.
hemberger added a commit that referenced this pull request Jan 8, 2023
The PHP functions `file_get_contents` and `parse_ini_string` can return
false if they fail. If the former does, it's an internal error, so we
throw an Exception. If the latter does, it indicates the SMR file may
be malformed, so we throw a UserError.

Fixes PHPStan warnings:

> Parameter #1 $haystack of function strstr expects string, string|false given.
> Cannot access offset 'Metadata' on array|false.
> Cannot access offset 'Galaxies' on array|false.
> Argument of an invalid type array|false supplied for foreach, only iterables are supported.
hemberger added a commit that referenced this pull request Jan 8, 2023
Array shapes provide a more precise constraint for an array return,
and so should be used whenever possible.

Fixes PHPStan warning:

> Parameter #1 $rank of static method Smr\HallOfFame::displayHOFRow() expects int, float|int given.
hemberger added a commit that referenced this pull request Jan 8, 2023
PHP functions often return false if they fail. It's important to check
for this return value, both so that we can throw a helpful Exception,
and so that we don't use a false value improperly down the line.

Fixes PHPStan warnings:

> Method Smr\Template::addJavascriptForAjax() should return string but returns string|false.
> Argument of an invalid type DOMNodeList<DOMNode>|false supplied for foreach, only iterables are supported.
> Parameter #1 $value of function count expects array|Countable, DOMNodeList<DOMNode>|false given.
hemberger added a commit that referenced this pull request Jan 8, 2023
Since all we want to do is split a string on a delimiter, and then trim
the results, we can do that much more explicitly with the `explode` and
`trim` methods. With `preg_split`, we would have needed to validate
that the result did not return false (which also would have prevented
us from using array destructuring as well).

Fixes the following PHPStan warnings:

>  158    Cannot use array destructuring on array<int, string>|false.
>  187    Parameter #1 $array of method Smr\Database::escapeArray() expects array, array<int, string>|false given.
>  195    Parameter #1 $array of method Smr\Database::escapeArray() expects array, array<int, string>|false given.

Unrelatedly, fixed an issue with player names not being escaped
properly for display.
hemberger added a commit that referenced this pull request Jan 8, 2023
We cannot pass an int value of `$X` to the `str2int` method, because it
only accepts strings. This regression was introduced in 7330d7a,
and was causing an error when accepting missions (since that's the only
place we currently pass an int `$X`).

We may want to consider changing `str2int` to allow other types, e.g.
int or float or even mixed, as the implementation works for any type
(but then obviously we'd need to rename the function).

Fixes the PHPStan warning:

> Parameter #1 $val of function str2int expects string, int|string given.
hemberger added a commit that referenced this pull request Jan 8, 2023
Use the FILTER_VALIDATE_IP flag of `filter_var` on the IP address to
avoid warnings from calling `gethostbyaddr` on invalid IP addresses.
We still fall back to "unknown" for the host if `gethostbyaddr` fails
(though I'm not sure if that's possible for validated IPs).

Fixes PHPStan warnings:

> Cannot use array destructuring on array<int, string>|false.
> Parameter #1 $string of method Smr\Database::escapeString() expects string|null, string|false given.
hemberger added a commit that referenced this pull request Jan 8, 2023
While we don't expect the database field `player_has_notes.note` to be
uncompressed, it is safer to make sure that it uncompresses properly.

Fixes PHPStan warning:

> Parameter #1 $message of function bbifyMessage expects string, string|false given.
hemberger added a commit that referenced this pull request Jan 8, 2023
Use an explicit array shape instead of `mixed` for the return type of
the SmrAccount::getIndividualScores method.

Fixes PHPStan warning:

> Parameter #1 $val of function IRound expects float, array|int given.
hemberger added a commit that referenced this pull request Jan 8, 2023
We don't need to include the bounty that the killer gained in the array
that we return in `killPlayerByPlayer`. This allows us to simplify the
logic a bit by using local variables.

Fixes PHPStan warning:

> Parameter #1 $type of method AbstractSmrPlayer::increaseCurrentBountyAmount() expects Smr\BountyType, Smr\BountyType::HQ|Smr\BountyType::UG|string given.
hemberger added a commit that referenced this pull request Jan 8, 2023
Narrow the typehint for "weapon data" arrays by using an explicit array
shape, since each field can only ever be a single type.

This fixes the following PHPStan warnings:

> Parameter #1 $damage of method AbstractSmrPort::takeDamageToShields() expects int, bool|int given.
> Parameter #1 $damage of method AbstractSmrShip::takeDamageToShields() expects int, bool|int given.
> Parameter #1 $damage of method SmrForce::takeDamageToMines() expects int, bool|int given.
> Parameter #1 $damage of method SmrPlanet::takeDamageToShields() expects int, bool|int given.

Ideally, we would convert this array to a data class, but the combat
templates are sufficiently fragile that I wanted to make the smallest
changes possible to fix the warnings, which meant only changing the
docstrings.

To avoid repeating a complex array shape in many places, we use the
PHPStan `typeAliases` configuration option, which allows us to specify
the type once, and then use it in docstrings as if it were a real type.
hemberger added a commit that referenced this pull request Jan 8, 2023
Fixes PHPStan warning:

> Parameter #1 $length of function random_bytes expects int<1, max>, (float|int) given.
hemberger added a commit that referenced this pull request Jan 8, 2023
The return value of `gzcompress` can be false if an error occurs.

Fixes PHPStan warning:

> Parameter #1 $binary of method Smr\Database::escapeBinary() expects string, string|false given.
hemberger added a commit that referenced this pull request Jan 8, 2023
We added validation of the result for the `write` method during the
major Database refactor in 7054e3d, but the `read` method was not
similarly modified.

For the `read` method, we expect a `mysqli_result`. If it returns bool
instead, then it failed (false) or was the wrong query type (true).

Fixes PHPStan warning:

> Parameter #1 $dbResult of class Smr\DatabaseResult constructor expects mysqli_result, bool|mysqli_result given.
hemberger added a commit that referenced this pull request Jan 26, 2023
Now that we are using strict types in templates, we cannot pass null
to `htmlspecialchars`, as it only takes strings. This was causing an
error on the page for any alliance with a null Discord server/channel.

Fixes PHPStan warnings of the type:

> Parameter #1 $string of function htmlspecialchars expects string, string|null given.
hemberger added a commit that referenced this pull request Feb 9, 2023
There are often large portions of a page that are displayed as a result
of a condition being met (in this case, if we've selected a ship to
compare against). This usually results in defining a whole collection
of template variables together, or not.

However, the template doesn't know about these dependencies. It only
knows that we've checked one condition (`isset($CompareShip)` in this
case), and then left a bunch of variables potentially undefined.

Until we have a more comprehensive solution for the static analysis of
templates, the best we can probably do is assert that _all_ variables
in the collection are defined. This may be a bit redundant and even
absurd in some cases, but it is certainly more explicit.

Fixes PHPStan warnings of the type:

> Parameter #1 $message of function bbifyMessage expects string, string|null given.
> Argument of an invalid type array<string, string>|null supplied for foreach, only iterables are supported.
> Parameter #1 $num of function number_format expects float, int|null given.
hemberger added a commit that referenced this pull request Feb 9, 2023
Use the existence of the `TotalFine` template variable to determine
whether or not illegals were found.

Fixes PHPStan warning:

> Parameter #1 $num of function number_format expects float, int|null given.
hemberger added a commit that referenced this pull request Feb 9, 2023
PHPStan does not seem to understand that if a variable is an array key
of an array that only takes strings as keys, then that variable must be
a string. It is simple enough to additionally guard against it being
null.

Fixes PHPStan warning:

> Parameter #1 $viewType of static method Smr\HallOfFame::getHofRank() expects string, string|null given.
hemberger added a commit that referenced this pull request Feb 9, 2023
When defining the "Other Info" for history games in the Summary class,
construct the database query in such a way that we can safely return
the result even if there are no players. We do this by adding IFNULL
checks on the min/max operations in the query. As a result, we can
remove the nullable from the template typehints.

Fixes PHPStan warnings of the type:

> Parameter #1 $num of function number_format expects float, int|null given.

Note that we make the query identical to the same page for non-history
games in the GameStats class. There, we remove an unnecessary ORDER BY
from the query.
hemberger added a commit that referenced this pull request Feb 9, 2023
This replaces the second parameter of the `escapeString` method. It is
more user-friendly to have a separate method for nullable input than to
overload one method with two purposes.

Furthermore, there's no way to tell PHPStan that the first argument of
`escapeString` is only allowed to be null if the 2nd argument is true.

Fixes PHPStan warning:

> Parameter #1 $string of method mysqli::real_escape_string() expects string, string|null given.
hemberger added a commit that referenced this pull request Feb 9, 2023
Eschew the additional `valid` property in favor of dynamically checking
that both `userID` and `email` are non-empty in `isValid()`.

This allows us to provide a more direct relationship between the result
of `isValid()` and the return types of `getUserID()` and `getEmail()`.

Fixes PHPStan warning in Account:

> Parameter #1 $string of method Smr\Database::escapeString() expects string, string|null given.
hemberger added a commit that referenced this pull request Feb 9, 2023
PHPStan cannot remember that `$doAjaxMiddle` is false if `$mid` is
null, so it doesn't understand that `$mid` is guaranteed to be a
DOMElement when used as such.

That said, it's probably more straightforward for humans as well to
scope that entire logic inside a `$mid !==` null check.

Fixes PHPStan warnings:

> Parameter #1 $node of closure expects DOMNode, DOMElement|null given.
> Cannot call method getAttribute() on DOMElement|null.
hemberger added a commit that referenced this pull request Feb 9, 2023
Check that the record fetched from the mysqli_result is not null in the
`record` method. This could occur if, for example, the method is called
twice on the same DatabaseResult (since by definition it only has one
record).

Fixes PHPStan warning:

> Parameter #1 $dbRecord of class Smr\DatabaseRecord constructor expects array<string, mixed>, array<string, string>|null given.
hemberger added a commit that referenced this pull request Feb 9, 2023
Since PHPStan cannot remember that `$MapPlayer` is null iff `$UniGen`
is true, we need to rely more on checks against `$MapPlayer` so that
PHPStan knows we're not trying to call Player methods on null.

Fixes the following PHPStan warnings:

> Cannot call method getAlliance() on Smr\Player|null.
> Cannot call method isPartOfCourse() on Smr\Player|null.
> Parameter #1 $player of method Smr\Sector::hasPlayerForces() expects Smr\AbstractPlayer, Smr\Player|null given.
> Cannot call method isFlagship() on Smr\Player|null.
hemberger added a commit that referenced this pull request Feb 9, 2023
The methods `hasCachedPort` and `hasFriendlyForces` return false if a
null argument is given. We need a way to express this relationship with
PHPStan. Two annotations that did NOT work:

1. @return ($player is null ? false : bool)

This seems to express the relationship most clearly, but unfortunately
it did not actually narrow the type of `$player`.

2. @phpstan-assert-if-true !null $player

This had the surprising behavior of also asserting the inverse -- that
if the function returned false, then `$player` was not null, which we
obviously do not want. Presumably because of how "assertions" are used
traditionally (in a simple function with no complex logic), this is a
desirable side effect (phpstan/phpstan#8351).

As alluded to in the linked PHPStan issue, we can achieve the desired
behavior by exploiting "equality assertions", for which the inverse is
not also asserted. With this, we can narrow the type of `$player` to
AbstractPlayer (and thus `!null`) if the function returns true. See
https://psalm.dev/docs/annotating_code/assertion_syntax/#equality-assertions.

Fixes from SectorMap.inc.php:

> Parameter #1 $player of method Smr\Sector::getCachedPort() expects Smr\AbstractPlayer, Smr\Player|null given.

And from SectorsFile.php:

> Parameter #1 $player of method Smr\Sector::getFriendlyForces() expects Smr\AbstractPlayer, Smr\AbstractPlayer|null given.
hemberger added a commit that referenced this pull request Feb 9, 2023
Use the same logic added to SectorMap.inc.php in be2da56.

> Parameter #1 $player of method Smr\Sector::getCachedPort() expects Smr\AbstractPlayer, Smr\AbstractPlayer|null given.
hemberger added a commit that referenced this pull request Feb 9, 2023
This allows the page to display even if there are no valid games for
which NPCs can be created, since we expect the game ID to be an int,
and there is never a real game ID of 0.

Alternatively, we could have skipped a lot of the logic if the game ID
was null, but that is a more involved refactor (which creates other
problems with conditionally defined template variables).

Fixes PHPStan warnings:

> Parameter #1 $selectedGameID of class Smr\Pages\Admin\NpcManageAddAccountProcessor constructor expects int, int|null given.
> Parameter $selectedGameID of class Smr\Pages\Admin\NpcManageProcessor constructor expects int, int|null given.
> Parameter #2 $gameID of static method Smr\AbstractPlayer::getPlayer() expects int, int|null given.
hemberger added a commit that referenced this pull request Feb 9, 2023
These constructor arguments come as a package - either they are all set
or they are all null. It's tricky to demand this relationship when they
are all separately nullable arguments. This suggests that they should
be grouped (ideally in a data class, but we use an array for simplicity
here, which is made much safer with PHPStan array shapes).

Fixes PHPStan warning:

> Parameter #1 $bytes of closure expects int, int|null given.
hemberger added a commit that referenced this pull request Feb 9, 2023
The constructor arguments are only nullable under certain conditions,
due to relationships with other args that are cumbersome to enumerate.
This suggests that the class needs to be refactored, but until then,
using an empty array for some of the arguments fixes the following
PHPStan warnings:

> Parameter #1 $array of method Smr\Database::escapeArray() expects array, array<int>|null given.
> Offset 0 does not exist on array<int>|null.
> Offset 1 does not exist on array<int>|null.

This is kind of a cop out, as it doesn't address the underlying dangers
in the design, but it does placate PHPStan.
hemberger added a commit that referenced this pull request Feb 9, 2023
PHPStan cannot remember that `$doAjaxMiddle` is false if `$mid` is
null, so it doesn't understand that `$mid` is guaranteed to be a
DOMElement when used as such.

That said, it's probably more straightforward for humans as well to
scope that entire logic inside a `$mid !==` null check.

Fixes PHPStan warnings:

> Parameter #1 $node of closure expects DOMNode, DOMElement|null given.
> Cannot call method getAttribute() on DOMElement|null.
hemberger added a commit that referenced this pull request Feb 9, 2023
Check that the record fetched from the mysqli_result is not null in the
`record` method. This could occur if, for example, the method is called
twice on the same DatabaseResult (since by definition it only has one
record).

Fixes PHPStan warning:

> Parameter #1 $dbRecord of class Smr\DatabaseRecord constructor expects array<string, mixed>, array<string, string>|null given.
hemberger added a commit that referenced this pull request Feb 9, 2023
Since PHPStan cannot remember that `$MapPlayer` is null iff `$UniGen`
is true, we need to rely more on checks against `$MapPlayer` so that
PHPStan knows we're not trying to call Player methods on null.

Fixes the following PHPStan warnings:

> Cannot call method getAlliance() on Smr\Player|null.
> Cannot call method isPartOfCourse() on Smr\Player|null.
> Parameter #1 $player of method Smr\Sector::hasPlayerForces() expects Smr\AbstractPlayer, Smr\Player|null given.
> Cannot call method isFlagship() on Smr\Player|null.
hemberger added a commit that referenced this pull request Feb 9, 2023
The methods `hasCachedPort` and `hasFriendlyForces` return false if a
null argument is given. We need a way to express this relationship with
PHPStan. Two annotations that did NOT work:

1. @return ($player is null ? false : bool)

This seems to express the relationship most clearly, but unfortunately
it did not actually narrow the type of `$player`.

2. @phpstan-assert-if-true !null $player

This had the surprising behavior of also asserting the inverse -- that
if the function returned false, then `$player` was not null, which we
obviously do not want. Presumably because of how "assertions" are used
traditionally (in a simple function with no complex logic), this is a
desirable side effect (phpstan/phpstan#8351).

As alluded to in the linked PHPStan issue, we can achieve the desired
behavior by exploiting "equality assertions", for which the inverse is
not also asserted. With this, we can narrow the type of `$player` to
AbstractPlayer (and thus `!null`) if the function returns true. See
https://psalm.dev/docs/annotating_code/assertion_syntax/#equality-assertions.

Fixes from SectorMap.inc.php:

> Parameter #1 $player of method Smr\Sector::getCachedPort() expects Smr\AbstractPlayer, Smr\Player|null given.

And from SectorsFile.php:

> Parameter #1 $player of method Smr\Sector::getFriendlyForces() expects Smr\AbstractPlayer, Smr\AbstractPlayer|null given.
hemberger added a commit that referenced this pull request Feb 9, 2023
Use the same logic added to SectorMap.inc.php in be2da56.

> Parameter #1 $player of method Smr\Sector::getCachedPort() expects Smr\AbstractPlayer, Smr\AbstractPlayer|null given.
hemberger added a commit that referenced this pull request Feb 9, 2023
This allows the page to display even if there are no valid games for
which NPCs can be created, since we expect the game ID to be an int,
and there is never a real game ID of 0.

Alternatively, we could have skipped a lot of the logic if the game ID
was null, but that is a more involved refactor (which creates other
problems with conditionally defined template variables).

Fixes PHPStan warnings:

> Parameter #1 $selectedGameID of class Smr\Pages\Admin\NpcManageAddAccountProcessor constructor expects int, int|null given.
> Parameter $selectedGameID of class Smr\Pages\Admin\NpcManageProcessor constructor expects int, int|null given.
> Parameter #2 $gameID of static method Smr\AbstractPlayer::getPlayer() expects int, int|null given.
hemberger added a commit that referenced this pull request Feb 9, 2023
These constructor arguments come as a package - either they are all set
or they are all null. It's tricky to demand this relationship when they
are all separately nullable arguments. This suggests that they should
be grouped (ideally in a data class, but we use an array for simplicity
here, which is made much safer with PHPStan array shapes).

Fixes PHPStan warning:

> Parameter #1 $bytes of closure expects int, int|null given.
hemberger added a commit that referenced this pull request Feb 9, 2023
The constructor arguments are only nullable under certain conditions,
due to relationships with other args that are cumbersome to enumerate.
This suggests that the class needs to be refactored, but until then,
using an empty array for some of the arguments fixes the following
PHPStan warnings:

> Parameter #1 $array of method Smr\Database::escapeArray() expects array, array<int>|null given.
> Offset 0 does not exist on array<int>|null.
> Offset 1 does not exist on array<int>|null.

This is kind of a cop out, as it doesn't address the underlying dangers
in the design, but it does placate PHPStan.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants