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

HTML API: Add PHP type annotations #6753

Closed
wants to merge 36 commits into from

Conversation

sirreal
Copy link
Member

@sirreal sirreal commented Jun 7, 2024

Trac ticket: Core-61399

Add type annotations in the HTML API where possible. In this PR I've avoided adding annotations where the supported annotations are worse that PHPDoc annotations.

For example, I've avoided using the array type where the @param or @return annotations are more expressive, or a mixed type instead of a type union.

I've extracted some bugfixes that arose as part of this work to #6754, that's a smaller patch that can be landed independently.

Includes #7031.


This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

Copy link

github-actions bot commented Jun 7, 2024

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

  • The Plugin and Theme Directories cannot be accessed within Playground.
  • All changes will be lost when closing a tab with a Playground instance.
  • All changes will be lost when refreshing the page.
  • A fresh instance is created each time the link below is clicked.
  • Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
    it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

@sirreal sirreal marked this pull request as ready for review June 7, 2024 11:00
Copy link

github-actions bot commented Jun 7, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props jonsurrell, dmsnell.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@sirreal sirreal force-pushed the html-api/annotate-types branch 2 times, most recently from a89ccd3 to c4d2ed6 Compare June 13, 2024 14:39
@sirreal
Copy link
Member Author

sirreal commented Jul 15, 2024

I've removed most of the type annotations from the parameters of public APIs. I did this to address concerns about the potential to break sites due to type errors from plugins that are not actionable in any way for the sites. I've maintained the type annotations on the private APIs and internal classes.

@dmsnell What do you think about the low-level API like the token map? Should I remove types in the same way there as well?

@@ -643,7 +641,6 @@ public function next_token() {
return true;
}


Copy link
Member

Choose a reason for hiding this comment

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

I won't ask to remove this, but it was intentionally added to give a little bit more separation between the next_ methods and the query/read methods

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, the two empty lines seemed out of place. I'm surprised PHPCS didn't complain, in other places that's a lint error.

I can restore the line or we could add an organizational comment, thoughts?

@sirreal sirreal requested a review from dmsnell July 18, 2024 10:30
@dmsnell
Copy link
Member

dmsnell commented Jul 18, 2024

@sirreal I think all of this as is is probably fine, but I want to run this against the Gutenberg repository first to ensure that nothing basic is broken, since we're introducing new intentional-breaking behaviors.

@dmsnell
Copy link
Member

dmsnell commented Jul 18, 2024

Backport contains these anticipated updates in the following Gutenberg PR.

WordPress/gutenberg#63723

Copy link
Member

@dmsnell dmsnell left a comment

Choose a reason for hiding this comment

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

The backport into Gutenberg did not immediately break everything, so I will merge this into Core.

pento pushed a commit that referenced this pull request Jul 19, 2024
This patch adds type annotations to internal and private methods of the HTML
API and the supporting WP_Token_Map. Annotations have not been added to the
public interfaces where it would likely crash a site if called wrong.

These annotations should help avoid unnecessary type-related bugs (as have
been uncovered in earlier work adding such annotations) and provide additional
guidance to developers when interacting with these classes in an IDE.

Developed in #6753
Discussed in https://core.trac.wordpress.org/ticket/61399

Props dmsnell, jonsurrell.
See #61399.


git-svn-id: https://develop.svn.wordpress.org/trunk@58769 602fd350-edb4-49c9-b593-d223f7449a82
@dmsnell
Copy link
Member

dmsnell commented Jul 19, 2024

Merged in [58769]
dmsnell@0b17989

@dmsnell dmsnell closed this Jul 19, 2024
@dmsnell dmsnell deleted the html-api/annotate-types branch July 19, 2024 23:45
markjaquith pushed a commit to markjaquith/WordPress that referenced this pull request Jul 19, 2024
This patch adds type annotations to internal and private methods of the HTML
API and the supporting WP_Token_Map. Annotations have not been added to the
public interfaces where it would likely crash a site if called wrong.

