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 UriFactory::createFromSapi() #123

Merged
merged 2 commits into from
Nov 22, 2022

Conversation

markstory
Copy link
Contributor

Q A
Documentation yes
Bugfix no
BC Break no
New Feature yes
RFC yes
QA no

Description

This method will provide a public API to create Uri instances from SERVER context. This helps libraries that depend on diactoros and have their own ServerRequest or Uri implementations use the logic in diactoros more efficiently.

I wanted to get a rough draft of this up for feedback. If it is headed in the right direction, I have a few TODOs remaining:

  • Complete the tests (coverage isn't quite on par with the rest of the library).
  • Write documentation. I was thinking of putting it the factories section, but would appreciate any direction here.

If there are other requirements that need to be met let me know 😄

Refs #122

This method will provide a public API to create Uri instances from
SERVER context. This helps libraries that depend on diactoros and have
their own ServerRequest or Uri implementations use the logic in
diactoros more efficiently.

Signed-off-by: Mark Story <[email protected]>
src/UriFactory.php Outdated Show resolved Hide resolved
src/UriFactory.php Outdated Show resolved Hide resolved
src/UriFactory.php Show resolved Hide resolved
test/UriFactoryTest.php Show resolved Hide resolved
@Ocramius
Copy link
Member

/cc @froschdesign for getting guidance on the docs section

@Ocramius
Copy link
Member

BTW, overall direction of this patch is good 👍

@markstory
Copy link
Contributor Author

I've resolved all but one of psalm's complaints. The remaining MixedArgumentTypeCoercion was added to the baseline file beside the existing one in the same method.

Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Thanks @markstory!

@Ocramius
Copy link
Member

Went through it again: I know most code is just transposed as-is, so we're good here.

Releasing 🚢

@Ocramius Ocramius merged commit df8c7f9 into laminas:2.22.x Nov 22, 2022
@Ocramius Ocramius changed the title Add UriFactory::createFromSapi() Add UriFactory::createFromSapi() Nov 22, 2022
@markstory markstory deleted the feat-uri-from-sapi branch November 22, 2022 15:56
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