Coding standards : Use Strict Comparison except Loose Comparison wp-includes/class-wp-hook.php

[rockonshajib]

It's help for coding standardization when get the Integers data type then use Strict Comparison except Loose Comparison

Trac ticket:

[mukesh27]

Hi there! Welcome to trac!

Thanks for the ticket and PR.

Moving to 6.2 milestone.

[audrasjb]

Removing commit for now, as there is still ​a remark to address in the PR.

This PR updates ​https://github.com/WordPress/wordpress-develop/pull/3690 with an int cast for BC.

Trac ticket: https://core.trac.wordpress.org/ticket/57213

[costdev]

​PR 3878 implements the feedback. Adding for commit consideration.

[johnbillion]

Is there a performance impact to this change? I doubt it but I'd like to see some numbers, possibly with PHPBench similar to #53218. We're allowed to nitpick and micro-optimise apply_filters() and do_action() because they're used so frequently :)

[mukesh27]

Thanks @rockonshajib for the ticket.

This ticket was discussed in the recent bug scrub.

Can someone from the testing team (@adeltahri or @robinwpdeveloper) and performance team (@flixos90) take a look and share the feedback?

Props to @costdev

[adeltahri]

Test Report:

There were no PHP errors on the frontend or backend of the general test.

Patch tested: 3878

Environment

• OS: macOS 13.0.1
• Web Server: Nginx
• PHP: 7.4.33
• WordPress: 6.2-alpha-54642
• Browser: Chrome 109
• Theme: Twenty Twenty-Two
• Active Plugins:
  • Gutenberg

PHPUnit Test

I have added a test unit to applyFilters.php:

        public function test_apply_filters_with_integer_argument() {
		$a             = new MockAction();
		$callback      = array( $a, 'filter' );
		$hook          = new WP_Hook();
		$hook_name     = __FUNCTION__;
		$priority      = 1;
		$accepted_args = 2;
		$arg           = 0;
	
		$hook->add_filter( $hook_name, $callback, $priority, $accepted_args );
	
		$returned = $hook->apply_filters( $arg, array( $arg ) );
	
		$this->assertSame( $returned, $arg );
	}
		
	public function test_apply_filters_with_string_argument() {
		$a             = new MockAction();
		$callback      = array( $a, 'filter' );
		$hook          = new WP_Hook();
		$hook_name     = __FUNCTION__;
		$priority      = 1;
		$accepted_args = 2;
		$arg           = '0';
	
		$hook->add_filter( $hook_name, $callback, $priority, $accepted_args );
	
		$returned = $hook->apply_filters( $arg, array( $arg ) );
	
		$this->assertSame( $returned, $arg );
	}

• Test pass.

[davidbaumwald]

With 6.2 Beta 1 scheduled to be released tomorrow(2023-02-06) and @johnbillion's request for some performance data on this change, this ticket is being moved to Future Release.

If any committer wishes to assume ownership of this ticket during a specific cycle, or if the remaining issues can be resolved in time, feel free to update the milestone accordingly.

[SirLouen]

Duplicate of #58831.

Funny to see that here people were discussing on a ton of things, including the performance implications and meanwhile this was merged under the hood and no one even noticed [56511] (and not even with static casting as proposed by @mukesh27), and obviously not with unit-tests.

By the way, @johnbillion, just as a curiosity, on the paper a type cast with a === can't have an impact compared with a regular comparison. For all scenarios where type conversion happens the complexity will be the same, except for strings, where casting from string to int (in case it's ever cast) will have a constant complexity, while a classic comparison will need to ultimately do the same. So I doubt that performance testing was required at all, and I doubt that these changes were required at all (other than aesthetic purposes).

Anyway, time to close this as it has been already fixed, and I wonder if there is a risk that $the_['accepted_args'] could be a noninteger (which shouldn't).

For more info I've tried this simple code

function hide_admin_bar() {
    return false;
}

add_filter('show_admin_bar', 'hide_admin_bar', 10, '0');

Just to showcase a string accepted_args, which theoretically should fail on the 0 === comparison, and it works! At some point, it appears that it's converted to 0 int… So, for some reason, this always worked for all cases with the simplest of the solutions :)