-
Notifications
You must be signed in to change notification settings - Fork 44
Conversation
Hello @bensherred You still need: |
Ah that makes sense, I've gone ahead and added that. Thanks for your help @shokme. |
There is a problem, you need to instantiate Guzzle inside the MeilisearchServiceProvider that break all laravel version that are not using Guzzle 7. But I found a potential solution I have to discuss with ppshobi about it. see you in meilisearch/meilisearch-php#76 |
Has laravel framework its own HTTP client that we can provide? |
@curquiza Laravel has a HTTP Client but it's just a wrapper for Guzzle. We can't require guzzle 6 either as Laravel 8 now uses Guzzle 7 instead. |
Following the updates to |
Hum, not really good to have an alpha version in our codebase. Plus if everything is good during testing, I will create the official v0.14.1 this afternoon after my lunch (Paris local time 😅). All should be fixed at the end of this day (Paris local time again sorry) maximum. |
Ah, that makes sense, I wasn't sure what your plans were on an official release. No point updating supporting alpha if it will likely be released this afternoon. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some questions and remarks! 🙂
@@ -24,7 +24,8 @@ | |||
"require": { | |||
"php": "^7.2.5", | |||
"laravel/scout": "^8.0", | |||
"meilisearch/meilisearch-php": "^0.13" | |||
"meilisearch/meilisearch-php": "^0.14", | |||
"http-interop/http-factory-guzzle": "^1.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aren't http-interop/http-factory-guzzle
installed by default in Laravel 8 as Guzzle 7 is? 🤞
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think Laravel comes with http-interop/http-factory-guzzle
installed, here's the composer.json. I've had a look through those dependencies as well and doesn't look like any require them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be worth removing still though as it's only required for Guzzle 6 and 7. I could add a note in the README about it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No the factory is not installed by default.
This is only here to exploit PSR-17 (the php sdk discover the factory and use it for thing like $this->requestFactory->createRequest(...)
.
I don't know if we should remove it, a part of me is yeah we should because we don't want to "tie" the package with guzzle but in other hand as laravel pre-install guzzle since the version 7 why should we not simplify user life by pre-installing the factory, it will be good for most of laravel users.
For me both are ok, but I prefer option 2 😛
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's keeping it (option 2). If someone is struggling with this decision, we will see!
A list of compatible HTTP clients and client adapters can be found at [php-http.org](http://docs.php-http.org/en/latest/clients.html). | ||
|
||
If you use **Laravel 8** you can skip this section as laravel pre-install Guzzle 7 by default. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we add a line for Guzzle 7:
$ composer require guzzlehttp/guzzle
For users using Laravel < 8 but who want to use guzzle 7
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it ok to you ?
Thanks again for this PR @bensherred! |
Not sure if I am doing something wrong with the rebase @curquiza? |
@bensherred, it looks like you did not do the first step I don't know if you join to our Slack, but if you want direct chatting for the rebase, join us (I'm Clementine). |
If you think all is ready @shokme, you can merge when you want! 🙂 |
Updated the
meilisearch-php
dependency to v0.14.0. This fixes any locked dependency issues with guzzle 6 to allow any psr18 compatible HTTP client to be used instead.This will fix #43.
As Laravel comes with Guzzle out the box, we do not need to require the additional installation of the following dependencies
php-http/guzzle6-adapter:^2.0