Skip to content

[CLEANUP] Make AbstractHtmlProcessor::domDocument private#1621

Merged
JakeQZ merged 1 commit intomainfrom
cleanup/non-nullable-dom-document
May 7, 2026
Merged

[CLEANUP] Make AbstractHtmlProcessor::domDocument private#1621
JakeQZ merged 1 commit intomainfrom
cleanup/non-nullable-dom-document

Conversation

@oliverklee
Copy link
Copy Markdown
Collaborator

Also make it non-nullable as it will never be read before initialization.

Access to the property already is guarded by the getDomDocument() accessor, which we'll need to keep as we access it from the outside.

We cannot guard the getter's code with an assert as PHPStan would complain about a useless assert there. So rely on PHP throwing an error if the getter is called before the property is initialized.

Also improve some related comments.

Also make it non-nullable as it will never be read before
initialization.

Access to the property already is guarded by the `getDomDocument()`
accessor, which we'll need to keep as we access it from the
outside.

We cannot guard the getter's code with an `assert` as PHPStan
would complain about a useless assert there. So rely on PHP
throwing an error if the getter is called before the property
is initialized.

Also improve some related comments.
@coveralls
Copy link
Copy Markdown

Coverage Status

coverage: 97.669% (-0.003%) from 97.672% — cleanup/non-nullable-dom-document into main

Comment on lines -457 to -458
*
* @throws \UnexpectedValueException
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

getHtmlElement() currently throws this exception if there is no <html> element, which would only be the case if loadHTML() failed. But the documentation doesn't indicate under which circumstances it would fail, merely stating "malformed HTML should load successfully".

So perhaps we can assume that there will always be an <html> element, and change getHtmlElement() to assert() so as a separate PR. (Also, calling methods aren't currently declared with this @throws annotation.)

@JakeQZ JakeQZ merged commit adfbf08 into main May 7, 2026
33 checks passed
@JakeQZ JakeQZ deleted the cleanup/non-nullable-dom-document branch May 7, 2026 15:57
@JakeQZ
Copy link
Copy Markdown
Collaborator

JakeQZ commented May 7, 2026

We cannot guard the getter's code with an assert as PHPStan would complain about a useless assert there.

We could set treatPhpDocTypesAsCertain to false in phpstan.neon to allow us to assert() complete construction. WDYT?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants