WPDBTrait::is_wpdb_method_call(): improve docblock description#2719
Conversation
- Clarified that the method supports static method calls as well. - Replaced incomplete description with a list of the three properties that may be automatically set: `$methodPtr`, `$i`, and `$end`. - Added clarification that `$methodPtr` and `$i` may be set even when the method returns false (e.g., for property access like `$wpdb->show_errors`). - Updated `$stackPtr` parameter description to mention it can be a "wpdb class name token" as well. - Made the `$end` description more precise: it points to the comma after the first parameter, or to one past the last token of the first parameter if there is no comma.
jrfnl
left a comment
jrfnl
left a comment
There was a problem hiding this comment.
@rodrigoprimo I 100% agree this is one of the most confusing sniff design parts within WPCS. I appreciate you taking the time to improve these docs as this is always a hard part to get your head around (again) when you haven't looked at this code for a while.
I've just left some small notes inline as suggestions to straighten it up even more.
Co-authored-by: Juliette <663378+jrfnl@users.noreply.github.com>
rodrigoprimo
force-pushed
the
update-is-wpdb-method-call-docblock
branch
from
7256065 to
074494e
Compare
|
Thanks for your review, @jrfnl! I have implemented all the suggestions you made, and this PR is ready for another review. |
GaryJones
left a comment
GaryJones
left a comment
There was a problem hiding this comment.
Couple of nit-picks but not blockers.
| * Note: Static calls to wpdb methods trigger a deprecation notice in PHP 7.0+ | ||
| * and result in a fatal error in PHP 8.0+ as wpdb methods are not declared static, | ||
| * but that's not our concern. |
There was a problem hiding this comment.
The reality is a bit more nuanced than this. The E_DEPRECATED for Class::method() when called outside an instance context was removed in PHP 8, and it becomes an Error only when $this is referenced.
Also, the blanket claim that “wpdb methods are not declared static” isn’t quite right — wpdb::esc_like() is documented as callable both ways historically, and people in the wild do it. For a docblock that is explicitly disclaiming “but that’s not our concern”, we can afford to just say “Static calls on non-static wpdb methods are problematic at runtime, but this helper still matches them so sniffs can flag them in source.”
There was a problem hiding this comment.
it becomes an Error only when $this is referenced
Correct me if I'm missing something, but I don't think the above is right. Static calls to non-static methods when called outside an instance context are an error in PHP 8+, whether $this is referenced or not:
Also, the blanket claim that “wpdb methods are not declared static” isn’t quite right — wpdb::esc_like() is documented as callable both ways historically, and people in the wild do it.
My understanding is that, even though some wpdb methods have historically been called statically, that doesn't change the fact that no method in that class is declared as static. That said, from my perspective, either the original version of the note or your suggestion works for me. Happy to implement your version if you prefer.
There was a problem hiding this comment.
I think Gary's suggestion "Static ... source" is a good one. Let's go for that.
There was a problem hiding this comment.
I just pushed a new commit applying this suggestion.
* upstream/develop: (223 commits) ContextHelper::is_in_type_test(): add tests (WordPress#2721) WPDBTrait::is_wpdb_method_call(): improve docblock description (WordPress#2719) ContextHelper::is_in_array_comparison(): add basic tests (WordPress#2726) ContextHelper::is_in_isset_or_empty(): add basic tests (WordPress#2725) WP/I18n: add tests for namespaced names WP/GetMetaSingle: add tests for namespaced names WP/DeprecatedParameterValues: add tests for namespaced names WP/DeprecatedParameterValues: move syntax error test to its own file WP/DeprecatedParameterValues: rename test case file WP/DeprecatedParameters: add tests for namespaced names FormattingFunctionsHelper::is_formatting_function(): add tests (WordPress#2713) UnslashingFunctionsHelper::is_unslashing_function(): add tests (WordPress#2715) GH Actions: Bump crate-ci/typos from 1.44.0 to 1.45.0 in the action-runners group (WordPress#2717) GH Actions: Bump codecov/codecov-action from 5.5.3 to 6.0.0 GH Actions: Bump ramsey/composer-install from 3.1.1 to 4.0.0 GH Actions: Bump the action-runners group with 2 updates Security/ValidatedSanitizedInput: add tests for namespaced names Apply suggestion from PR review WP/EnqueuedResourceParameters: add tests for namespaced names (WordPress#2675) GH Actions: Bump crate-ci/typos in the action-runners group ...
3 participants
Description
While working on an upcoming PR that adds tests to
WPDBTrait::is_wpdb_method_call(), I struggled to understand parts of the docblock for this method. This PR is my attempt to improve it and make it more precise.Here is a list of what I changed:
$methodPtr,$i, and$end.$methodPtrand$imay be set even when the method returns false (e.g., for property access like$wpdb->show_errors).$stackPtrparameter description to mention it can be a "wpdb class name token" as well.$enddescription more precise: it points to the comma after the first parameter, or to one past the last token of the first parameter if there is no comma.Note: I'm not sure if the clarification that
$methodPtrand$imay be set even when the method returns false should be mentioned or not, but I opted to keep it for now as the original version did mention this information for the$iproperty.Suggested changelog entry
N/A