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

Add stub for Http facade #2051

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

Add stub for Http facade #2051

wants to merge 2 commits into from

Conversation

adriaanzon
Copy link

@adriaanzon adriaanzon commented Oct 1, 2024 •
edited
Loading

  • Added or updated tests
  • Documented user facing changes

Given the following code:

try {
    $response = Http::get('http://localhost:3000/health');
} catch (ConnectionException) {
    return Result::make()->failed('Unreachable');
}

PHPStan currently emits a false positive because it doesn't know about the @throws tag defined here. The only way PHPStan currently knows about the get method is via the @method tag on the Http facade here, which does not and cannot include a @throws tag.

 ------ -------------------------------------------------------------------------------------------
  Line   app/Health/GotenbergCheck.php
 ------ -------------------------------------------------------------------------------------------
  16     Dead catch - Illuminate\Http\Client\ConnectionException is never thrown in the try block.
         🪪  catch.neverThrown
 ------ -------------------------------------------------------------------------------------------

Changes

Defines Illuminate\Http\Client\Factory as a mixin on the Http facade. This class has a mixin tag for Illuminate\Http\Client\PendingRequest which includes the actual get method (and other request methods).

Breaking changes

When your code contains calls like Http:get() without a catch statement or @throws tag, PHPStan will start emitting errors like these:

 ------ --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
  Line   modules/Language/Database/Seeders/ImportIso639Part3Table.php
 ------ --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
  34     Method Modules\Language\Database\Seeders\ImportIso639Part3Table::synchronizeLanguages() throws checked exception Illuminate\Http\Client\ConnectionException but it's missing
         from the PHPDoc @throws tag.
         🪪  missingType.checkedException
  69     Method Modules\Language\Database\Seeders\ImportIso639Part3Table::synchronizeMacrolanguageDefinitions() throws checked exception Illuminate\Http\Client\ConnectionException but
         it's missing from the PHPDoc @throws tag.
         🪪  missingType.checkedException
 ------ --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------

However, you will already get this when calling any method on the Http facade that returns a PendingRequest before finally calling the ->get() method, so I do not consider this to be a breaking change, but rather just a fix.

@canvural
Copy link
Collaborator

canvural commented Oct 1, 2024

Hi,

Thank you for the PR. Can you add some tests for it too?

@adriaanzon
Copy link
Author

@canvural I'm not really sure how to add a test for a stub. Can you point me to a similar test that I can use as an example?

@calebdw
Copy link
Contributor

calebdw commented Oct 2, 2024

You can write an integration test for the same failure you experienced

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@adriaanzon @canvural @calebdw