SITE_ID_CURRENT_SITE broken in 6.3 or later

[scottculverhouse]

We set the constants

<?php
define('SITE_ID_CURRENT_SITE', 1);
define('BLOG_ID_CURRENT_SITE', 2);

This was working up until 6.3 where the following ​commit was added

I believe this is a bug for the following reasons

1. The typecast of SITE_ID_CURRENT_SITE to int
2. along with the strict comparison
3. and $this->id is always a string

[ironprogrammer]

Welcome to Trac, and thanks for the report, @scottculverhouse!

Could you provide additional information on the error or what isn't working as expected since the strict comparison update? ​Since version 4.6, the network ID has been an int, and enforced as such by the ​getter/​setter. In the updated code, $this->id shouldn't be a string (​compared with $this->blog_id which is still a string for backward compat).

I'm marking this ticket with reporter-feedback to signal that more information has been requested. Thanks!

[scottculverhouse]

Replying to ironprogrammer:

Could you provide additional information on the error or what isn't working as expected since the strict comparison update? ​Since version 4.6, the network ID has been an int, and enforced as such by the ​getter/​setter. In the updated code, $this->id shouldn't be a string (​compared with $this->blog_id which is still a string for backward compat).

Thanks for your prompt response, I think something funky is going on but can't put my finger on it. I've added some var_dump inside __get / __isset & __set to see what's going on. Also added a var_dump($this->id) which is returning string(1) "1". Please see attached diff.

The output of this debugging is as follows

string(5) "__get"
string(2) "id"
string(7) "__isset"
string(7) "blog_id"
string(5) "__get"
string(7) "blog_id"
string(30) "top of get_main_site_id method"
string(1) "1"
stopped

So to me it appears the var_dump($this->id); is not going through __get magic method. Like I said something "funky is going on", I'm not familiar with the WP source code so hope this helps.

[scottculverhouse]

Diff adding debugging to class-wp-network.php

[scottculverhouse]

Also to clarify the blog_id isn't getting set correctly when we use SITE_ID_CURRENT_SITE and BLOG_ID_CURRENT_SITE in our wp-config.php This previously worked with WP < 6.3

I'm on your slack if you want to DM for more info.

[spacedmonkey]

Mind taking a look @SergeyBiryukov ?

[SergeyBiryukov]

Replying to spacedmonkey:

Mind taking a look @SergeyBiryukov ?

Thanks for the ping! I saw the ticket, but I can't figure out how to reproduce this yet. $this->id should only ever be an int, as noted in comment:1::

​Since version 4.6, the network ID has been an int, and enforced as such by the ​getter/​setter. In the updated code, $this->id shouldn't be a string (​compared with $this->blog_id which is still a string for backward compat).

[SergeyBiryukov]

Replying to SergeyBiryukov:

I saw the ticket, but I can't figure out how to reproduce this yet. $this->id should only ever be an int

Hmm, I was able to reproduce it with class-wp-network.php.diff​.

When a new WP_Network instance is created, it sets the initial properties as retrieved from the database:

WP_Network::get_instance() {
    ...
    $_network = $wpdb->get_row( $wpdb->prepare( "SELECT * FROM {$wpdb->site} WHERE id = %d LIMIT 1", $network_id ) );
    ...
    return new WP_Network( $_network );
}

WP_Network::__construct( $network ) {
    ...
    foreach ( get_object_vars( $network ) as $key => $value ) {
        $this->$key = $value;
    }
    ...
}

This is where $this->id is set to a string, so it appears that [37870] / #37050 didn't quite work as expected: despite being documented as an int, $this->id is still a string in practice.

62035.diff​ fixes the issue in my testing, but this could use some unit tests.

Uses setters when constructing a WP_Network object. Ensures that WP_Network::id is stored internally as int, as follow-up to changeset 37870.

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

[ironprogrammer]

Replying to SergeyBiryukov:

62035.diff​ fixes the issue in my testing, but this could use some unit tests.

Thanks for the patch, @SergeyBiryukov! I've submitted ​PR #7345 which includes a unit test for the type of WP_Network::id. The other consideration might be to verify that WP_Network::blog_id is string (enforcing future backward compat), but I wanted to seek input before adding that.

[SergeyBiryukov]

In 59020:

Networks and Sites: Set WP_Network properties via setters upon creation.

This ensures that WP_Network::$id is stored internally as int, to match the documented type.

Follow-up to [37870].

Props ironprogrammer, scottculverhouse, spacedmonkey, SergeyBiryukov.
See #62035.

Thanks for the PR! Merged in r59020.

[SergeyBiryukov]

Replying to ironprogrammer:

I've submitted ​PR #7345 which includes a unit test for the type of WP_Network::id. The other consideration might be to verify that WP_Network::blog_id is string (enforcing future backward compat), but I wanted to seek input before adding that.

The test looks perfect, thank you!

Indeed, it would be great to verify the type of WP_Network::blog_id as well, keeping the ticket open for that for now.

[SergeyBiryukov]

In 59021:

Tests: Add tests to ensure that the WP_Network::$blog_id property is a string.

Follow-up to [34097], [36340], [37657], [37870], [37871], [59020].

Fixes #62035.

[scottculverhouse]

Great work guys and thanks again for the prompt response / turnaround.

I guess this issue is how PHP is tokenising the code and doesn't process $this->$key = $value the same as $this->id = $value therefore not passing to the __set().

[SergeyBiryukov]

Replying to scottculverhouse:

I guess this issue is how PHP is tokenising the code and doesn't process $this->$key = $value the same as $this->id = $value therefore not passing to the __set().

Thanks for the ticket! Right, in my testing it also appears that __get() is only called when retrieving the property value outside of the class, but not within methods of the same class.

For example,

class A {

	private $test = '1';

	public function __get( $key ) {
		switch ( $key ) {
			case 'test':
				return (int) $this->test;
		}

		return null;
	}

	public function get_test_value() {
		return $this->test;
	}
}

$a = new A;
var_dump( $a->test );
var_dump( $a->get_test_value() );

outputs:

int(1)
string(1) "1"

which explains the previous discrepancy here.

[scottculverhouse]

Replying to SergeyBiryukov:

Right, in my testing it also appears that __get() is only called when retrieving the property value outside of the class, but not within methods of the same class.

Ah ok that worth noting