Default assignment of `use_ssl` user meta value causes unnecessary DB write.

[prettyboymp]

When the $meta in wp_insert_user() is processed using update_metadata() it first does a comparison against the current values in the database prior to running an update query. However, because the DB returns a string value ('1' or '0') while the meta_value is set to a boolean value by default the comparison always fails causing a write when not needed.

While this is relatively minor in itself, in multi-datacenter setups using hyperdb, this can cause a bit of overhead since all future reads to the usermeta table are then sent to the master database.

[prettyboymp]

Set $meta['use_ssl'] to string value.

Removing unnecessary writes to the DB for use_ssl user meta, with unit tests.

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

[rajinsharwar]

Added the unit tests for this.

Instructions to test if someone is willing to help:

1. Edit any user from the Admin Dashboard, and copy that user's ID.

2. Add the below lines just after the statement of function update_metadata( $meta_type, $object_id, ..)

	if ( $meta_key == 'use_ssl' ){
		error_log( 'Use SSL value in Meta: ' . ' ' . print_r(gettype($meta_value), true));
	}

3. Now, in your theme's functions.php, add the below line of code. Replace the USER_ID with the int value you copied in step 1.

error_log( 'Use SSL value in DB: ' . ' ' . gettype(get_user_meta( USER_ID, 'use_ssl', true )) );

4. Without making any changes to the Profile of that user, simply click "Update User"

5. In your Error Log file, you should see that both the datatypes in the error log outputs will be different. ❌

6. Now, apply the patch, and re-do the steps. You will find the Data types are the same. ✅

@Rajinsharwar Many thanks for the PR. Are you able to take a look at the feedback above from Adam?

Hi @johnbillion, I have updated the PR with @adamsilverstein feedback. :)

[rajinsharwar]

I believe this ticket doesn't need much testing, as it's a fairly small change itself. Removing 'needs-testing', and adding 'commit'.

[adamsilverstein]

Thanks for updating the PR with tests @rajinsharwar and for providing testing instructions. I'll try to get this tested and then committed for 6.6.

[rayhatron]

As part of WCEU contributor day, I'm picking up the testing of this ticket.

[rayhatron]

My notes from looking at this at WCEU contributor day:

wp_insert_user() receives the userdata from wp_update_user() ​here. wp_update_user() receives the userdata from edit_user() ​here. In edit_user(), we see use_ssl being set to either 0 or 1 as integers ​here.

Coming back to wp_insert_user, we see ​here $meta['use_ssl'] being set to either the integer 0 if $userdata['use_ssl'] is empty otherwise it sets it to the boolean equivalent of what $userdata['use_ssl'] would have.

In the case where we have the userdata being passed as explained earlier, it means $meta['use_ssl'] will either be true or false. So when update_user_meta() is called and it calls update_metadata(), we then see the old value already stored in DB (which is a string, ​see schema) being compared to the boolean in $meta['use_ssl'] ​here.

And those two values won't match the strict comparison since they're different values and data types. This then means $wpdb->update() is called ​here. This is the unnecessary db write that was mentioned in the description.

Now the patch did 2 things:

1. Set $meta['use_ssl'] to the string '0' instead of the integer 0 if $userdata['use_ssl'] is empty.
2. Force set $meta['use_ssl'] to the string '1' instead of attempting to retrieve it from the passed $userdata['use_ssl'].

We have a filter insert_user_meta and in its documentation $use_ssl is specified to either be an int or bool. This means changing $meta['use_ssl'] to be a string will potentially cause existing code in the community that hooks on that filter to break because the datatype has changed from what was originally specified.

So a different approach that we can take is at the point of comparing, if the meta key is use_ssl then we make sure the old value retrieved from the database is changed to the same datatype as the new value which will either be an int or a boolean.

We'll also need to make the unit tests verify that we indeed have 1 less db query.

[mukesh27]

Hi there!

We discussed this ticket in today's performance bug scrub. The findings in https://core.trac.wordpress.org/ticket/60299#comment:17 seem valid, so we need more eyes on it.

Punted to the next milestone. Feel free to add it to the milestone if it's ready for commit.

additional props: @joemcgill