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

Add HTTP status helper class #833

Merged
merged 3 commits into from
Feb 12, 2024
Merged

Add HTTP status helper class #833

merged 3 commits into from
Feb 12, 2024

Conversation

schlessera
Copy link
Member

@schlessera schlessera commented Oct 9, 2023

This PR adds a utility class to deal with HTTP status codes and messages and then changes existing code in Http exceptions and the TransportMock to make use of this utility class.

Pull Request Type

  • I have checked there is no other PR open for the same change.

This is a:

  • Bug fix
  • New feature
  • Documentation improvement
  • Code quality improvement

Context

PR #582 added a new TransportRedirectMock which was based on TransportMock. While doing so, it needed to duplicate the long list of status code messages already included in TransportMock. To make this process easier and simplify maintenance, we decided to extract knowledge about HTTP status codes and texts into a separate utility class instead, making both transport mock classes simpler in the process.

Detailed Description

The new helper class HttpStatus contains two methods:

  • get_text($code) retrieves a status text from a status code
  • is_valid_code($code) returns a boolean on whether a given code is a valid (known) status code

Note: This will need a follow-up PR to complete the list of HTTP status codes and texts, as the current codebase is outdated and does not contain all official HTTP status codes yet.

The helper class also contains constants that can be directly referenced, like so:

$not_found_status_message = '404 ' . HttpStatus::TEXT_404;

The PR updates all Http exceptions to make use of this to de-duplicate the status text strings.

Quality assurance

  • This change does NOT contain a breaking change (fix or feature that would cause existing functionality to change).
  • I have commented my code, particularly in hard-to-understand areas.
  • I have added unit tests to accompany this PR.
  • The (new/existing) tests cover this PR 100%.
  • I have (manually) tested this code to the best of my abilities.
  • My code follows the style guidelines of this project.

Documentation

For new features:

  • I have added a code example showing how to use this feature to the examples directory.
  • I have added documentation about this feature to the docs directory.
    If the documentation is in a new markdown file, I have added a link to this new file to the Docs folder README.md file.

@schlessera schlessera force-pushed the add/http-status-code-helper branch 3 times, most recently from 11f6958 to cca92a9 Compare October 9, 2023 10:12
@schlessera schlessera requested a review from jrfnl October 9, 2023 10:51
Copy link
Member

@jrfnl jrfnl left a comment

Choose a reason for hiding this comment

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

Reviewed. Left a few notes. Nothing major.

src/Utility/HttpStatus.php Outdated Show resolved Hide resolved
src/Utility/HttpStatus.php Outdated Show resolved Hide resolved
tests/Utility/HttpStatus/GetTextTest.php Outdated Show resolved Hide resolved
tests/Utility/HttpStatus/GetTextTest.php Show resolved Hide resolved
src/Utility/HttpStatus.php Outdated Show resolved Hide resolved
src/Utility/HttpStatus.php Outdated Show resolved Hide resolved
src/Utility/HttpStatus.php Outdated Show resolved Hide resolved
@jrfnl jrfnl added this to the 2.1.0 milestone Oct 9, 2023
@schlessera schlessera marked this pull request as ready for review December 11, 2023 13:13
Copy link
Member

@jrfnl jrfnl left a comment

Choose a reason for hiding this comment

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

Looking good, left a few notes inline. Nothing major.

src/Utility/HttpStatus.php Outdated Show resolved Hide resolved
tests/Exception/Http/HttpTest.php Outdated Show resolved Hide resolved
tests/Exception/Http/HttpTest.php Outdated Show resolved Hide resolved
tests/Utility/HttpStatus/GetTextTest.php Show resolved Hide resolved
Copy link
Member

@jrfnl jrfnl left a comment

Choose a reason for hiding this comment

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

Thanks for all your work on this @schlessera ! LGTM.

I've added two small commits still with some very minor nitpicks (mostly CS).

Do you want to clean up the branch before merge ? Or shall I merge it as is ?

@jrfnl
Copy link
Member

jrfnl commented Jan 9, 2024

P.S.: build failures are due to GH having trouble. We'll probably best restart them once GH has recovered.

This class provides constants with the available HTTP status codes with thorough documentation based on https://developer.mozilla.org/en-US/docs/Web/HTTP/Status.
It also comes with helper functions get_text($code) and is_valid_code($code) to simplify consuming code.
@jrfnl
Copy link
Member

jrfnl commented Feb 12, 2024

Yippia-a-yeah! Ready for merge once the build finishes.

@jrfnl jrfnl enabled auto-merge February 12, 2024 10:36
@jrfnl jrfnl merged commit 90db63a into develop Feb 12, 2024
28 checks passed
@jrfnl jrfnl deleted the add/http-status-code-helper branch February 12, 2024 10:38
@jrfnl jrfnl mentioned this pull request Feb 12, 2024
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants