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

Report/Code: fix fatal potential fatal error when combined with Diff report #955

Merged
merged 1 commit into from
Apr 13, 2025

Conversation

jrfnl
Copy link
Member

@jrfnl jrfnl commented Apr 10, 2025

Description

Okay, so this is an awkward one.

If both the Code + the Diff report were requested + caching was turned on + the order of the requested reports was Code,Diff (i.e. first the code report), the following fatal error would occur:

Fatal error: Uncaught Error: Call to a member function getFixableCount() on null in path/to/PHP_CodeSniffer/src/Fixer.php:144
Stack trace:
#0 path/to/PHP_CodeSniffer/src/Reports/Diff.php(73): PHP_CodeSniffer\Fixer->fixFile()
#1 path/to/PHP_CodeSniffer/src/Reporter.php(285): PHP_CodeSniffer\Reports\Diff->generateFileReport(Array, Object(PHP_CodeSniffer\Files\LocalFile), true, 150)
#2 path/to/PHP_CodeSniffer/src/Runner.php(659): PHP_CodeSniffer\Reporter->cacheFileReport(Object(PHP_CodeSniffer\Files\LocalFile))
#3 path/to/PHP_CodeSniffer/src/Runner.php(400): PHP_CodeSniffer\Runner->processFile(Object(PHP_CodeSniffer\Files\LocalFile))
#4 path/to/PHP_CodeSniffer/src/Runner.php(119): PHP_CodeSniffer\Runner->run()
#5 path/to/PHP_CodeSniffer/bin/phpcs(30): PHP_CodeSniffer\Runner->runPHPCS()
#6 {main}
  thrown in path/to/PHP_CodeSniffer/src/Fixer.php on line 144

To reproduce the issue (on master):

  1. Create a small test file like:
    <?php
    
    echo 'hello'.callMe( $p );
  2. Run the following command to initialize the cache:
    phpcs -ps ./test.php --standard=PSR2 --report=Code,Diff --cache
    All should be fine.
  3. Now run the same command again and see the fatal error.

Some investigating later, it turns out that both the Code report, as well as the Diff report, re-parse the current file, with the Diff report initializing the fixer once the file has reparsed, but the Code report not doing so (as it doesn't need the fixer). The problem with that is that the Code report basically leaves the Fixer in an invalid state, leading to the above error.

While it is up for debate whether reports should ever (be allowed to) re-parse files, for now, let's fix the fatal.

Re-evaluating the report setup should definitely be done, but should probably wait until the Reports classes have test coverage.

Suggested changelog entry

Prevent a potential race condition when both the Diff + the Code reports are requested and caching is on.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

…report

Okay, so this is an awkward one.

If both the `Code` + the `Diff` report were requested + caching was turned on + the _order_ of the requested reports was `Code,Diff` (i.e. first the code report), the following fatal error which would occur:
```
Fatal error: Uncaught Error: Call to a member function getFixableCount() on null in path/to/PHP_CodeSniffer/src/Fixer.php:144
Stack trace:
#0 path/to/PHP_CodeSniffer/src/Reports/Diff.php(73): PHP_CodeSniffer\Fixer->fixFile()
#1 path/to/PHP_CodeSniffer/src/Reporter.php(285): PHP_CodeSniffer\Reports\Diff->generateFileReport(Array, Object(PHP_CodeSniffer\Files\LocalFile), true, 150)
#2 path/to/PHP_CodeSniffer/src/Runner.php(659): PHP_CodeSniffer\Reporter->cacheFileReport(Object(PHP_CodeSniffer\Files\LocalFile))
#3 path/to/PHP_CodeSniffer/src/Runner.php(400): PHP_CodeSniffer\Runner->processFile(Object(PHP_CodeSniffer\Files\LocalFile))
#4 path/to/PHP_CodeSniffer/src/Runner.php(119): PHP_CodeSniffer\Runner->run()
#5 path/to/PHP_CodeSniffer/bin/phpcs(30): PHP_CodeSniffer\Runner->runPHPCS()
#6 {main}
  thrown in path/to/PHP_CodeSniffer/src/Fixer.php on line 144
```

To reproduce the issue (on `master`):
1. Create a small test file like:
    ```php
    <?php

    echo 'hello'.callMe( $p );
    ```
2. Run the following command to initialize the cache:
    ```bash
    phpcs -ps ./test.php --standard=PSR2 --report=Code,Diff --cache
    ```
    All should be fine.
3. Now run the same command again and see the fatal error.

Some investigating later, it turns out that both the `Code` report, as well as the `Diff` report, re-parse the current file, with the `Diff` report initializing the fixer once the file has reparsed, but the `Code` report not doing so (as it doesn't need the fixer).
The problem with that is that the `Code` report basically leaves the `Fixer` in an invalid state, leading to the above error.

While it is up for debate whether reports should ever (be allowed to) re-parse files, for now, let's fix the fatal.

Re-evaluating the report setup should definitely be done, but should probably wait until the Reports classes have test coverage.
@jrfnl jrfnl added this to the 3.12.2 milestone Apr 10, 2025
@jrfnl jrfnl merged commit aa409cb into master Apr 13, 2025
62 of 64 checks passed
@jrfnl jrfnl deleted the feature/reports-fix-fatal-error-code-diff-report branch April 13, 2025 01:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant