Skip to content

PHP/TypeCasts: add XML documentation#2738

Open
rodrigoprimo wants to merge 3 commits into
WordPress:developfrom
rodrigoprimo:docs-type-casts
Open

PHP/TypeCasts: add XML documentation#2738
rodrigoprimo wants to merge 3 commits into
WordPress:developfrom
rodrigoprimo:docs-type-casts

Conversation

@rodrigoprimo

Copy link
Copy Markdown
Contributor

Description

This PR adds XML documentation for the WordPress.PHP.TypeCasts sniff.

The documentation is based on the work started by @brentwilson-clariio in #2591. I used the original commits and, in a separate commit, made the changes discussed in the review of #2591, plus I opted to replace the single bulleted standard block with three separate standard blocks, one per error/warning, and context indicating when each cast was deprecated and/or removed from PHP.

Regarding the (unset) cast, the sniff message suggests using unset() instead, so I kept that in the documentation from the original PR. But I'm not sure about it, as the behavior is different. As far as I can see, (unset) $a returns null while unset($a) unsets the variable $a.

I suggest squashing the commits before merging. I'm opening the PR without doing that to make it easier to tell my changes apart from the original changes.

Suggested changelog entry

N/A

Related issues/external references

Related to: #1722
Supersedes: #2591
Closes #2591

>
<standard>
<![CDATA[
The (float) cast must be used instead of the (double) and (real) casts. The (real) cast was deprecated in PHP 7.4 and removed in PHP 8.0. The (double) cast was deprecated in PHP 8.5.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Are the parentheses needed in this English string? The float cast... reads just as well as The (float) cast.... We don't need to demonstrate the syntax for how to cast when talking about casting.

Same applies for title attributes below.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your review, @GaryJones. I don't have a strong preference. I went ahead and pushed a new commit applying your suggestion.

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.

3 participants