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

monolog-discord-handler pulls a version of GuzzleHttp incompatible with other plugins #4

Closed
drzraf opened this issue Nov 11, 2021 · 9 comments

Comments

@drzraf
Copy link

drzraf commented Nov 11, 2021

Got Call to undefined method GuzzleHttp\Utils::chooseHandler() at user/plugins/logger-channels/vendor/guzzlehttp/guzzle/src/functions.php:61

Related to bundling the libraries instead of using Grav's ones: Apparently, when loaded after login-oauth2 plugin (which bundles guzzlehttp/guzzle 6.5.4), the autoload is messed up and subsequent GuzzleHttp\Utils references are the one to the old version, missing chooseHandler.

Screenshot from 2021-11-11 08-44-49

@drzraf
Copy link
Author

drzraf commented Nov 11, 2021

Ironically, this dependency is pulled by:
lefuturiste/monolog-discord-handler 0.2.1 (which I have no need for)
which requires guzzlehttp/guzzle (*) (so it pulled the latest stable but login-oauth2 would perfectly fit too)

Let alone the fact that it's loaded even though I don't even configure this handler.

The workaround is simple:
sed -ri '/telegram-handler|monolog-discord-handler/d' composer.json && rm -f composer.lock && composer install but I wonder if there is any other solution than a custom fork to handle this in the long term (other than #1).

By the way, given the comment on grav-plugin-login-oauth2#41 it's very important that each plugin dependencies be restricted to a minimum.

@drzraf drzraf changed the title Call to undefined method GuzzleHttp\Utils::chooseHandler() monolog-discord-handler pulls a version of GuzzleHttp incompatible with other plugins Nov 11, 2021
@stephan-strate
Copy link
Owner

This is indeed a problem, as mahagr pointed out already. I have no real chance of loading dependencies only if they are configured. An idea would be to have grav-plugin-logger-channels as basic implementation and addon plugins for each handler integration. I think close to no one will ever need more than one handler/target to send logs to.

I will try to get a poc working this weekend and let you know how to build the graylog handler addon plugin yourself. This would be a much smoother workflow for all of us, because you have the tools and setup to test graylog properly.

@stephan-strate
Copy link
Owner

I will think about this again. Maybe it is better to implement such handlers, that require additional dependencies, ourselfs. As far as I know the "addon pattern" is not officially supported by the grav core team and therefore not really reliable.
As I said, I will have a look at this on the weekend and let you know what the solution is. Goal is to remove all dependencies, that could interfere with other plugins.

@drzraf
Copy link
Author

drzraf commented Nov 11, 2021

Multiple plugins/projects is a developing/maintenance pain and still seem overkill.

  • Any composer plugin that could help?
  • Some custom flag to bin/gpm or a custom hook to handle a particular gpm command-line stuff?
  • or using branch so that gpm syntax allows to fetch it directly?
  • or bundling the code but not autoloading it (requir'ing the code only if the plugin is loaded and according to configuration)?

@stephan-strate
Copy link
Owner

I am not quite sure what you mean with gpm hook or flags. The package manager basically just downloads the plugin and puts it in the right folder. There is not much to configure. It also needs to be compatible with admin.

Autoloading stuff only if its needed is a nice optimization, but does not solve the problem.

I aggree with you, that maintaining multiple plugins is a pain. So I will try to easily implement the handlers myself to reduce the external dependencies. They are super simple to implement. This will solve the problem once and for all.

@drzraf
Copy link
Author

drzraf commented Nov 12, 2021

This is the autoload:

    'Psr\\Log\\' => array($vendorDir . '/psr/log/Psr/Log'),
    'Psr\\Http\\Message\\' => array($vendorDir . '/psr/http-message/src'),
    'Psr\\Http\\Client\\' => array($vendorDir . '/psr/http-client/src'),
    'ParagonIE\\ConstantTime\\' => array($vendorDir . '/paragonie/constant_time_encoding/src'),
    'Monolog\\' => array($vendorDir . '/monolog/monolog/src/Monolog'),
    'Mero\\Monolog\\' => array($vendorDir . '/mero/telegram-handler/src/Mero/Monolog'),
    'GuzzleHttp\\Psr7\\' => array($vendorDir . '/guzzlehttp/psr7/src'),
    'GuzzleHttp\\Promise\\' => array($vendorDir . '/guzzlehttp/promises/src'),
    'GuzzleHttp\\' => array($vendorDir . '/guzzlehttp/guzzle/src'),
    'Grav\\Plugin\\LoggerChannels\\' => array($baseDir . '/classes'),
    'Gelf\\' => array($vendorDir . '/graylog2/gelf-php/src/Gelf'),
    'DiscordHandler\\' => array($vendorDir . '/lefuturiste/monolog-discord-handler/src'),

Maybe the autoload() function could be tweaked only spl_autoload_register the necessary classes on demand?

Since neither composer nor gpm allows package's "flavors", another distribution/packaging-based solution would be to create various plugin pointing to different branch of this very repository (then it's up to the GPM repository manager to see whether it's acceptable or not).

I think any of these would be a more long-term solution than reimplementing the handlers by hand (which means much more unaudited custom bundled code that'd hardly match latest upstream changes/bugfixes). The Gelf handler is far from obvious btw.

Please note that GPM also allow to install from a zip file URL (which, in this case, is handy to use a custom fork's zip file)

@stephan-strate
Copy link
Owner

I won't tweak around in an auto generated composer file. This is so far from stable and long-term.

And again, this does not solve the problem, maybe for your usecase, but not for the plugin itself. You want to get rid of the discord handler, I get this, but I need to implement the discord handler to not use external dependencies.
My plan is not to reimplement every handler as most of them use basic curl operations and do not install guzzle or something similar. The discord handler is super simple and straight forward. This would already solve the problem for this plugin.

Trust me, its not the grav way to autload anything based on configuration. This would be super experimental and not stable at all. Maybe in the future grav will support something like this, but at the moment they don't.

@stephan-strate
Copy link
Owner

I created a simple Discord handler: https://github.com/stephan-strate/monolog-discord
Will do some more testing and then change the discord handler used by this plugin.

@stephan-strate
Copy link
Owner

Thanks for reporting the issue :)
The problem should be solved now. It will take some hours until GPM will pick up the new version.
This plugin now only requires the minimal amount of dependencies.

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

No branches or pull requests

2 participants