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

Enable PHP 8.4 compat and update #13

Merged
merged 21 commits into from
Nov 20, 2024
Merged

Enable PHP 8.4 compat and update #13

merged 21 commits into from
Nov 20, 2024

Conversation

koriym
Copy link
Member

@koriym koriym commented Nov 19, 2024

No description provided.

Extend the range of supported PHP versions by including 8.3 in the old stable list. Update the current stable PHP version to 8.4 for the latest compatibility and improvements.
Define a new @psalm-type for Match to improve type annotations. This enhances code clarity and ensures more precise type checking in the Streamer class.
Streamline tests and update dependencies

Removed PHP version checks and path exclusions to simplify test bootstrapping. Updated method return types for consistency and added a type check in test assertions. Upgraded various dependencies and their configurations, ensuring compatibility with newer PHP versions and coding standards.
Added a placeholder bin command to indicate no binary is installed. Updated paths for PHP tools (phpcs, phpcbf, phpstan, etc.) to use vendor-bin/tools directory for improved dependency management and organization.
Updated coding standards, continuous integration, and static analysis workflows to version 3. This helps ensure compatibility with PHP 8.3 and new stable versions.
This commit removes the unnecessary check for is_string before processing the view. Instead, the view is directly cast to a string, ensuring type safety and simplifying the code. This change is also annotated to ignore specific PHPStan warnings.
Copy link

coderabbitai bot commented Nov 19, 2024

Important

Review skipped

Auto reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

📝 Walkthrough
📝 Walkthrough
📝 Walkthrough

Walkthrough

The pull request introduces several updates across various configuration files and source code. The continuous integration workflow is adjusted to support new PHP versions, while the composer.json file sees changes in script execution and the addition of a new configuration section. The PHP CodeSniffer configuration is updated for PHP version compatibility and exclusion patterns. Additionally, type safety is enhanced in several classes through return type declarations, and minor documentation style changes are made throughout. Overall, the modifications focus on improving version support, type safety, and code clarity.

Changes

