Skip to content

Document ext/uri#5392

Open
kocsismate wants to merge 1 commit intophp:masterfrom
kocsismate:uri
Open

Document ext/uri#5392
kocsismate wants to merge 1 commit intophp:masterfrom
kocsismate:uri

Conversation

@kocsismate
Copy link
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.

Co-authored-by: Jordi Kroon <jkroon@onyourmarks.agency>
@kocsismate kocsismate marked this pull request as ready for review March 14, 2026 20:40
Copy link
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.

<term><parameter>context</parameter></term>
<listitem>
<simpara>
Context of the error.
Copy link
Member

Choose a reason for hiding this comment

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

This is pretty tautological.

Suggested change
Context of the error.
The input URL at the point where the error was detected.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, documenting such a huge API is very painful, so I usually tried to rely on very basic content (it can be improved over time). But I agree, your suggestion is much better!

Copy link
Member

Choose a reason for hiding this comment

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

so I usually tried to rely on very basic content (it can be improved over time)

Absolutely. I'm happy to get this merged when the obvious mistakes (e.g. example code is there, but the output is still a placeholder) are fixed and will then go through the rendered version to propose better explanations.

Comment on lines +77 to +79
<member><methodname>Uri\Rfc3986\Uri::getUsername</methodname></member>
<member><methodname>Uri\Rfc3986\Uri::getRawUsername</methodname></member>
<member><methodname>Uri\WhatWg\Url::getUsername</methodname></member>
Copy link
Member

Choose a reason for hiding this comment

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

I don't think the RFC 3986 getters are appropriate "see also" for this method.

Suggested change
<member><methodname>Uri\Rfc3986\Uri::getUsername</methodname></member>
<member><methodname>Uri\Rfc3986\Uri::getRawUsername</methodname></member>
<member><methodname>Uri\WhatWg\Url::getUsername</methodname></member>
<member><methodname>Uri\WhatWg\Url::getUsername</methodname></member>
<member><methodname>Uri\Rfc3986\Uri::withUserinfo</methodname></member>

Comment on lines +76 to +77
<member><methodname>Uri\Rfc3986\Uri::withScheme</methodname></member>
<member><methodname>Uri\WhatWg\Url::getScheme</methodname></member>
Copy link
Member

Choose a reason for hiding this comment

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

Prefer references of the same class first (same for the others).

Suggested change
<member><methodname>Uri\Rfc3986\Uri::withScheme</methodname></member>
<member><methodname>Uri\WhatWg\Url::getScheme</methodname></member>
<member><methodname>Uri\WhatWg\Url::getScheme</methodname></member>
<member><methodname>Uri\Rfc3986\Uri::withScheme</methodname></member>

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

Choose a reason for hiding this comment

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

It should reference withUserInfo: For the same class the getters should be listed, for the other class, the corresponding with-er should be lsited.

&example.outputs;
<screen>
<![CDATA[
Code example
Copy link
Member

Choose a reason for hiding this comment

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

Placeholder.

Comment on lines +36 to +38
<simpara>
Description.
</simpara>
Copy link
Member

Choose a reason for hiding this comment

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

Placeholder.

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't have a good idea what kind of description I should write in the examples. Do you have a good suggestion?

Copy link
Member

Choose a reason for hiding this comment

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

The description for the example can just be dropped if you have nothing useful to say.

Suggested change
<simpara>
Description.
</simpara>

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah yes, that makes sense

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

Choose a reason for hiding this comment

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

I think listing the contructor / parse method here is appropriate.

</methodsynopsis>
&warn.undocumented.func;
<simpara>
Description.
Copy link
Member

Choose a reason for hiding this comment

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

Placeholder

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