beyondcode / helo-laravel

HELO Laravel helper package to add
MIT License
87 stars 24 forks source link

Type Error on ide-helper #37

Closed schonhoff closed 1 year ago

schonhoff commented 1 year ago

Versions:

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)

More information:

https://github.com/barryvdh/laravel-ide-helper/issues/1422

muffycompo commented 1 year ago

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

schonhoff commented 1 year ago

Short Info: It is addressed in this issue from Laravel: https://github.com/laravel/framework/issues/46180

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

muffycompo commented 1 year ago

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

joelbutcher commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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

schonhoff commented 1 year ago

@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 commented 1 year ago

@schonhoff see #39

schonhoff commented 1 year ago

@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 commented 1 year ago

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

AtlasApollo commented 1 year ago

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