File Change Summary
.github/workflows/continuous-integration.yml Updated old_stable to include PHP 8.3 and changed current_stable from 8.3 to 8.4.
composer.json Removed post-install-cmd and post-update-cmd, added bin script, updated script paths, and added extra section.
phpcs.xml Updated schema location to local path, changed PHP version compatibility to 8.3, modified exclusion patterns and rules.
phpstan.neon Removed excludePaths for tests/Fake, allowing its inclusion in analysis.
psalm.xml Added attributes findUnusedBaselineEntry and findUnusedCode, both set to false.
src/Annotation/Stream.php Separated Attribute and Qualifier declarations into distinct lines and removed unused imports.
src/StreamModule.php Updated PHPDoc tag from {@inheritdoc} to {@inheritDoc} for configure method.
src/StreamProvider.php Changed documentation from {@inheritdoc} to {@inheritDoc} for get method.
src/StreamRenderer.php Updated documentation from {@inheritdoc} to {@inheritDoc} for constructor and methods.
src/StreamResponder.php Changed documentation from {@inheritdoc} to {@inheritDoc} for __invoke method and added coverage ignore comment.
src/StreamTransferInject.php Changed return type of setRenderer from static to ResourceObject and updated documentation in transfer.
src/Streamer.php Added type alias Match and updated method parameter type in collect.
tests/Fake/Resource/Page/StreamArray.php Updated onGet method to include a return type declaration of static.
tests/Fake/Resource/Page/StreamString.php Updated onGet method to include a return type declaration of static.
tests/Fake/Resource/Page/TextArray.php Updated onGet method to include a return type declaration of static.
tests/Fake/Resource/Page/TextString.php Updated onGet method to include a return type declaration of static.
tests/IntegrateTest.php Modified $view handling in testRender method for type casting.
tests/bootstrap.php Removed conditional check for PHP version before setting AttributeReader.
tests/header.php Changed parameter type for $http_response_code from ?string to `string
vendor-bin/tools/composer.json Moved several dependencies from require to require-dev and updated their versions.

Possibly related PRs


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@koriym
Copy link
Member Author

koriym commented Nov 19, 2024

@coderabbitai review

Copy link

coderabbitai bot commented Nov 19, 2024

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link

codecov bot commented Nov 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (76978ee) to head (4b4f57d).
Report is 40 commits behind head on 1.x.

Additional details and impacted files
@@             Coverage Diff             @@
##                 1.x       #13   +/-   ##
===========================================
  Coverage     100.00%   100.00%           
+ Complexity        32        30    -2     
===========================================
  Files              6         6           
  Lines             82        79    -3     
===========================================
- Hits              82        79    -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

Switched various packages from the primary list to the dev list, and removed several other packages from the lock file. These changes optimize dependency management and clarify package usage in development environments.
Removed the "maglnet/composer-require-checker" and "webmozart/glob" packages from composer.lock and composer.json. Also, updated the "allow-plugins" configuration to disable "dealerdirect/phpcodesniffer-composer-installer". These changes streamline dependencies and improve plugin security settings.
annotations and upgrade dependencies

Replaced {@inheritdoc} annotations with {@inheritdoc} for consistency and upgraded doctrine/coding-standard from v11 to v12. This change ensures that the code adheres to updated coding practices and maintains consistency across the project.
Changing "forward-command" to false in composer.json prevents commands from being forwarded to other scripts. This ensures better command isolation and avoids unexpected behavior in dependency scripts.
Adjusted the header function's type hinting for consistency. Modified the PHPCS configuration for PHP 8 compatibility and streamlined the exclusion patterns. Corrected minor formatting issues and removed unnecessary code from the Annotation/Stream file.
The "post-install-cmd" and "post-update-cmd" scripts were removed to simplify the composer configuration. Additionally, the "forward-command" option in the "bamarni-bin" section was changed to true for better command forwarding.
Replaced separate old_stable and current_stable parameters with a single php_versions parameter to streamline version management. This change enhances maintainability by reducing redundancy and potential for errors in specifying PHP versions.
@koriym koriym force-pushed the php84 branch 2 times, most recently from cdffcc9 to 0fb4b29 Compare November 19, 2024 23:27
Added `findUnusedBaselineEntry` and `findUnusedCode` attributes to the `psalm.xml` configuration, setting both to `false`. This change prevents Psalm from flagging unused code or baseline entries, focusing analysis only on currently utilized elements.
This change updates the continuous integration workflow to specify 'old_stable' PHP versions (8.1, 8.2, 8.3) and 'current_stable' PHP version (8.4). Additionally, it changes the workflow configuration to use the older version (v1) of the template.
Changed the "bin-links" setting from false to true under the "bamarni-bin" configuration in composer.json. This will ensure that bin links are created for the project's dependencies.
Changed the PHP compatibility version from 8.0.0 to 8.3.0. This ensures that the codebase adheres to the standards and features of PHP 8.3.0, potentially catching issues or incompatibilities with this version.
Previously, the `dealerdirect/phpcodesniffer-composer-installer` plugin was not allowed in the vendor-bin/tools/composer.json configuration. This change enables the plugin, facilitating automated code style checks.
Reverted workflow versions for both coding-standards and static-analysis jobs from v3 to v1. This change ensures compatibility with existing configurations and avoids potential issues with newer versions.
Added a code coverage ignore annotation for `http_response_code` in `StreamResponder.php` to avoid coverage tool issues for non-CLI environments. Also included an unrelated whitespace change in `composer.json`.
@koriym
Copy link
Member Author

koriym commented Nov 20, 2024

@coderabbitai review

Copy link

coderabbitai bot commented Nov 20, 2024

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

🧹 Outside diff range and nitpick comments (9)
tests/Fake/Resource/Page/TextArray.php (1)

9-9: LGTM! Consider adding PHPDoc for better documentation.

The addition of the static return type is correct and aligns well with PHP 8.4 compatibility goals. The implementation properly returns $this, matching the declared return type.

Consider adding a PHPDoc block to document the method's purpose and return value:

+    /**
+     * Get text array response
+     *
+     * @return static ResourceObject with greeting message
+     */
     public function onGet(): static
tests/Fake/Resource/Page/StreamString.php (1)

Line range hint 8-16: Consider implementing resource cleanup.

Since this class deals with file handles, it should properly manage resource cleanup to prevent memory leaks in the test environment.

 class StreamString extends ResourceObject
 {
+    private $fileHandle;
+
     /**
      * Ignore renderer, just stream $this->body
+     *
+     * @throws \RuntimeException When the message file cannot be opened
+     * @return static
      */
     public function onGet(): static
     {
-        $this->body = fopen(__DIR__ . '/message.txt', 'r');
+        $this->fileHandle = fopen(__DIR__ . '/message.txt', 'r');
+        if ($this->fileHandle === false) {
+            throw new \RuntimeException('Failed to open message.txt');
+        }
+        $this->body = $this->fileHandle;
 
         return $this;
     }
+
+    public function __destruct()
+    {
+        if (is_resource($this->fileHandle)) {
+            fclose($this->fileHandle);
+        }
+    }
 }
tests/header.php (1)

9-9: LGTM! Good modernization of type declaration syntax.

The change from ?string to string|null for the $http_response_code parameter aligns well with modern PHP type system practices. This union type syntax is more explicit and consistent with PHP 8+ conventions while maintaining the same nullability semantics.

Consider applying similar union type modernization across the codebase for consistency, as this style is preferred in modern PHP development.

tests/Fake/Resource/Page/StreamArray.php (1)

Line range hint 12-19: Consider improving file stream handling for tests

The current implementation has a few potential issues:

  1. No error handling for fopen which could fail
  2. No stream cleanup mechanism
  3. Hardcoded file path might be problematic in different test environments

Consider applying these improvements:

     public function onGet(): static
     {
+        $filePath = __DIR__ . '/message.txt';
+        $stream = @fopen($filePath, 'r');
+        if ($stream === false) {
+            throw new \RuntimeException(sprintf('Failed to open test file: %s', $filePath));
+        }
         $this->body = [
             'msg' =>'hello world',
-            'stream' => fopen(__DIR__ . '/message.txt', 'r')
+            'stream' => $stream
         ];
 
         return $this;
     }
+
+    protected function __destruct()
+    {
+        if (isset($this->body['stream']) && is_resource($this->body['stream'])) {
+            fclose($this->body['stream']);
+        }
+    }
vendor-bin/tools/composer.json (1)

16-16: Ensure proper JSON formatting

The file appears to have extra whitespace at the end. Consider removing the extra newlines while maintaining a single newline at EOF.

}
-    
-
+
src/Streamer.php (1)

51-51: Consider adding PHP 8.4 resource return type

While the Match type assertion improves type safety, consider enhancing it further with PHP 8.4's resource return type declaration:

-    public function getStream(string $string)
+    /** @return resource */
+    public function getStream(string $string): mixed
composer.json (3)

40-40: Improve clarity of the bin script message

The message "no bin installed." is unclear and potentially confusing. Consider making it more descriptive to explain why there are no binaries or what users should expect.

-          "bin": "echo 'no bin' installed.",
+          "bin": "echo 'No binary files are provided with this package.'",

41-50: Standardize vendor binary paths

While most scripts have been updated to use vendor-bin/tools/vendor/bin/, the test script still uses ./vendor/bin/. Consider updating it for consistency.

-          "test": ["./vendor/bin/phpunit"],
+          "test": ["./vendor-bin/tools/vendor/bin/phpunit"],

PHP version constraint needs adjustment for dependency compatibility

Based on the dependency analysis:

  • PHPUnit requires PHP >=8.3, which is more restrictive than the current ^8.1
  • Other dependencies have varying constraints but are compatible with PHP 8.x

Recommendations:

  • Update PHP version constraint to "php": "^8.3" to match PHPUnit's requirement
  • This will ensure compatibility with PHP 8.4 while maintaining alignment with dependencies
🔗 Analysis chain

Line range hint 26-26: Verify PHP version constraint for 8.4 compatibility

The current PHP version constraint ^8.1 will allow PHP 8.4 when it's released. However, consider if you want to:

  1. Explicitly document 8.4 support by updating to ^8.1 || ^8.4
  2. Test against PHP 8.4 alpha/beta releases
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if other dependencies have PHP 8.4 compatibility issues

# Get all direct dependencies
jq -r '.require + ."require-dev" | to_entries[] | select(.key != "php") | .key' composer.json | while read pkg; do
    echo "Checking $pkg..."
    curl -s "https://packagist.org/packages/$pkg.json" | jq -r '.package.versions | to_entries[0].value.require.php // "No PHP constraint"'
done

Length of output: 1468

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 9f1c1a7 and 6a9cbbe.

⛔ Files ignored due to path filters (1)
  • vendor-bin/tools/composer.lock is excluded by !**/*.lock
📒 Files selected for processing (20)
  • .github/workflows/continuous-integration.yml (1 hunks)
  • composer.json (1 hunks)
  • phpcs.xml (1 hunks)
  • phpstan.neon (0 hunks)
  • psalm.xml (1 hunks)
  • src/Annotation/Stream.php (1 hunks)
  • src/StreamModule.php (1 hunks)
  • src/StreamProvider.php (1 hunks)
  • src/StreamRenderer.php (2 hunks)
  • src/StreamResponder.php (2 hunks)
  • src/StreamTransferInject.php (3 hunks)
  • src/Streamer.php (3 hunks)
  • tests/Fake/Resource/Page/StreamArray.php (1 hunks)
  • tests/Fake/Resource/Page/StreamString.php (1 hunks)
  • tests/Fake/Resource/Page/TextArray.php (1 hunks)
  • tests/Fake/Resource/Page/TextString.php (1 hunks)
  • tests/IntegrateTest.php (1 hunks)
  • tests/bootstrap.php (1 hunks)
  • tests/header.php (1 hunks)
  • vendor-bin/tools/composer.json (1 hunks)
💤 Files with no reviewable changes (1)
  • phpstan.neon
✅ Files skipped from review due to trivial changes (6)
  • psalm.xml
  • src/Annotation/Stream.php
  • src/StreamModule.php
  • src/StreamProvider.php
  • src/StreamRenderer.php
  • src/StreamResponder.php
🧰 Additional context used
🪛 phpstan
tests/IntegrateTest.php

81-81: No error to ignore is reported on line 81.

(ignore.unmatchedLine)

🔇 Additional comments (18)
tests/bootstrap.php (1)

10-10: Verify AttributeReader compatibility with PHP 8.4

The unconditional setting of AttributeReader needs verification for PHP 8.4 compatibility. Also, the removal of the PHP version check might affect compatibility with older PHP versions if they're still supported.

Let's verify the compatibility requirements:

Consider adding a comment explaining why the version check was removed and confirming PHP 8.4 compatibility.

+// PHP 8.4 compatibility: AttributeReader is now used unconditionally as it's compatible with all supported PHP versions
 ServiceLocator::setReader(new AttributeReader());
✅ Verification successful

Removal of PHP version check is appropriate

The changes are verified to be correct based on the following evidence:

  • The project requires PHP ^8.1 as per composer.json
  • Git history shows active support for PHP 8.1 through 8.4
  • AttributeReader is only used in the test bootstrap file
  • The codebase has been updated to support PHP 8.4 as seen in the version configurations

Since the minimum PHP version is 8.1, and attributes are natively supported since PHP 8.0, the version check is no longer necessary. The unconditional setting of AttributeReader is safe and appropriate.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check composer.json for PHP version constraints and AttributeReader dependency
echo "Checking PHP version constraints and AttributeReader dependency..."
cat composer.json | jq -r '.require["php"], .require["koriym/attributes"]'

# Look for any version-specific code or comments related to AttributeReader
echo "Checking for version-specific AttributeReader usage..."
rg -l "AttributeReader" --type php

# Check if there are any PHP version checks elsewhere in the codebase
echo "Checking for PHP version checks..."
rg -l "PHP_VERSION|php_version" --type php

Length of output: 527


Script:

#!/bin/bash
# Let's check the actual implementation of ServiceLocator and AttributeReader
echo "Checking ServiceLocator implementation..."
rg -A 5 "class ServiceLocator" --type php

echo "Checking AttributeReader implementation..."
rg -A 5 "class AttributeReader" --type php

# Check composer.json more thoroughly for any attributes-related dependencies
echo "Checking composer.json for all dependencies..."
cat composer.json

# Check for any PHP version related changes in git history
echo "Checking recent commits for PHP version changes..."
git log -p | rg "PHP_VERSION|php_version|^8\."

Length of output: 3668

tests/Fake/Resource/Page/TextString.php (1)

9-9: LGTM! Good addition of return type declaration.

The addition of the static return type declaration improves type safety and code clarity while maintaining PHP 8.4 compatibility. This change aligns well with PHP's movement towards stricter typing and makes the method's contract more explicit.

.github/workflows/continuous-integration.yml (2)

12-12: LGTM: Moving PHP 8.3 to old_stable is appropriate.

The addition of PHP 8.3 to the old_stable versions is correct as it's now a stable release.


13-13: Verify PHP 8.4 compatibility and CI support.

PHP 8.4 is currently in development/alpha stage. Please ensure:

  1. The CI workflow supports testing against pre-release PHP versions
  2. The codebase is compatible with PHP 8.4's breaking changes

Consider:

  1. Adding a comment in the workflow file indicating PHP 8.4 is in development
  2. Documenting any PHP 8.4 specific changes or requirements
tests/Fake/Resource/Page/StreamString.php (1)

12-12: LGTM! Return type declaration is appropriate.

The addition of the : static return type declaration improves type safety and correctly reflects that the method returns $this. This change aligns well with PHP 8.4 compatibility goals.

tests/Fake/Resource/Page/StreamArray.php (1)

12-12: LGTM! The return type addition improves type safety.

The addition of the : static return type declaration correctly specifies that the method returns an instance of the current class.

vendor-bin/tools/composer.json (1)

9-15: LGTM! Plugin configuration is correct

The allow-plugins configuration for dealerdirect/phpcodesniffer-composer-installer is necessary and correctly set.

src/StreamTransferInject.php (2)

34-34: LGTM! Documentation style update

The change from {@inheritdoc} to {@inheritDoc} follows a consistent documentation style across the codebase.


Line range hint 18-25: Verify the return type change compatibility

The return type annotation has been changed from static to ResourceObject. While this provides more specific type information, we need to verify:

  1. If this change is backward compatible with existing code
  2. If the parent class's setRenderer method returns ResourceObject
✅ Verification successful

Return type change is correct and necessary

The change from static to ResourceObject return type is correct because:

  1. The trait StreamTransferInject is used in classes that extend ResourceObject
  2. The trait's setRenderer method is designed to work with ResourceObject as evidenced by the imports and type declarations
  3. The change provides more precise type information while maintaining compatibility since all classes using this trait are ResourceObject instances

The change aligns with PHP 8.4 compatibility goals by using more specific return types, which is a recommended practice for modern PHP code.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check parent class implementation and usages of setRenderer

# Find the parent class implementation
ast-grep --pattern 'class $_ {
  $$$
  public function setRenderer($_) {
    $$$
  }
  $$$
}'

# Find all places where setRenderer is called to verify compatibility
rg -l "setRenderer" | xargs rg -A 2 "->setRenderer\("

Length of output: 554


Script:

#!/bin/bash
# Let's try a different approach to find the parent class and usages

# Find files containing setRenderer method
echo "=== Files containing setRenderer ==="
rg -l "setRenderer"

# Find the method implementation and its return type
echo -e "\n=== Method implementations ==="
rg -B2 -A2 "function setRenderer"

# Find class/trait declarations containing setRenderer
echo -e "\n=== Class/trait declarations ==="
ast-grep --pattern 'trait $_ {
  $$$
  function setRenderer($_) {
    $$$
  }
  $$$
}'

ast-grep --pattern 'class $_ {
  $$$
  function setRenderer($_) {
    $$$
  }
  $$$
}'

# Find method calls
echo -e "\n=== Method calls ==="
rg "setRenderer\("

Length of output: 1113


Script:

#!/bin/bash
# Let's find the parent class and its implementation

# Find the trait's parent class reference
echo "=== Parent class reference ==="
rg -B5 "class|trait.*StreamTransferInject"

# Look for the base class implementation
echo -e "\n=== Base class implementation ==="
fd -e php | xargs rg -l "class.*Transfer"

# If found, check its content
echo -e "\n=== Transfer class content ==="
fd -e php | xargs rg -B2 -A2 "class.*Transfer.*\{"

Length of output: 12142


Script:

#!/bin/bash
# Let's check the TransferInterface and ResourceObject hierarchy

# Check TransferInterface
echo "=== TransferInterface definition ==="
rg -B2 -A2 "interface.*TransferInterface"

# Check ResourceObject class
echo -e "\n=== ResourceObject class ==="
rg -B2 -A2 "class.*ResourceObject"

# Check if StreamTransferInject uses any parent trait/class
echo -e "\n=== StreamTransferInject trait definition ==="
rg -B5 -A5 "trait StreamTransferInject"

Length of output: 2202

src/Streamer.php (3)

21-21: Improved type safety with accurate type alias

The new Match type alias better represents the nested array structure returned by preg_match_all with PREG_SET_ORDER. This change enhances type safety and code maintainability.


68-71: LGTM: Accurate parameter type annotation

The updated parameter type using Match correctly reflects the nested array structure while maintaining the accurate return type annotation.


Line range hint 1-81: Verify PHP 8.4 compatibility

Let's check for any PHP 8.4 deprecation notices or potential compatibility issues in the codebase.

✅ Verification successful

Let me gather more information about the resource handling and PHP version compatibility.


Resource handling is compatible with PHP 8.4

The codebase's resource handling practices are compatible with PHP 8.4:

  • Uses proper resource type hints in docblocks
  • Correctly validates resources using is_resource() and get_resource_type()
  • Stream operations follow PHP's standard practices
  • No deprecated resource handling functions are used

While the package currently requires PHP ^8.1, no incompatibilities with PHP 8.4 were found in the implementation. The resource handling for streams follows best practices that remain valid in PHP 8.4.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for potentially incompatible PHP features or deprecated functionality

# Check for PHP 8.4 compatibility tests
rg -l "8.4" --type php

# Look for potentially affected resource handling
ast-grep --pattern 'resource'

# Check composer.json for PHP version constraints
cat composer.json | jq '.require.php'

Length of output: 97


Script:

#!/bin/bash
# Check for resource type hints and handling
rg "resource" -A 2 -B 2 --type php

# Check composer requirements and platform info
cat composer.json | jq '.require, .config.platform'

# Look for PHPUnit tests to understand version compatibility coverage
fd -e php -e dist test/ tests/

Length of output: 6256

composer.json (1)

58-62: LGTM! Good practice for tool isolation

The addition of the bamarni-bin configuration with enabled bin-links and command forwarding is a good practice. This setup:

  • Isolates development tools from project dependencies
  • Helps prevent conflicts between different PHP version requirements
  • Supports the goal of PHP 8.4 compatibility
tests/IntegrateTest.php (1)

80-81: Verify test coverage for all response types

The changes in type handling might affect how different response types are processed. Let's ensure all test cases are still covered.

✅ Verification successful

Let me gather more information about the response types and test coverage.


Let me check one more aspect to understand the test data provider and the types of responses being tested.


Test coverage is comprehensive for all response types

The test coverage is complete and properly handles all response types:

  • String responses (TextString class)
  • Array responses (TextArray class)
  • Stream responses (StreamString class)
  • Mixed array with stream (StreamArray class)

The type cast to string in the test $this->streamer->getStream((string) $view) is safe because:

  1. All resource classes return static type
  2. The StreamerInterface correctly handles both string input and resource streams
  3. The test data provider covers all response variations
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check response type handling in related resource classes

# Look for onGet method implementations and their return types
ast-grep --pattern 'function onGet()' -l php

# Check for any type-related assertions or type hints in test files
rg -t php 'assert.*instanceof|@return' tests/

Length of output: 158


Script:

#!/bin/bash
# Check the implementation of onGet() methods and their context
ast-grep --pattern 'function onGet() {
  $$$
}'

# Look for the streamer implementation and type handling
rg -t php "class.*Streamer" -A 5
rg -t php "getStream.*function" -A 5

# Check test data providers and assertions
rg -t php "dataProvider" -A 10 tests/
rg -t php "assert" tests/

Length of output: 1786


Script:

#!/bin/bash
# Check the test data provider implementation
ast-grep --pattern 'function caseProvider()' -A 10

# Look for the actual resource class implementation
rg -t php "class.*Resource" -A 10

# Check the StreamerInterface definition
rg -t php "interface.*StreamerInterface" -A 10

Length of output: 3209

🧰 Tools
🪛 phpstan

81-81: No error to ignore is reported on line 81.

(ignore.unmatchedLine)

phpcs.xml (4)

3-4: LGTM! Good improvement in schema location.

Moving the schema location to the local vendor path is a good practice. It ensures consistent validation against the installed PHP_CodeSniffer version and removes external dependencies.


26-40: Review impact of relaxed coding standards.

The changes to rule exclusions could affect code quality and type safety:

  1. Allowing non-explicit assertions (RequireExplicitAssertion)
  2. Permitting non-camelCase variables (ValidVariableName)
  3. Removing property type hint requirements

While these changes might be necessary for compatibility, please ensure they don't compromise code quality.

Consider adding PHPStan or Psalm rules to compensate for the relaxed PHPCS rules, especially for type safety.

✅ Verification successful

Relaxed coding standards have minimal impact on codebase quality

The verification shows:

  • Only test-related code uses snake_case variables, specifically in tests/header.php which is acceptable for test fixtures
  • No properties without type hints were found in the codebase

The relaxed rules appear to be carefully chosen exceptions that don't compromise code quality, while accommodating specific testing needs.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for potential issues that might be missed due to relaxed rules

# Check for non-camelCase variables
echo "Files with potential non-camelCase variables:"
rg -t php '(\$[a-z]+_[a-z]+)' 

# Check for properties without type hints
echo "\nProperties without type hints:"
ast-grep --pattern 'class $_ {
  $$$
  private $_
  $$$
}'

Length of output: 453


20-21: Verify the impact of broader exclusion patterns.

The exclusion patterns have been simplified, but this could potentially exclude more files than intended. Please verify that these patterns don't accidentally exclude important files that should be checked.

✅ Verification successful

Let me gather more context about the old patterns and the purpose of these test files.


The simplified exclusion patterns are safe to use

The new patterns (*/tmp/* and */Fake/*) are actually more restrictive than before. The git history shows that previously there were multiple overlapping patterns including */tests/Fake/*, */Fake/*, */tests/tmp/*, and */tmp/*. The affected files are all test fixtures under tests/Fake/ directory, which are meant to be excluded from code style checks.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Compare files that would be excluded by old vs new patterns

echo "Files matching new exclusion patterns:"
fd -t f . -E "vendor" | rg -e ".*/tmp/.*" -e ".*/Fake/.*"

echo "\nPHP files in these directories that might need checking:"
fd -e php . -E "vendor" | rg -e ".*/tmp/.*" -e ".*/Fake/.*"

Length of output: 692


Script:

#!/bin/bash
# Check the old version of phpcs.xml and test file contents
echo "Previous phpcs.xml patterns:"
git log -p phpcs.xml | rg "exclude-pattern"

echo "\nChecking test file purpose:"
head -n 10 tests/Fake/Resource/Page/*.php

Length of output: 2012


11-12: Verify PHP version configuration for 8.4 compatibility.

While the PR aims for PHP 8.4 compatibility, the configuration is set to PHP 8.3.0. Please verify if this is intentional or if it should be updated to 8.4.0 (80400).

✅ Verification successful

PHP 8.3.0 configuration is appropriate

The current PHP version configuration (8.3.0) in phpcs.xml is appropriate and doesn't need to be updated to 8.4.0 because:

  1. The codebase's composer.json requires PHP "^8.1", indicating production compatibility from PHP 8.1 onwards
  2. No PHP 8.4-specific features are used in the codebase (only attributes and basic type annotations which are supported in PHP 8.3)
  3. PHP 8.3.0 is the latest stable version that PHP_CodeSniffer can reliably check against, as PHP 8.4 is still in development
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if any PHP 8.4 specific features are used in the codebase
# that might not be covered by the current 8.3.0 setting

# Search for PHP 8.4 features in PHP files
rg -t php "(?i)(readonly\s+class|#\[\S+\]|enum\s+\w+|match\s*\()" 

# Check composer.json for PHP version constraint
if [ -f "composer.json" ]; then
    echo "PHP version constraint in composer.json:"
    jq .require.php composer.json
fi

Length of output: 531


Script:

#!/bin/bash
# Let's check the PHP version requirements in other configuration files
# and look for any PHP 8.4 specific syntax or features

# Check for any other PHP version references in XML files
echo "=== Checking XML files for PHP version references ==="
rg -t xml "php_version|phpVersion|PHP" -A 2 -B 2

# Check for any PHP 8.4 specific features that might be used
# (focusing on features introduced in 8.4)
echo -e "\n=== Checking for potential PHP 8.4 specific features ==="
rg -t php "(class\s+.*\{.*readonly\s+|readonly\s+class|type\s+\w+\s*=|class_alias\s*\()"

# Look for any upgrade/migration related files
echo -e "\n=== Checking for upgrade/migration related files ==="
fd -t f "(upgrade|migration|CHANGELOG|README)" -X cat {}

Length of output: 2264

tests/Fake/Resource/Page/StreamString.php Show resolved Hide resolved
vendor-bin/tools/composer.json Show resolved Hide resolved
tests/IntegrateTest.php Outdated Show resolved Hide resolved
Add an assertion to verify that $view implements the Stringable interface before casting it to a string. This enhances type safety and prevents potential runtime errors.
@koriym koriym merged commit 44205a7 into 1.x Nov 20, 2024
30 checks passed
@koriym koriym deleted the php84 branch November 20, 2024 07:11
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.

1 participant