Skip to content

Conversation

@fraggerfox
Copy link
Collaborator

@fraggerfox fraggerfox commented Aug 20, 2025

While here also disable testRedirect test, since this is causing PHP fatal error. (@UnnikrishnanBhargavakurup is working on a fix).

This pull request introduces improvements to code quality, type safety, and configuration for static analysis and testing. The most significant changes include stricter type handling in Command.php, enhancements to test and static analysis configurations, and the addition of #[\Override] attributes to clarify interface implementations.

Type safety and code quality improvements:

  • Improved type safety in src/Handler/Command.php by replacing loose casting with explicit type assertions and variable annotations, ensuring safer handling of decoded JSON and command claims. [1] [2] [3] [4] [5]

Testing and static analysis configuration:

  • Updated phpunit.xml to enable stricter checks for deprecations, risky tests, and warnings, and improved code coverage reporting.
  • Added Psalm static analysis tool to CI workflow and suppressed selected unused code warnings in psalm.xml for a cleaner analysis report. (.github/workflows/continuous-integration.yml [1] psalm.xml [2]

Interface implementation clarity:

@fraggerfox fraggerfox self-assigned this Aug 20, 2025
@fraggerfox fraggerfox force-pushed the update-phpunit-configuration branch from 94ed185 to 16f7f7d Compare August 20, 2025 15:39
@fraggerfox fraggerfox force-pushed the update-phpunit-configuration branch 3 times, most recently from b7320ca to 5ea8932 Compare August 20, 2025 21:53
@fraggerfox fraggerfox force-pushed the update-phpunit-configuration branch from 5ea8932 to 7e9d610 Compare August 20, 2025 21:54
@fraggerfox fraggerfox force-pushed the update-phpunit-configuration branch from bb8c4b0 to dd0e77b Compare August 21, 2025 04:58
{
// Mock the header function
$headerMock = $this->getFunctionMock('HelloCoop\HelloResponse', 'header');
// XXX: Comment out for now, since this is causing PHP to die with Fatal error
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Enable this test once it is fixed. cc: @UnnikrishnanBhargavakurup


$curlMock = $this->createMock(CurlWrapper::class);
$curlMock->method('init')->willReturn(true);
$curlMock->method('init')->willReturn($curl);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if this is the correct way to mock init() for the curl wrapper. cc: @UnnikrishnanBhargavakurup

xmlns="https://getpsalm.org/schema/config"
xsi:schemaLocation="https://getpsalm.org/schema/config vendor/vimeo/psalm/config.xsd"
>
<issueHandlers>
Copy link
Collaborator Author

@fraggerfox fraggerfox Aug 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to update psalm with a lot of the Unused* to be suppressed, I am not sure how many of the cases are false positive since this is a library. Running this in your local system by removing the suppression you can see the output it generates:

Here is a list of all the errors reported by psalm.

cc: @UnnikrishnanBhargavakurup

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR focuses on fixing lint issues and updating PHPUnit configuration to meet modern standards and resolve compatibility issues.

  • Updates PHPUnit configuration with stricter error handling and modern structure
  • Fixes deprecation warnings by replacing deprecated PHPUnit methods with current alternatives
  • Adds type annotations and proper type handling for better static analysis compliance

Reviewed Changes

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

Show a summary per file
File Description
tests/Lib/TokenFetcherTest.php Updates curl mocking to return proper CurlHandle instead of boolean
tests/HelloResponse/HelloResponseTest.php Comments out problematic test causing fatal errors
tests/HelloClientTest.php Replaces deprecated isType() with isArray() and isString()
tests/Handler/InviteTest.php Adds proper type annotations for array properties
tests/Handler/CommandTest.php Improves type safety with explicit variable typing
tests/E2E/index.php Removes entire E2E test file
src/Utils/CurlWrapper.php Updates type hints from resource to CurlHandle and removes phpstan ignores
src/Type/AuthUpdates.php Adds Override attributes to ArrayAccess methods
src/Renderers/DefaultPageRenderer.php Adds Override attributes to interface implementations
src/HelloResponse/HelloResponse.php Adds Override attributes to interface implementations
src/HelloRequest/HelloRequest.php Adds Override attributes to interface implementations
src/Handler/Command.php Improves type safety with explicit type checking and variable annotations
src/Config/HelloConfig.php Adds Override attributes to interface implementations
psalm.xml Suppresses various static analysis warnings
phpunit.xml Updates configuration to modern PHPUnit standards
.github/workflows/continuous-integration.yml Adds psalm tool to CI workflow

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

*/
$payload = json_decode($payloadJson, true);
if ($payload === null) {
if (!is_array($payload)) {
Copy link

Copilot AI Aug 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The condition !is_array($payload) is more restrictive than the original $payload === null check. This change may reject valid non-array payloads that were previously accepted, potentially breaking existing functionality.

Copilot uses AI. Check for mistakes.
xsi:schemaLocation="https://getpsalm.org/schema/config vendor/vimeo/psalm/config.xsd"
>
<issueHandlers>
<UnusedMethod errorLevel="suppress" />
Copy link

Copilot AI Aug 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suppressing all unused method warnings globally may hide legitimate issues. Consider addressing specific unused methods or using more targeted suppressions instead of blanket suppression.

Suggested change
<UnusedMethod errorLevel="suppress" />

Copilot uses AI. Check for mistakes.

$commandToken = (string) $this->helloRequest->fetch('command_token') ?? '';
/** @var string $commandToken */
$commandToken = $this->helloRequest->fetch('command_token');
Copy link

Copilot AI Aug 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The type annotation assumes $commandToken is always a string, but fetch() can return mixed types including null. This could cause type errors if the parameter is missing from the request.

Suggested change
$commandToken = $this->helloRequest->fetch('command_token');
$commandToken = $this->helloRequest->fetch('command_token');
if (!is_string($commandToken) || $commandToken === '') {
$this->helloResponse->setStatusCode(400);
$this->helloResponse->json([
'error' => 'invalid_request',
'error_description' => 'command_token must be a non-empty string',
]);
$this->helloResponse->send();
}
/** @var string $commandToken */
$commandToken = $commandToken;

Copilot uses AI. Check for mistakes.
@fraggerfox fraggerfox marked this pull request as ready for review August 21, 2025 05:06
@UnnikrishnanBhargavakurup UnnikrishnanBhargavakurup merged commit 6722768 into master Aug 21, 2025
9 checks passed
@UnnikrishnanBhargavakurup UnnikrishnanBhargavakurup deleted the update-phpunit-configuration branch August 21, 2025 08:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants