-
Notifications
You must be signed in to change notification settings - Fork 801
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
Conversation
a2f9650
to
3040a8e
Compare
<term><parameter>nodes</parameter></term> | ||
<listitem> | ||
<para> | ||
Description. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
314d435
to
d3bba5e
Compare
A question. The after() method (and others coming from the interface) is not listed on https://www.php.net/manual/en/class.domelement.php |
Oh so that's where they are documented. I mean one solution is to add:
In the inherited method section, maybe having a look at how countable classes do it? As Countable::count also has a page |
Indeed. I've ran into this exact issue myself before: https://chat.stackoverflow.com/transcript/message/56491752#56491752 |
Co-authored-by: Gina Peter Banyard <girgias@php.net>
These still remain and I'll try to finish them soon:
Thanks for having a look already |
Looks like I'm getting some errors because of the xincludes:
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. |
I think this is ready for review now. |
There was a problem hiding this 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.
reference/dom/domchildnode/after.xml
Outdated
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. |
There was a problem hiding this comment.
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/
There was a problem hiding this comment.
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:
&random.engineErrors; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed via entity
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thank you!
Generate using php/php-src#12330 and running the following command (for my folder hierarchy):