Skip to content

dom: Add stubs generated from php-src stubs #2813

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

Merged
merged 45 commits into from
Nov 12, 2023
Merged

Conversation

Girgias
Copy link
Member

@Girgias Girgias commented Sep 30, 2023

Generate using php/php-src#12330 and running the following command (for my folder hierarchy):

../php-src/build/gen_stub.php --generate-methodsynopses ../PHP-8.3/ext/dom/ ../docs-php/en/reference/dom/

<term><parameter>nodes</parameter></term>
<listitem>
<para>
Description.
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it be better to leave these empty instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, ideally, people would work on them directly after generating the stubs. And if no text is generated, one gets an empty tag, which I don't think is great.

@Girgias Girgias force-pushed the stub-8.3-dom branch 5 times, most recently from 314d435 to d3bba5e Compare September 30, 2023 19:08
@nielsdos
Copy link
Member

nielsdos commented Oct 1, 2023

A question.
The after() method on DOMElement etc come from the interface DOMChildNode. Looking at that page I see after() listed there: https://www.php.net/manual/en/class.domchildnode.php
Clicking on after() reveals its documentation, albeit a bit lackluster. It's also hard to discover these methods right now.

The after() method (and others coming from the interface) is not listed on https://www.php.net/manual/en/class.domelement.php
So how do we go about it? I guess the description should be the same for all of them. It's a bit weird that the interface has a page tbh?

@Girgias
Copy link
Member Author

Girgias commented Oct 1, 2023

Oh so that's where they are documented.

I mean one solution is to add:

    <xi:include xpointer="xmlns(db=http://docbook.org/ns/docbook) xpointer(id('class.domparentnode')/db:refentry/db:refsect1[@role='description']/descendant::db:methodsynopsis[@role='DOMParentNode'])">
     <xi:fallback/>
    </xi:include>

In the inherited method section, maybe having a look at how countable classes do it? As Countable::count also has a page

@TimWolla
Copy link
Member

TimWolla commented Oct 1, 2023

It's also hard to discover these methods right now.

Indeed. I've ran into this exact issue myself before: https://chat.stackoverflow.com/transcript/message/56491752#56491752

@nielsdos
Copy link
Member

nielsdos commented Nov 9, 2023

These still remain and I'll try to finish them soon:

  • replaceWith
  • replaceChildren
  • getIterator

Thanks for having a look already

@nielsdos
Copy link
Member

nielsdos commented Nov 9, 2023

Looks like I'm getting some errors because of the xincludes:

File /home/runner/work/doc-en/doc-en/en/reference/dom/domcharacterdata/after.xml No parameters sections.
File /home/runner/work/doc-en/doc-en/en/reference/dom/domcharacterdata/after.xml No returnvalues sections.

are those checked before resolving the xincludes by any chance?

@Girgias
Copy link
Member Author

Girgias commented Nov 10, 2023

Looks like I'm getting some errors because of the xincludes:

File /home/runner/work/doc-en/doc-en/en/reference/dom/domcharacterdata/after.xml No parameters sections.
File /home/runner/work/doc-en/doc-en/en/reference/dom/domcharacterdata/after.xml No returnvalues sections.

are those checked before resolving the xincludes by any chance?

Yes, they don't actually work on the manual but individual files. Just go and add exceptions to the sections_check QA script. As it probably should be written to work on the full manual.

@nielsdos nielsdos marked this pull request as ready for review November 11, 2023 16:00
@nielsdos
Copy link
Member

I think this is ready for review now.
I submitted php/doc-base#106 to fix the integration.

Copy link
Member Author

@Girgias Girgias left a comment

Choose a reason for hiding this comment

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

LGTM except the formatting of the error sections which I am not sure.

variablelist feels inappropriate, I usually think if doing it that way we use itemizedlist but DocBook seems to be OK with using it that way.

Now the wrapping para tag is unnecessary and should be removed.

The last thin is that it would be nice to have the descriptions for the errors somehow only written once and included so that they don't go out of sync.

Comment on lines 51 to 53
Raised if the parent is of a type that does not allow children of the
type of one of the passed <parameter>nodes</parameter>, or if the node to
put in is one of this node's ancestors or this node itself.
Copy link
Member Author

Choose a reason for hiding this comment

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

This might make sense to either use an entity in language snippets, or possibly add a file to XInclude with an URL to a local file? (maybe for translations one want's a fallback that traverses up to directory structure then go down with ../en/reference/dom/

Copy link
Member

Choose a reason for hiding this comment

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

For ext/random, I have this:

Copy link
Member

Choose a reason for hiding this comment

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

Fixed via entity

Copy link
Member Author

@Girgias Girgias left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you!

@Girgias Girgias merged commit 7b1704c into php:master Nov 12, 2023
@Girgias Girgias deleted the stub-8.3-dom branch November 12, 2023 13:40
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.

4 participants