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

Update ext/dom names after policy change #14171

Merged
merged 4 commits into from May 9, 2024
Merged

Conversation

nielsdos
Copy link
Member

@nielsdos nielsdos commented May 7, 2024

Per https://wiki.php.net/rfc/class-naming-acronyms, change the acronyms in ext/dom's new PHP 8.4 classes.

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.

Thank you for doing the hard work!

@@ -889,7 +889,7 @@ public function prepend(...$nodes): void {}
public function replaceChildren(...$nodes): void {}
}

/** @alias DOM\DOMException */
/** @alias Dom\DOMException */
Copy link
Member

@kocsismate kocsismate May 8, 2024

Choose a reason for hiding this comment

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

Is it intentional that the class name follows the discouraged pattern of uppercased abbreviations? Is it for internal consistency with the existing pattern? Personally, I'd vote for going full steam PascalCase. This applies for other abbreviations like HTTP, XML, etc.

Copy link
Member Author

Choose a reason for hiding this comment

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

The official name is DOMException: https://webidl.spec.whatwg.org/#idl-DOMException and so this was left alone.

Copy link
Member

Choose a reason for hiding this comment

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

Ah ok, I see. Then I guess the same applies to all the other names which were left intact.

@@ -1252,7 +1252,7 @@ public function count(): int {}
public function getIterator(): \Iterator {}
}

class DTDNamedNodeMap implements \IteratorAggregate, \Countable
class DtdNamedNodeMap implements \IteratorAggregate, \Countable
Copy link
Member

Choose a reason for hiding this comment

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

I saw a few properties using discouraged names, like namespaceURI. Can these be renamed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Same thing here: namespaceURI is defined per spec with that capitalization.

@nielsdos nielsdos merged commit 6e7adb3 into php:master May 9, 2024
9 of 10 checks passed
xabbuh added a commit to symfony/symfony that referenced this pull request May 15, 2024
… classes (xabbuh)

This PR was merged into the 7.1 branch.

Discussion
----------

[VarDumper]  adapt namespace changes for new DOM extension classes

| Q             | A
| ------------- | ---
| Branch?       | 7.1
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Issues        |
| License       | MIT

related to php/php-src#14171

Commits
-------

7e25a46 adapt namespace changes for new DOM extension classes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants