Skip to content

Document ext/uri#5392

Open
kocsismate wants to merge 7 commits intophp:masterfrom
kocsismate:uri
Open

Document ext/uri#5392
kocsismate wants to merge 7 commits intophp:masterfrom
kocsismate:uri

Conversation

@kocsismate
Copy link
Copy Markdown
Member

@kocsismate kocsismate commented Feb 26, 2026

Another attempt to document ext/uri after #5060

Some of my sentences are very clunky, so I am happy to receive suggestions.

@kocsismate kocsismate force-pushed the uri branch 7 times, most recently from 0667c3e to c074b2a Compare March 1, 2026 21:38
@kocsismate kocsismate force-pushed the uri branch 4 times, most recently from 3ec979f to 893cc0e Compare March 14, 2026 20:39
@kocsismate kocsismate marked this pull request as ready for review March 14, 2026 20:40
Copy link
Copy Markdown
Member

@TimWolla TimWolla left a comment

Choose a reason for hiding this comment

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

First pass at comments. Didn't yet look at all files, but this looks like a good start. Will take another look after you reviewed my comments to avoid looking at all files twice in case they change.

<refsect1 role="seealso">
&reftitle.seealso;
<simplelist>
<member><methodname>Uri\WhatWg\Url::getPassword</methodname></member>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Here it might make sense to also link to Uri\WhatWg\Url::withUsername (second position).

Suggested change
<member><methodname>Uri\WhatWg\Url::getPassword</methodname></member>
<member><methodname>Uri\WhatWg\Url::getPassword</methodname></member>
<member><methodname>Uri\WhatWg\Url::withUsername</methodname></member>

Copy link
Copy Markdown
Member Author

@kocsismate kocsismate Mar 17, 2026

Choose a reason for hiding this comment

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

I think doing so would make more sense for Uri because password is just a pseudo component together with username. For WHATWG URL, they are completely separate components with not much more connection than e.g. the host and port. I'm not against adding it though, so if you have a strong opinion, then I'm fine with it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

For WHATWG URL, they are completely separate components with not much more connection than e.g. the host and port.

I feel username and password logically belong together (e.g. because setting a password will also add an empty username) and thus I would cross-link them.

@kocsismate
Copy link
Copy Markdown
Member Author

I've just aded the documentation for the two ext/uri enums. Could you please have another look sometime soon? :)

Copy link
Copy Markdown
Member

@TimWolla TimWolla left a comment

Choose a reason for hiding this comment

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

Okay, gone through all files now. Skimmed some of them. If this is resolved, the PR should be good enough for merging and I'll then double-check the rendered version and send PRs 😄

<refsect1 role="seealso">
&reftitle.seealso;
<simplelist>
<member><methodname>Uri\WhatWg\Url::getPassword</methodname></member>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

For WHATWG URL, they are completely separate components with not much more connection than e.g. the host and port.

I feel username and password logically belong together (e.g. because setting a password will also add an empty username) and thus I would cross-link them.

<refsect1 role="seealso">
&reftitle.seealso;
<simplelist>
<member><methodname>Uri\Rfc3986\Uri::getRawPassword</methodname></member>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should this link to the userinfo methods?

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