Upgrade to PHP 8.2 and modernization by matasarei · Pull Request #27 · matasarei/json-rpc

matasarei · 2026-02-24T21:49:48Z

Update minimum PHP version requirement to 8.2 in composer.json
Replace mt_rand() with \Random\Randomizer in RequestBuilder
Use array_is_list() for batch and positional argument detection
Add explicit string casting to avoid PHP 8.1+ null-to-string deprecations
Fix PHPUnit setMethods() deprecation in tests
Pin doctrine/instantiator to 2.0.0 to maintain PHP 8.2 compatibility

- Update minimum PHP version requirement to 8.2 in composer.json - Replace mt_rand() with \Random\Randomizer in RequestBuilder - Use array_is_list() for batch and positional argument detection - Add explicit string casting to avoid PHP 8.1+ null-to-string deprecations - Fix PHPUnit setMethods() deprecation in tests - Pin doctrine/instantiator to 2.0.0 to maintain PHP 8.2 compatibility

Copilot

Copilot reviewed 9 out of 9 changed files in this pull request and generated 9 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-24T21:58:25Z

src/JsonRPC/Validator/UserValidator.php

 {
    public static function validate(array $users, $username, $password)
    {
+        $username = (string)$username;


Inconsistent string casting: $username is cast to string to avoid PHP 8.1+ null-to-string deprecations, but $password is not cast even though it's also used in a string comparison. For consistency and to avoid potential null comparison issues, $password should also be cast to string.

Suggested change

$username = (string)$username;

$username = (string) $username;

$password = (string) $password;

Copilot · 2026-02-24T21:58:25Z

src/JsonRPC/Validator/HostValidator.php

+        $host = trim((string)$host);
        if (strpos($host, '/') !== false) {
            list($network, $mask) = explode('/', $host);
-            if (self::netMatch($remoteAddress, $network, $mask)) {
+            if (self::netMatch((string)$remoteAddress, (string)$network, (string)$mask)) {


The string casts added for $remoteAddress, $network, and $mask in the netMatch call on line 48 are good defensive programming. However, note that the comparison on line 53 (not shown in diff) uses $remoteAddress without a cast. Consider casting $remoteAddress at the start of ipMatch() to ensure all uses are type-safe throughout the function.

Copilot · 2026-02-24T21:58:25Z

composer.json

    "phpunit/phpunit": "^9.6",
-    "squizlabs/php_codesniffer": "^3.9"
+    "squizlabs/php_codesniffer": "^3.9",
+    "doctrine/instantiator": "^2.0"


The PR description states "Pin doctrine/instantiator to 2.0.0 to maintain PHP 8.2 compatibility", but the composer.json shows "^2.0" which means any version >= 2.0.0 and < 3.0.0, not exactly 2.0.0. The caret operator allows minor and patch updates. If you need to pin to exactly version 2.0.0, use "2.0.0" without the caret.

Copilot · 2026-02-24T21:58:25Z

src/JsonRPC/Request/RequestBuilder.php

            'jsonrpc' => '2.0',
            'method' => $this->procedure,
-            'id' => $this->id ?: mt_rand(),
+            'id' => $this->id ?: (new \Random\Randomizer())->getInt(1, 1000000),


Creating a new \Random\Randomizer instance every time an ID is generated could be inefficient if build() is called frequently. Consider creating a single static instance or caching the Randomizer instance to improve performance. However, this is a minor optimization and the current implementation is functionally correct.

Copilot · 2026-02-24T21:58:26Z

src/JsonRPC/Response/ResponseParser.php

    private function isBatchResponse()
    {
-        return array_keys($this->payload) === range(0, count($this->payload) - 1);
+        return ! empty($this->payload) && array_is_list($this->payload);


The change from array_keys($this->payload) === range(0, count($this->payload) - 1) to ! empty($this->payload) && array_is_list($this->payload) introduces a behavior change: empty arrays are no longer treated as batch responses. In the old code, [] would return true (be treated as a batch), but with the new code it returns false. While this aligns better with JSON-RPC 2.0 spec (which states empty batches are invalid), this is a behavior change that could affect existing code expecting empty arrays to be treated as batches. Consider whether this is intentional and whether it needs documentation or migration notes.

Suggested change

return ! empty($this->payload) && array_is_list($this->payload);

return is_array($this->payload) && array_is_list($this->payload);

Copilot · 2026-02-24T21:58:26Z

src/JsonRPC/Request/BatchRequestParser.php

    public static function isBatchRequest(array $payload)
    {
-        return array_keys($payload) === range(0, count($payload) - 1);
+        return ! empty($payload) && array_is_list($payload);


The change from array_keys($payload) === range(0, count($payload) - 1) to ! empty($payload) && array_is_list($payload) introduces a behavior change: empty arrays are no longer treated as batch requests. In the old code, [] would return true (be treated as a batch), but with the new code it returns false. While this aligns better with JSON-RPC 2.0 spec (which states empty batches are invalid), this is a behavior change that could affect existing code expecting empty arrays to be treated as batches. Consider whether this is intentional and whether it needs documentation or migration notes.

Suggested change

return ! empty($payload) && array_is_list($payload);

return array_is_list($payload);

Copilot · 2026-02-24T21:58:26Z

src/JsonRPC/ProcedureHandler.php

    public function isPositionalArguments(array $request_params)
    {
-        return array_keys($request_params) === range(0, count($request_params) - 1);
+        return ! empty($request_params) && array_is_list($request_params);


The change from array_keys($request_params) === range(0, count($request_params) - 1) to ! empty($request_params) && array_is_list($request_params) introduces a behavior change: empty parameter arrays are no longer treated as positional arguments. In the old code, [] would return true, but with the new code it returns false. This means functions with optional parameters and empty parameter arrays might be handled differently. Consider whether this is intentional and add test coverage for edge cases with empty parameter arrays.

Suggested change

return ! empty($request_params) && array_is_list($request_params);

return $request_params === [] || array_is_list($request_params);

Copilot · 2026-02-24T21:58:26Z

tests/Response/HeaderMockTest.php

        self::$functions = $this
            ->getMockBuilder('stdClass')
-            ->setMethods(['header'])
+            ->addMethods(['header'])


The PHPUnit setMethods() deprecation fix is incomplete. While HeaderMockTest.php was updated to use addMethods(), there are still two other test files that use the deprecated setMethods():

tests/ClientTest.php line 16

tests/HttpClientTest.php line 68

These should also be updated to use addMethods() for consistency and to fully address the PHPUnit deprecation.

Copilot · 2026-02-24T21:58:27Z

src/JsonRPC/Validator/UserValidator.php

        if (! empty($users) && (! isset($users[$username]) || $users[$username] !== $password)) {
            throw new AuthenticationFailureException('Access not allowed');
        }


In UserValidator::validate, the $users array is documented and used as a username/password dictionary and the code compares $users[$username] directly against $password with a plain string comparison, without hashing or a password-safe comparison API. This encourages storing HTTP Basic credentials in plaintext configuration or source code, so if the code/config or backups are leaked an attacker can immediately recover valid credentials. Consider changing this flow to store only password hashes and use a modern password hashing/verification mechanism (for example password_hash/password_verify or an equivalent strong algorithm) when validating logins.

Suggested change

if (! empty($users) && (! isset($users[$username]) || $users[$username] !== $password)) {

throw new AuthenticationFailureException('Access not allowed');

}

// If no users are configured, do not enforce authentication.

if (empty($users)) {

return;

}

// Username must exist in the configured user list.

if (! isset($users[$username])) {

throw new AuthenticationFailureException('Access not allowed');

}

$storedHash = $users[$username];

// Verify provided password against the stored password hash.

if (! is_string($storedHash) || ! password_verify($password, $storedHash)) {

throw new AuthenticationFailureException('Access not allowed');

}

matasarei requested a review from Copilot February 24, 2026 21:49

Copilot started reviewing on behalf of matasarei February 24, 2026 21:50 View session

Copilot AI reviewed Feb 24, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade to PHP 8.2 and modernization#27

Upgrade to PHP 8.2 and modernization#27
matasarei wants to merge 1 commit intomasterfrom
php82-modernization

matasarei commented Feb 24, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Feb 24, 2026

Uh oh!

Copilot AI Feb 24, 2026

Uh oh!

Copilot AI Feb 24, 2026

Uh oh!

Copilot AI Feb 24, 2026

Uh oh!

Copilot AI Feb 24, 2026

Uh oh!

Copilot AI Feb 24, 2026

Uh oh!

Copilot AI Feb 24, 2026

Uh oh!

Copilot AI Feb 24, 2026

Uh oh!

Copilot AI Feb 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	$username = (string)$username;
	$username = (string) $username;
	$password = (string) $password;

	return ! empty($this->payload) && array_is_list($this->payload);
	return is_array($this->payload) && array_is_list($this->payload);

	return ! empty($payload) && array_is_list($payload);
	return array_is_list($payload);

	return ! empty($request_params) && array_is_list($request_params);
	return $request_params === [] \|\| array_is_list($request_params);

-        if (! empty($users) && (! isset($users[$username]) || $users[$username] !== $password)) {
-            throw new AuthenticationFailureException('Access not allowed');
-        }
+        // If no users are configured, do not enforce authentication.
+        if (empty($users)) {
+            return;
+        }
+        // Username must exist in the configured user list.
+        if (! isset($users[$username])) {
+            throw new AuthenticationFailureException('Access not allowed');
+        }
+        $storedHash = $users[$username];
+        // Verify provided password against the stored password hash.
+        if (! is_string($storedHash) || ! password_verify($password, $storedHash)) {
+            throw new AuthenticationFailureException('Access not allowed');
+        }

Conversation

matasarei commented Feb 24, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants