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.

Sign up for GitHub

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

Jump to bottom

Replace Guzzle with generic PSR18 discovery #99

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Replace Guzzle with generic PSR18 discovery #99

wants to merge 2 commits into from

Conversation

GuySartorelli
Copy link
Contributor

Resolves #91
Note that this would most likely need to be a new major release, as it changes the method signature of several methods which were previously relying on a Guzzle-specific options array.

This works well locally, but I have never used phpspec before and I don't know how to update the tests. Can someone with some knowledge of phpspec please give me some guidance?

All of the ClientSpec tests are failing - the error message and trace is:

Packagist/Api/Client                                                              
  29  - it search for packages
      exception [err:TypeError("Double\GuzzleHttp\Client\P1::sendRequest(): Return value must be of type Psr\Http\Message\ResponseInterface, null returned")] has been thrown.
       0 vendor/phpspec/prophecy/src/Prophecy/Doubler/Generator/ClassCreator.php(44) : eval()'d code:25
         throw new PhpSpec\Exception\ErrorException("Double\GuzzleHttp\Client\...")
       1 vendor/symfony/console/Application.php:1096
         Symfony\Component\Console\Command\Command->run()
       2 vendor/symfony/console/Application.php:324
         Symfony\Component\Console\Application->doRunCommand()
       3 vendor/phpspec/phpspec/src/PhpSpec/Console/Application.php:91
         Symfony\Component\Console\Application->doRun()
       4 vendor/phpspec/phpspec/bin/phpspec:25
         Symfony\Component\Console\Application->run()
       5 vendor/phpspec/phpspec/bin/phpspec:27
         {closure}()
       6 vendor/bin/phpspec:120
         include("/home/gsartorelli/dump/te...")

@GuySartorelli GuySartorelli mentioned this pull request Dec 14, 2023
Open
robbieaverill
robbieaverill reviewed Jan 11, 2024
View reviewed changes
Copy link
Collaborator

@robbieaverill robbieaverill left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've only spend a little bit of time so far on the tests, but haven't managed to work out how to get them running either. I'm not that familiar with PSR-17/18 so did some reading about them too, and the supporting packages. I'll spend more time on this soon.

I have a feeling this is the part that isn't being mocked correctly in the PHPSpec tests:

$this->httpClient = new HttpMethodsClient(
    $httpClient,
    Psr17FactoryDiscovery::findRequestFactory(),
    Psr17FactoryDiscovery::findStreamFactory()
);

src/Packagist/Api/Client.php
*/
protected function postRequest(string $url, array $options): string
protected function postRequest(string $url, array $headers = [], string|StreamInterface|null $body = null): string
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This branch currently still supports PHP 7.4, which doesn't let us use union types. Since PHP 7.4 is EOL, I think we can bump to ^8.1 and keep these types.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. I've made this change

@robbieaverill
Copy link
Collaborator

Sorry, I haven't had time to look into this again, but it is on my mind.

@GuySartorelli
Copy link
Contributor Author

No worries, there's no rush.

@robbieaverill
Copy link
Collaborator

@GuySartorelli I'm not sure when I will have time to look more into this

@GuySartorelli
Copy link
Contributor Author

That's fine, I don't have any real need for this - just saw an opportunity to tidy up this package's dependencies and figured I'd give it a go.
If I get a chance in the next few weeks I'll try learn how mocking works with this testing library.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@robbieaverill robbieaverill robbieaverill left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Don't require guzzle explicitly
2 participants
@GuySartorelli @robbieaverill