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

Document Psr\Http\Message\UriInterface as an allowed type for $url args #125

Closed
wants to merge 1 commit into from

Conversation

mbabker
Copy link
Contributor

@mbabker mbabker commented Aug 3, 2021

In places where a URL is expected, the doc blocks state that strings are required, but there is no type enforcement at the moment. Thanks to the implementation details of GuzzleHttp\Psr7\uri_for() (or its replacement method in their 2.0 release), any Psr\Http\Message\UriInterface is also acceptable in the Pawl API. So, this PR updates the doc blocks to note that both strings and Psr\Http\Message\UriInterface objects are acceptable types when a URL is expected.

Copy link
Member

@clue clue left a comment

Choose a reason for hiding this comment

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

@mbabker Thanks for filing this PR, perhaps you can help me understand what's the motivation for this change?

It looks like casting an instance implementing UriInterface to a string is somewhat trivial, is adding a union type really worth it? I would lean towards keeping the simple type definition as is (Poka-yoke) unless we have a good reason to accept more types here.

@mbabker
Copy link
Contributor Author

mbabker commented Aug 23, 2021

I'd say that since you can already slip through a UriInterface since there isn't strict type checking in place, it doesn't necessarily hurt anything to officially support it. Beyond that, you end up wasting a few CPU cycles building the URL as a UriInterface (which I do in an app to make a couple of dynamic bits easier to deal with), then casting it to a string, only for this library to convert it to a new UriInterface a little later on.

All in all, I'd call it a mild DX improvement since ultimately the Connector internals are going to be working with a UriInterface anyway.

@clue
Copy link
Member

clue commented Aug 24, 2021

I'd say that since you can already slip through a UriInterface since there isn't strict type checking in place, it doesn't necessarily hurt anything to officially support it.

This project currently uses a docbloc type hint, so passing anything other than a string is prohibited and might lead to unexpected behavior. Static analysis tools such as phpstan or even your IDE will warn you against such usage. This project currently targets PHP 5.4+ which is why it doesn't use PHP's primitive type declarations introduced with PHP 7.0. If we really want to use union type declarations, we would have to target PHP 8.0 (which I would argue would be a bit premature considering the current roadmap).

Beyond that, you end up wasting a few CPU cycles building the URL as a UriInterface (which I do in an app to make a couple of dynamic bits easier to deal with), then casting it to a string, only for this library to convert it to a new UriInterface a little later on.

Fair point, but I would argue this is an implementation detail that might change in the future (see #2) and even now the overhead of creating this object is negligible because it's not something that would execute on your critical path.

Very much appreciate the constructive discussion by the way and keep in mind I'm not responsible for maintaining this project, so take my input with a grain of salt. I'm currently looking into updating a few aspects of this project and seeing if this project could take advantage of a more JavaScript-like API (WebSocket API) and I would argue that supporting a wider variety of types for its API might lead to a more confusing API in the long run.

@mbabker mbabker closed this Nov 7, 2021
@mbabker mbabker deleted the patch-1 branch November 7, 2021 17:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants