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

Type Error on ide-helper #37

Closed
schonhoff opened this issue Feb 20, 2023 · 15 comments · Fixed by #39
Closed

Type Error on ide-helper #37

schonhoff opened this issue Feb 20, 2023 · 15 comments · Fixed by #39

Comments

@schonhoff
Copy link

Versions:

  • ide-helper Version: 2.13.0
  • Laravel Version: 10.0.3
  • PHP Version: 8.1.9

Description:

Hey,

after migrating to Laravel 10, I tried to update my ide-helper with php artisan ide-helper:generate. Sadly the following error occoured. I can not update my ide-helper at all. If more information were needed, I will try to provide them.

   TypeError 

  Illuminate\Support\Testing\Fakes\MailFake::__construct(): Argument #1 ($manager) must be of type Illuminate\Mail\MailManager, BeyondCode\HeloLaravel\Laravel7Mailer given, called in l\vendor\laravel\framework\src\Illuminate\Support\Facades\Mail.php on line 68

  at \vendor\laravel\framework\src\Illuminate\Support\Testing\Fakes\MailFake.php:54      
     50▕      *
     51▕      * @param  MailManager  $manager
     52▕      * @return void
     53▕      */
  ➜  54▕     public function __construct(MailManager $manager)
     55▕     {
     56▕         $this->manager = $manager;
     57▕     }
     58▕

  1   \vendor\laravel\framework\src\Illuminate\Support\Facades\Mail.php:68
      Illuminate\Support\Testing\Fakes\MailFake::__construct(Object(BeyondCode\HeloLaravel\Laravel7Mailer))      

  2   \vendor\barryvdh\laravel-ide-helper\src\Alias.php:220
      Illuminate\Support\Facades\Mail::fake()

Steps To Reproduce:

(Does not checked it myself but I would guess it is the same)

  • Fresh Laravel installation
  • beyondcode/helo-laravel as required dev in composer
  • barryvdh/laravel-ide-helper as required dev in composer
  • composer update
  • try to launch php artisan ide-helper:generate

More information:

barryvdh/laravel-ide-helper#1422

@muffycompo
Copy link

Getting the same error myself. Hope this gets looked at. Tested using the following:

  • ide-helper Version: 2.13.0
  • Laravel Version: 10.0.3
  • PHP Version: 8.1.16

@schonhoff
Copy link
Author

Short Info:
It is addressed in this issue from Laravel:
laravel/framework#46180

The Fix should be merged soon. Maybe it will fix this problem. If it fixes the problem I will close the issue.

@muffycompo
Copy link

Thanks @schonhoff for the research. Will be monitoring the changes as well.

@joelbutcher
Copy link
Contributor

joelbutcher commented Feb 22, 2023

@mpociot FYI this issue is fixed by laravel/framework#46188. You will want to bump the minimum supported Laravel version for v10 when it's tagged by Taylor.

@schonhoff
Copy link
Author

Hey @joelbutcher ,

I tried the laravel/framework:v10.1.4 patch but it won't fix this particular problem. Here is a reproduction repo for this problem:
https://github.com/schonhoff/laravel-helo-error
If you use it and just type php artisan ide-helper:generate in the console it throws the error mentioning above.

I would guess, that Laravel helo is not using a MailManager class but using a Mailer class. This was possible before Laravel 10. But I'm not 100% sure.

@schonhoff
Copy link
Author

Hey @joelbutcher,

I did the same twice on different systems and still get the error. Both on Windows 10. That is quite interesting.

Can you try the following:
1.) composer create-project laravel/laravel laravel-helo-error
2.) composer require --dev barryvdh/laravel-ide-helper
3.) composer require --dev beyondcode/helo-laravel
4.) php artisan ide-helper:generate

That is how I set up the repo.

I did a composer clearcache before to be sure that nothing is cached on my side.

@joelbutcher
Copy link
Contributor

It seems this stems primarily from laravel/framework#31073, where Taylor introduced the feature for multiple mailers per app. It's only become a problem for this package because the change wasn't countered for, thus causing this package to break when laravel/framework#45988 was merged.

@schonhoff
Copy link
Author

schonhoff commented Feb 23, 2023

I used dd(static::getFacadeRoot()); on the Illuminate\Support\Facades\Mail to show me the root and got Laravel7Mailer. The Laravel7Mailer class extends the Illuminate\Mail\Mailer but the MailFake wants a MailManager in the constructor. If I remove the MailManager type-hint on line 54 of the Illuminate\Support\Testing\Fakes\MailFake class I can run the command without a problem. I guess something needs to be changed in this package to use the MailManager class. Am I correct with my guess?

Edit:
I will dig into the PRs and have a look if I can submit a PR to the package. Thanks for your help!

@joelbutcher
Copy link
Contributor

@schonhoff That's correct. Since laravel/framework#31073, the facade root for the Mail facade is now the mail manager (which tbh, just defers calls to the default mailer, unless configured otherwise)

@joelbutcher
Copy link
Contributor

@schonhoff I was gonna take a look this afternoon, but feel free to investigate the yourself if you have the time 😁

@schonhoff
Copy link
Author

schonhoff commented Feb 23, 2023

@joelbutcher That would be nice! I'm still stuck with my app to upgrade to Laravel 10 but maybe I can find a solution. If you have the time this afternoon and I had not find a solution you could do it :) Would love to see your solution.

Edit:
Tried something, but it won't work. I'm not good enough in the Facade game :/ sorry

@joelbutcher
Copy link
Contributor

@schonhoff see #39

@schonhoff
Copy link
Author

@joelbutcher I was close :D At least something, I will investigate your PR tomorrow in detail to check what I missed. Thank you for your time and the PR hopefully it will get merged.

@muffycompo
Copy link

muffycompo commented Feb 23, 2023

Again thank you @joelbutcher, @schonhoff for the world class troubleshooting and PR. @mpociot any chance this PR will be reviewed and merged? thanks.

@AtlasApollo
Copy link

+1 for this, same issue. Thank you all for trying to get it fixed, I'm looking forward to the release.

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 a pull request may close this issue.

4 participants