search

verumconsilium / laravel-browsershot

Browsershot wrapper for Laravel 5
MIT License
111 stars 20 forks source link

Reproducing output from first iteration when used in loop or Laravel queue job #1

Closed satyago closed 5 years ago

satyago commented 6 years ago

It seems there are issues when laravel-browsershot is used from a queue job or loop. The behavior here seems to be that it caches whatever Html you load, then generates the same pdf or screenshot as done on the first iteration.

I first suspected spatie/browsershot or Puppeteer to be the reason, but using spatie's Browsershot directly doesn't show the same behavior. Original report issued here https://github.com/spatie/browsershot/issues/197

satyago commented 6 years ago

With a little help from the friendly people at Slack, the problem was traced back to how Laravel's facades work.

Because once a class is resolved through the Facade, it is cached on a static property and will always return the same instance.

The only solution to the problem is to NOT use the facade and instead resolve a fresh instance out of the IOC container like this: $pdf = resolve('browsershot.pdf'); or $screenshot = resolve('browsershot.screenshot');. After that, one can continue to use the instance as normally, with the exception to not using static methods.

The only suggestions I can make are to remove Facade support from the package, move the bind to register in the service provider and ideally resolve against interfaces. Either that and update the Readme to reflect proper usage, or provide both options and update the Readme anyways.

I could offer you to do a pull request, but you have to decide ;-)

Thank you for providing this package by the way !

AkenRoberts commented 6 years ago

I'd recommend adding a Factory pattern to create PDF and Screenshot instances, instead of binding instances directly to the container. You could make the Facade the factory, or add factory creation methods to the individual PDF and Screenshot wrappers (personally I'd recommend the former, with a dedicated Factory class that could be dependency injected as an alternative to the facade).

verumconsilium commented 5 years ago

sorry for the long wait, I didn't have time to properly mantain the package due to personal problems but now I'm intending to resolve all the opened issues and the ones to come

I'm considering the options here, I think I'll go for the Factory Pattern since it feels more clean Hope to close this issue soon, stay tuned