search

gnello / php-mattermost-driver

The PHP Driver to interact with the Mattermost Web Service API.
Apache License 2.0
79 stars 27 forks source link

Decouple from Pimple #50

Open bradjones1 opened 2 years ago

bradjones1 commented 2 years ago

Thanks for this handy package. Pimple development has ended/is in maintenance mode, and I don't necessarily think this package need be aware of the dependency injection container per se. Open to decoupling from pimple/pimple and refactoring to allow dependencies like the HTTP client be pulled in as a dependency from your DIC of choice?

gnello commented 2 years ago

Hi @bradjones1, thanks for forcing me to think about this. You're totally right, this package should follow a more SOLID approach.

I'm thinking of changing the constructor as follows

public function __construct(array $options[])

rather than

public function __construct(Psr\Container\ContainerInterface $container)

What do you think? Do you have any suggestions?

akuzia commented 1 year ago

@gnello I've made a fork removing pimple and switching from Guzzle to any PSR7/17/18 compatible http client implementation. If you are ok with this changes I'm willing to continue working on that in next several weeks until changes are merged to the upstream.

tacman commented 1 year ago

both those changes sound great -- can you also update the documentation to use the Symfony HttpClient instead of guzzle? And if it's ready, submit a PR, since having guzzle and pimple just for this library seems a bit heavy. Thanks.

akuzia commented 1 year ago

@tacman I've updated README & added #54

gnello commented 1 year ago

Hi @akuzia, @tacman, these are very good changes.

I think we should bump the version and make all possible improvements before tagging the 3.0.0, e.g. require php8. I will check the pull requests and look for other possible breaking changes to bring in.