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

[WIP] 8.4: Document all new DOM classes and methods #4212

Draft
wants to merge 42 commits into
base: master
Choose a base branch
from

Conversation

nielsdos
Copy link
Member

@nielsdos nielsdos commented Dec 1, 2024

Very WIP, not ready for review yet.

TODO:

  • Update other extensions that link into DOM
  • Update constants
  • Finish class skeletons (class page, properties, ...)
  • Document the NO_DEFAULT_NS constant (with caution)
  • Document methods
  • Update note about encoding
  • Create examples of array indexing in NamedNodeMap ? Let's do this separately
  • Add a note on top of the dom book to guide users to the new classes?
  • Explain xpath subtleties
  • Add note to XMLDocument properties that you should rely on the LIBXML_ flags now

@nielsdos nielsdos force-pushed the dom-84 branch 2 times, most recently from b40772e to 6b116fb Compare December 1, 2024 15:49
@nielsdos nielsdos added this to the PHP 8.4 milestone Dec 1, 2024
@nielsdos nielsdos force-pushed the dom-84 branch 11 times, most recently from 5b1b19a to f762c1a Compare December 1, 2024 20:55
@nielsdos
Copy link
Member Author

nielsdos commented Dec 1, 2024

@Girgias I've finished with some basics. Most notably the class synopses are generated and the properties etc are all filled in. I've used xinclude quite a bit. I also fixed up some of the old DOM class descriptions.
It's probably best to already review this as-is, even though it's not finished and most notably the methods are missing.
This won't compile out of the box because of the entities.whatever.xml files not being generated because I haven't filled in the method stuff yet, but here's a zip that you can extract in reference/dom/dom to make it build:
entities.dom.zip
You also need php/doc-base#193

Copy link
Member

@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.

Didn't test locally but this a pass across the whole PR

Comment on lines 278 to +280
<row xml:id="constant.domstring-size-err">
<entry>
<constant>DOMSTRING_SIZE_ERR</constant>
<constant>DOMSTRING_SIZE_ERR</constant> / <constant>Dom\STRING_SIZE_ERR</constant>
Copy link
Member

Choose a reason for hiding this comment

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

The issue I can see for this, is that automatic linking for <constant>Dom\STRING_SIZE_ERR</constant> is not going to work because the row has no corresponding XML ID.

Can an XML element have 2 IDs? If yes that would fix it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Can an XML element have 2 IDs? If yes that would fix it.

No, but I don't think this is ever linked to directly 🤔

reference/dom/dom/dom-attr.xml Outdated Show resolved Hide resolved
reference/dom/dom/dom-attr.xml Show resolved Hide resolved
reference/dom/dom/dom-document.xml Show resolved Hide resolved
reference/dom/dom/dom-document.xml Outdated Show resolved Hide resolved
reference/dom/domcomment.xml Outdated Show resolved Hide resolved
reference/dom/domdocument.xml Outdated Show resolved Hide resolved
reference/dom/domdocumenttype.xml Outdated Show resolved Hide resolved
reference/dom/domentity.xml Outdated Show resolved Hide resolved
reference/xsl/xsltprocessor/transformtoxml.xml Outdated Show resolved Hide resolved
@nielsdos
Copy link
Member Author

nielsdos commented Dec 2, 2024

Thanks for the initial review. I pushed fixup commits so they can be validated individually, I'll later rebase this to merge the fixups with the original commits.

@@ -7,7 +7,7 @@
<partintro>
<section xml:id="dom-entity.intro">
&reftitle.intro;
<xi:include xpointer="xmlns(db=http://docbook.org/ns/docbook) xpointer(id('domentity.intro')/db:simpara)">
<xi:include xpointer="xmlns(db=http://docbook.org/ns/docbook) xpointer(id('domentity.intro')/db:para)">
Copy link
Member

Choose a reason for hiding this comment

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

Why not just Xinclude the whole introduction section instead of just including the para? (same for some of the other pages)

@nielsdos nielsdos changed the title [WIP] 8.4: Document all new DOM classes [WIP] 8.4: Document all new DOM classes and methods Dec 2, 2024
Comment on lines +58 to +62
<para>
<simplelist>
<member><link xlink:href="&url.spec.dom.living.comment;">WHATWG specification of Comment</link></member>
</simplelist>
</para>
Copy link
Member

Choose a reason for hiding this comment

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

No need for the wrapping para tag here (haven't checked all of the files, but it might be in others?)

Comment on lines +11 to +14
<para>
The <classname>Dom\Text</classname> class inherits from
<classname>Dom\CharacterData</classname> and represents a text node.
</para>
Copy link
Member

Choose a reason for hiding this comment

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

New file, so <simpara>

Comment on lines +16 to +18
According to the DOM standard this requires a DTD which defines the
attribute ID to be of type ID. You need to validate your document by
passing <constant>LIBXML_DTDVALID</constant> to the parse method.
Copy link
Member

Choose a reason for hiding this comment

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

Personalization is used here.

Comment on lines +141 to +145
<simpara>
It is sometimes necessary to change
the qualified name and namespace <acronym>URI</acronym> together in one
step to not break any namespace rules.
</simpara>
Copy link
Member

Choose a reason for hiding this comment

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

The splitting of the line here is a bit strange

Comment on lines +25 to +27
<para>
Adds nodes before the character data.
</para>
Copy link
Member

Choose a reason for hiding this comment

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

Simpara

Comment on lines +24 to +26
<para>
Removes the character data.
</para>
Copy link
Member

Choose a reason for hiding this comment

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

simpara

Comment on lines +25 to +27
<para>
Replaces the character data with new nodes.
</para>
Copy link
Member

Choose a reason for hiding this comment

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

simpara

Comment on lines +46 to +49
<para>
&return.void;
</para>
</refsect1>
Copy link
Member

Choose a reason for hiding this comment

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

simpara

Comment on lines +22 to +33
<para>
<variablelist>
<varlistentry>
<term><parameter>selectors</parameter></term>
<listitem>
<simpara>
A string containing one or more CSS selectors.
</simpara>
</listitem>
</varlistentry>
</variablelist>
</para>
Copy link
Member

Choose a reason for hiding this comment

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

Para tag

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