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

Update method signature for getPropertiesFromTable #136

Merged
merged 3 commits into from
Jun 9, 2021
Merged

Update method signature for getPropertiesFromTable #136

merged 3 commits into from
Jun 9, 2021

Conversation

hendrikheil
Copy link
Contributor

LaravelIdeHelper changed the visibility of getPropertiesFromTable from protected to public in this PR: barryvdh/laravel-ide-helper#945

This causes Psalm to fail with the lastest version

LaravelIdeHelper changed the visibility of `getPropertiesFromTable` from `protected` to `public` in this PR: barryvdh/laravel-ide-helper#945
Copy link
Contributor

@caugner caugner left a comment

Choose a reason for hiding this comment

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

We would probably need to bump the version constraint for barryvdh/laravel-ide-helper to 2.9.2 at the same time.

@hendrikheil
Copy link
Contributor Author

Yeah, my mistake. Just updated composer.json to reflect that

@caugner
Copy link
Contributor

caugner commented Apr 6, 2021

@CiiDyR Thanks! You committed the version constraint with another email address. Was that intentional?

@hendrikheil
Copy link
Contributor Author

hendrikheil commented Apr 6, 2021

@caugner Not really intentional. I made the first commit from GitHub directly, the second one I made locally. My local git config seems to be different between those two :-)

@caugner
Copy link
Contributor

caugner commented Apr 6, 2021

@caugner Not really intentional. I made the first commit from GitHub directly, the second one I made locally. My local git config seems to be different between those two :-)

And/or it seems that the email address you used for the second commit is not (yet) linked to your GitHub account.

@caugner
Copy link
Contributor

caugner commented Apr 6, 2021

I have opened #138 as a first mitigation, as there is at least one other potentially breaking change in the pipeline: https://github.com/barryvdh/laravel-ide-helper/pull/1198/files#r606697817

That change (>=2.8.0 <2.9.2) could be released as 1.4.3 and the change here could be released as 1.5.0 as soon as the barryvdh/laravel-ide-helper interface has stabilised.

Otherwise we would need to specify (>=2.9.2 <2.10.0) here, as it's not clear whether upstream will bump the major version for this or similar changes, see Barry's comment.

/cc @mr-feek

@mr-feek
Copy link
Collaborator

mr-feek commented Apr 7, 2021

This can really only be resolved by ide-helper creating a hotfix bugfix release reverting their changes. We can't retroactively go back and change constraints on our previous releases that said "we work with any minor version updates of ide-helper".

This is very unfortunate.

@yaegassy
Copy link
Contributor

yaegassy commented Jun 9, 2021

@caugner @mr-feek Let me ask you a question.

Currently version-locked "laravel-ide-helper" to >=2.8.0 <2.9.2, but will "psalm-plugin-laravel" itself eventually be deprecated?

@caugner
Copy link
Contributor

caugner commented Jun 9, 2021

@caugner @mr-feek Let me ask you a question.

Currently version-locked "laravel-ide-helper" to >=2.8.0 <2.9.2, but will "psalm-plugin-laravel" itself eventually be deprecated?

It won't, but laravel-ide-helper 2.9.2 contains a breaking change.

@mr-feek
Copy link
Collaborator

mr-feek commented Jun 9, 2021

Doesn't seem like ide-helper is going to make a hot fix release, so it's probably about time for us to require the latest version. I'll take a look tomorrow!

@mr-feek mr-feek added the fix label Jun 9, 2021
@mr-feek mr-feek merged commit 1ff7fd2 into psalm:master Jun 9, 2021
@mr-feek
Copy link
Collaborator

mr-feek commented Jun 9, 2021

Thanks for the PR and friendly reminder @yaegassy / @CiiDyR !

@yaegassy
Copy link
Contributor

I tried v1.4.6, but I get an "error" in psalm.

Repro

cd /tmp
laravel new psalm-plugin-laravel-check
cd psalm-plugin-laravel-check

composer require --dev vimeo/psalm
./vendor/bin/psalm --init

composer require --dev psalm/plugin-laravel
./vendor/bin/psalm-plugin enable psalm/plugin-laravel

./vendor/bin/psalm

Result (Error)

psalm-plugin-laravel-check$ ./vendor/bin/psalm
Scanning files...
PHP Fatal error:  Declaration of Psalm\LaravelPlugin\FakeMetaCommand::registerClassAutoloadExceptions() must be compatible with Barryvdh\LaravelIdeHelper\Console\MetaCommand::registerClassAutoloadExceptions(): callable in /private/tmp/psalm-plugin-laravel-check/vendor/psalm/plugin-laravel/src/FakeMetaCommand.php on line 11

   Symfony\Component\ErrorHandler\Error\FatalError

  Declaration of Psalm\LaravelPlugin\FakeMetaCommand::registerClassAutoloadExceptions() must be compatible with Barryvdh\LaravelIdeHelper\Console\MetaCommand::registerClassAutoloadExceptions(): callable

  at vendor/psalm/plugin-laravel/src/FakeMetaCommand.php:11
      7▕ {
      8▕     /**
      9▕      * @return void
     10▕      */
  ➜  11▕     protected function registerClassAutoloadExceptions()
     12▕     {
     13▕         // by default, the ide-helper throws exceptions when it cannot find a class. However it does not unregister that
     14▕         // autoloader when it is done, and we certainly do not want to throw exceptions when we are simply checking if
     15▕         // a certain class exists. We are instead changing this to be a noop.


   Whoops\Exception\ErrorException

  Declaration of Psalm\LaravelPlugin\FakeMetaCommand::registerClassAutoloadExceptions() must be compatible with Barryvdh\LaravelIdeHelper\Console\MetaCommand::registerClassAutoloadExceptions(): callable

  at vendor/psalm/plugin-laravel/src/FakeMetaCommand.php:11
      7▕ {
      8▕     /**
      9▕      * @return void
     10▕      */
  ➜  11▕     protected function registerClassAutoloadExceptions()
     12▕     {
     13▕         // by default, the ide-helper throws exceptions when it cannot find a class. However it does not unregister that
     14▕         // autoloader when it is done, and we certainly do not want to throw exceptions when we are simply checking if
     15▕         // a certain class exists. We are instead changing this to be a noop.

      +1 vendor frames
  2   [internal]:0
      Whoops\Run::handleShutdown()

Related packages

$ composer show -i | grep laravel
You are using the deprecated option "installed". Only installed packages are shown by default now. The --all option can be used to show all packages.
barryvdh/laravel-ide-helper           v2.10.0   Laravel IDE Helper, generates correct PHPDocs for all Facade classes, to improve auto-complet...
fruitcake/laravel-cors                v2.0.4    Adds CORS (Cross-Origin Resource Sharing) headers support in your Laravel application
laravel/framework                     v8.46.0   The Laravel Framework.
laravel/sail                          v1.8.1    Docker files for running a basic Laravel application.
laravel/tinker                        v2.6.1    Powerful REPL for the Laravel framework.
psalm/plugin-laravel                  v1.4.6    A Laravel plugin for Psalm
spatie/laravel-ray                    1.19.0    Easily debug Laravel apps

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

Successfully merging this pull request may close these issues.

4 participants