These annotations should help avoid unnecessary type-related bugs (as have
been uncovered in earlier work adding such annotations) and provide additional
guidance to developers when interacting with these classes in an IDE.

Developed in WordPress/wordpress-develop#6753
Discussed in https://core.trac.wordpress.org/ticket/61399

Props dmsnell, jonsurrell.
See #61399.

Built from https://develop.svn.wordpress.org/trunk@58769


git-svn-id: http://core.svn.wordpress.org/trunk@58171 1a063a9b-81f0-0310-95a4-ce76da25c4cd
github-actions bot pushed a commit to gilzow/wordpress-performance that referenced this pull request Jul 19, 2024
This patch adds type annotations to internal and private methods of the HTML
API and the supporting WP_Token_Map. Annotations have not been added to the
public interfaces where it would likely crash a site if called wrong.

These annotations should help avoid unnecessary type-related bugs (as have
been uncovered in earlier work adding such annotations) and provide additional
guidance to developers when interacting with these classes in an IDE.

Developed in WordPress/wordpress-develop#6753
Discussed in https://core.trac.wordpress.org/ticket/61399

Props dmsnell, jonsurrell.
See #61399.

Built from https://develop.svn.wordpress.org/trunk@58769


git-svn-id: https://core.svn.wordpress.org/trunk@58171 1a063a9b-81f0-0310-95a4-ce76da25c4cd
@TobiasBg
Copy link

@dmsnell @sirreal:
I see 2-3 $case_sensitivity parameters (2nd/3rd parameters of a function) that could also get a string annotation?
Or is this similar to $termination_list not getting array as the PHPDoc has the more specific string[] (which is intentional from what I understand from the PR description)?
I would actually vote for adding these as well, as the annotations not only serve documentation purposes but also prevent wrong parameter types from being passed to the function.

@dmsnell
Copy link
Member

dmsnell commented Jul 20, 2024

@TobiasBg some of the annotations were removed to prevent site crashes when improperly called. I think we are still gathering experience knowing how to handle this scenario. the PHDDoc comment adds the types, right? so IDE's should still type-check, autocomplete, and auto-document the parameters.

I've seen plenty of well-intentioned efforts to add type annotations only to realize that something was a mistake and now WordPress crashes because the types were wrong. I'm sensitive to avoid doing this out of over-eagerness.

@TobiasBg
Copy link

@dmsnell: Yes, PHPDocs (correctly) adds the types for syntax highlighting, autocomplete, documentation, use in static analysis, etc. But these are pretty much "optional". Only the annotations let PHP raise an error message if a parameter has the wrong type.

And of course that's why type annotations should be added with care (and e.g. prevent with custom type checks in the code) to prevent crashes! Thus, excluding public-facing functions from this totally makes sense!

I was more referring to "inconsistencies": E.g. in [trunk/src/wp-includes/class-wp-token-map.php](https://core.trac.wordpress.org/changeset/58769/trunk/src/wp-includes/class-wp-token-map.php):
In the line

public function contains( string $word, string $case_sensitivity = 'case-sensitive' ): bool {

both parameters (including $case_sensitivity) got an annotation. But a few lines below, in the line

public function read_token( string $text, int $offset = 0, &$matched_token_byte_length = null, $case_sensitivity = 'case-sensitive' ): ?string {

only the first parameter got an annotation while the same $case_sensitivity parameter did not (but in both cases, it's PHPdoc'ed as string).
(I'm fine with the call-by-reference parameter not getting one, as I don't know what consequences that might have.)

Similarly, in trunk/src/wp-includes/html-api/class-wp-html-open-elements.php, there's the line

public function has_element_in_specific_scope( string $tag_name, $termination_list ): bool {

where only the first parameter got an annotation.

@dmsnell
Copy link
Member

dmsnell commented Jul 20, 2024

@TobiasBg happy to review a patch!

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.

3 participants