Add PHPStan static analysis at level 0, minimal fixes #42
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Install PHPStan at level 0 - Fix few issues that PHPStan found: - Add missing return statement in Parser::_convertFunction() - Fix regex patterns by properly escaping $ characters - Replace unset() with null assignment for PHP 8.4 compatibility - Fix duplicate array keys in ptg definitions - Add unit tests to verify the fixes - Add dedicated static analysis workflow using PHP 8.2
PHPStan correctly identified that the Parser's $ptg array had duplicate keys, which is invalid in PHP. The Excel PTG specification legitimately has some opcodes with the same name at different positions: - ptgMemNoMem appears at 0x28, 0x48, 0x68 (per Microsoft spec) - ptgAreaErr3d appears at 0x3D, 0x5D, 0x7D (per Microsoft spec) - ptgMemNoMemN had invalid duplicates at 0x2F, 0x4F, 0x6F (not in spec) In PHP arrays, duplicate keys result in the last value overwriting earlier ones. To maintain backward compatibility, we: 1. Keep the LAST occurrence of each duplicate (BC preserved) 2. Comment out earlier duplicates to satisfy PHPStan 3. Remove incorrectly suffixed variants (ptgMemNoMemV/A) that don't exist in spec 4. Add comprehensive tests to ensure BC is maintained This preserves existing behavior while fixing the static analysis errors.
sanmai
commented
Jun 28, 2025
sanmai
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR introduces PHPStan static analysis at level 0, makes minimal code fixes discovered by the analysis, and updates the CI to run the new workflow.
- Add PHPStan configuration and GitHub Actions workflow for static analysis
- Fix missing return in
Parser::_convertFunction, adjustunsettonullinWorksheet::getData, and update duplicate PTG definitions - Enhance tests to cover the new behaviors and update
composer.jsondev dependencies
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/Test/Spreadsheet/Excel/Writer/WorksheetTest.php | Add assertion that _data is now null after getData() |
| test/Test/Spreadsheet/Excel/Writer/ParserTest.php | Extend _convertFunction tests with invalid-args exception path |
| phpstan.neon | Introduce PHPStan at level 0 and ignore one known error |
| composer.json | Include phpstan/phpstan and php-parallel-lint; sort packages |
| Spreadsheet/Excel/Writer/Worksheet.php | Replace unset($this->_data) with assignment to null |
| Spreadsheet/Excel/Writer/Parser.php | Comment out duplicate PTG keys and add exception for bad args |
| .github/workflows/static-analysis.yaml | New workflow to run PHPStan analysis on PHP 8.2 |
| .github/workflows/ci.yaml | Remove phpstan/phpstan from default CI dev dependencies |
Comments suppressed due to low confidence (1)
Spreadsheet/Excel/Writer/Parser.php:675
- Add an
@throws \InvalidArgumentExceptiontag to the_convertFunctiondocblock to document the new exception path.
throw new \InvalidArgumentException("Invalid argument count $args for function $token");
| $this->assertTrue(is_string($result)); | ||
|
|
||
| // Array structure: [function_number, arg_count, unknown, volatile_flag] | ||
| $parser->_functions['INVALID'] = array(999, -2, 0, 0); // -2 is not valid |
There was a problem hiding this comment.
Directly assigning to _functions may fail if the property is protected. Use ReflectionProperty to make _functions accessible or provide a setter method in Parser for testing.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Reference: https://learn.microsoft.com/en-us/openspecs/office_file_formats/ms-xls/9310c3bb-d73f-4db0-8342-28e1e0fcb68f