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

Rollback stubs-rector tweak and remove e2e/define-constant #2377

Merged

Conversation

samsonasik
Copy link
Member

@TomasVotruba it seems the error is only happen on Github action on php 7.2. This rollback the tweak and remove the e2e/define-constant, The e2e/define-constant was exists to verify RichParser which it working ok now, so I think it is safe to be removed.

@samsonasik
Copy link
Member Author

All checks have passed 🎉 @TomasVotruba I think it is ready.

@TomasVotruba
Copy link
Member

Ok, I trust your judgement 👍

@TomasVotruba TomasVotruba merged commit 38ed8d2 into main May 27, 2022
@TomasVotruba TomasVotruba deleted the roll-back-stubs-rector-tweak-remove-define-constant branch May 27, 2022 15:48
@samsonasik
Copy link
Member Author

@TomasVotruba I just re-tried at my module and it seems the error happen in PHP 7.4 too

https://github.com/samsonasik/ci4-album/runs/6628279352?check_suite_focus=true#step:10:36

I will rollback the e2e test and try if it can be handled.

@TomasVotruba
Copy link
Member

TomasVotruba commented May 27, 2022

Keep on searching 👍

I wonder how we managed this before, I recall we've fixed this already with loading own stubs. There was also some issue reported by @Wirone

@samsonasik
Copy link
Member Author

probably github issue, but it is interesting that it only happen in github action:

composer update
Loading composer repositories with package information
Info from https://repo.packagist.org: #StandWithUkraine
Updating dependencies
Lock file operations: 0 installs, 1 update, 0 removals
  - Upgrading rector/rector (0.13.0 => dev-main 4838a73)
Writing lock file
Installing dependencies from lock file (including require-dev)
Package operations: 0 installs, 1 update, 0 removals
  - Upgrading rector/rector (0.13.0 => dev-main 4838a73): Extracting archive
Generating autoload files
51 packages you are using are looking for funding.
Use the `composer fund` command to find out more!

➜  ci4-album git:(master) ✗ /opt/homebrew/Cellar/php@7.4/7.4.29/bin/php /usr/local/bin/composer rectify
> rector --dry-run
 46/46 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%
2 files with changes
====================

1) test/Database/Infrastructure/Persistence/Track/SQLTrackRepositoryTest.php:99

    ---------- begin diff ----------
@@ @@

     /**
      * @dataProvider invalidData
+     * @param mixed[]|null $data
      */
     public function testSaveInvalidData(?array $data): void
     {
    ----------- end diff -----------

Applied rules:
 * AddArrayParamDocTypeRector


2) test/Database/Infrastructure/Persistence/Album/SQLAlbumRepositoryTest.php:88

    ---------- begin diff ----------
@@ @@

     /**
      * @dataProvider invalidData
+     * @param mixed[]|null $data
      */
     public function testSaveInvalidData(?array $data): void
     {
    ----------- end diff -----------

Applied rules:
 * AddArrayParamDocTypeRector


                                                                                                                        
 [OK] 2 files would have changed (dry-run) by Rector                                                                    
                                                                                                                        

Script rector --dry-run handling the rectify event returned with error code 1

placing back to bootstrap.php is incorrect as it change the autoload which should not, as should only happen during rector usage.

@TomasVotruba
Copy link
Member

It happens also outside Rector. In one legacy project on PHP 7.2, I've upgraded Rector to 0.13 and PHPStan 1.7.1.
The class is in PHPStan, but it was never loaded while running Rector and Rector always crashed.

In the end only using local stub helped in composer.json:

{
    "autoload-dev": {
        "classmap": ["stubs"]
    }
}

The stubs/ReflectionUnionType.php here:

<?php
declare(strict_types=1);

// for some reason, PHPStan 1.7.1 does not load its internal stubs anymore https://github.com/phpstan/phpstan-src/blob/1.7.x/stubs/runtime/ReflectionUnionType.php
// so we have to do it here again with this file and composer.json > classmap

if (\PHP_VERSION_ID < 80000) {
    if (class_exists('ReflectionUnionType', false)) {
        return;
    }

    class ReflectionUnionType extends ReflectionType
    {

        /** @return ReflectionType[] */
        public function getTypes()
        {
            return [];
        }

    }
}

@samsonasik
Copy link
Member Author

samsonasik commented May 27, 2022

It seems on 0.13, the $rectorConfig->bootstrap() is working ok, while in dev-main, it is not working:

In 0.13

/opt/homebrew/Cellar/php@7.4/7.4.29/bin/php /usr/local/bin/composer rectify -- --clear-cache
> rector --dry-run '--clear-cache'
 46/46 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%

                                                                                                                        
 [OK] Rector is done!                                                                                                   
                                                                                                                        

In dev-main

got error:

                                                                                                                        
 [ERROR] Could not process "src/Controllers/Track.php" file, due to:                                                    
         "System error: "Class "App\Controllers\BaseController" not found"                                              
         Run Rector with "--debug" option and post the report here: https://github.com/rectorphp/rector/issues/new". On 
         line: 24           

The error start from commit rectorphp/rector@6693d5c after config merged.

@TomasVotruba
Copy link
Member

@samsonasik
Copy link
Member Author

Remove the check will got error:

/opt/homebrew/Cellar/php@7.4/7.4.29/bin/php /usr/local/bin/composer rectify -- --clear-cache
> rector --dry-run '--clear-cache'
PHP Fatal error:  Uncaught RectorPrefix20220527\Symfony\Component\DependencyInjection\Exception\ParameterNotFoundException: You have requested a non-existent parameter "phpstan_for_rector_path". in /Users/samsonasik/www/GithubModules/ci4-album/vendor/rector/rector/vendor/symplify/package-builder/src/Parameter/ParameterProvider.php:92
Stack trace:

@TomasVotruba
Copy link
Member

@Wirone
Copy link
Contributor

Wirone commented May 27, 2022

@TomasVotruba FYI: PHPStan 1.7.0-1.7.1 has autoloading issues, fixed in 1.7.2. Make sure you use most recent version 🙂

@TomasVotruba
Copy link
Member

@Wirone Thanks for letting us know 👍

@samsonasik Btw, the revert of test was not complete. The directory is missing:
https://github.com/rectorphp/rector/runs/6628895789?check_suite_focus=true

@samsonasik
Copy link
Member Author

@TomasVotruba thank you, re-add at #2380

@samsonasik
Copy link
Member Author

@Wirone we should already use 1.7.2 as no composer.lock

@samsonasik
Copy link
Member Author

@TomasVotruba I just tried rector/rector rectorphp/rector@7e46eb8 before config merging, and the bootstrap still not autoloaded

                                                                                                                        
 [ERROR] Could not process "src/Controllers/Track.php" file, due to:                                                    
         "System error: "Class "App\Controllers\BaseController" not found"                                              
         Run Rector with "--debug" option and post the report here: https://github.com/rectorphp/rector/issues/new". On 
         line: 24             

so it possibly a phpstan issue.

@samsonasik
Copy link
Member Author

@Wirone the rector autoload and phpstan is crossed, and the prefixed seems loaded first, that's why the following issue can happen

which for previous use case, working on 0.13, but not work on dev-main

@Wirone
Copy link
Contributor

Wirone commented May 27, 2022

Probably because @ondrejmirtes is experimenting and openly states that some issues could be fixed in 1.7.2 while others could be intruduced 😉 Maybe stick to version that have been working properly before?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants