Opened 16 months ago
Last modified 14 months ago
#58723 new defect (bug)
Remove deprecated parameter tests in comments_link()
Reported by: | Cybr | Owned by: | |
---|---|---|---|
Milestone: | Awaiting Review | Priority: | normal |
Severity: | normal | Version: | |
Component: | Comments | Keywords: | reporter-feedback has-patch |
Focuses: | performance | Cc: |
Description
In comments_link()
, two unused parameters are checked if they're not empty. If they are, only a deprecation warning is shown for a WordPress version of the distant past.
I suggest removing the parameters altogether.
If we _must_ keep these deprecation warnings, at least use if ( $deprecated )
instead of if ( ! empty( $deprecated ) )
for the former is about 2.7x faster while achieving the same.
Change History (6)
#3
@
14 months ago
In #58724 it was suggested that this ticket be expanded to cover additional functions that could be optimized in the same way. Per this comment:
Yes. After realizing there was a pattern, I found a total of 38 instances (including this one) in Trunk using regex:
function.*?deprecated[0-9]{0,2} = .{1,10} \)
Personally, I don't think that removing these params entirely are that beneficial. Avoiding the empty
check on deprecated functions like this is a valid suggestion. Thanks for providing some benchmarking numbers, @Cybr.
In practice, I suspect this type of change would have very little measurable impact since these functions are not called all that often.
Looking over the list of functions returned in your regex, we seem to be pretty inconsistent about how we're handling deprecated parameters, with only some calling _deprecated_argument
. A good task would be to go through all of these functions and see if we can standardize how these are handled, with any performance optimizations in mind.
This ticket was mentioned in PR #5085 on WordPress/wordpress-develop by @tabrisrp.
14 months ago
#5
- Keywords has-patch added
Improve deprecated parameters handling, by removing usage of empty()
to improve performance, adding calls to _deprecated_argument()
where it was missing, and updating the inline doc as much as possible where it was missing.
Trac ticket: https://core.trac.wordpress.org/ticket/58723
#6
@
14 months ago
To clarify: if ( empty( $var ) )
is OK, if ( ! empty( $var ) )
isn't unless the variable might not exist. If the variable certainly exists, use a simple if ( $var )
check.
Due to this misunderstanding, the attached PR breaks many things by flipping the boolean.
Nevertheless, I assume we want to keep the "final argument that is deprecated" for forward compatibility.
Then, yes, adding deprecation notices for their use would be helpful.
Please note that it might be better to deprecate these functions altogether and rewrite them for improved performance.
I don't think there's a net benefit of having developer-focused heavy "if"-jumps in functions that can be used thousands of times during production runtime.
Offenders include convert_chars()
, is_email()
, switch_to_blog()
, and get_site_option()
.
There's also something to be said about keeping deprecations from back to WordPress 0.71 alive (20 years ago!), like we do in convert_chars()
. If we can throw away backward compatibility for half the code in 5.0, we might as well consider doing that for earlier versions.
Here's the test proving
! empty()
is slower than a plain boolean check: https://3v4l.org/YvKIn.