Make WordPress Core

Opened 13 months ago

Last modified 11 months ago

#58723 new defect (bug)

Remove deprecated parameter tests in comments_link()

Reported by: cybr's profile Cybr Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: Comments Keywords: reporter-feedback has-patch
Focuses: performance Cc:


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)

#1 @Cybr
13 months ago

Here's the test proving ! empty() is slower than a plain boolean check:

#2 @joemcgill
11 months ago

#58724 was marked as a duplicate.

#3 @joemcgill
11 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.

#4 @mukesh27
11 months ago

  • Keywords reporter-feedback added

This ticket was mentioned in PR #5085 on WordPress/wordpress-develop by @tabrisrp.

11 months ago

  • 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:

#6 @Cybr
11 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.

Note: See TracTickets for help on using tickets.