Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not serialize already serialized values #515

Merged
merged 15 commits into from
May 15, 2014
Merged

Conversation

shadyvb
Copy link
Contributor

@shadyvb shadyvb commented May 7, 2014

Related to #477 , however will not fix existing data, yet

@shadyvb
Copy link
Contributor Author

shadyvb commented May 7, 2014

@westonruter Please review.

return $current_version;
}

/**
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shadyvb from the author_meta streammeta record shared by @krogsgard in #477, the underlying DB value is not double-serialized. Also, maybe_serialize() explicitly prevents double-serialization of strings: https://github.com/x-team/wordpress-develop/blob/cd356b6404b5a770cf69f81f80efd6eb046e6c57/src/wp-includes/functions.php#L354-L372

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am so wrong. It is getting double-serialized, as @shady pointed out: s:192:"a:5:{s:10:"user_. It's a serialized string containing the serialized array.

@lukecarbis
Copy link
Contributor

How widespread is issue #477? If this is only affecting a minority of users, would we consider ignoring existing data, so that we don't need an update routine?

We could provide the fix via support channels to those who need it. That way we're not taxing everyone's database with a big update routine, even if they might not need it.

Of course, if this is a widespread problem, that won't work.


In my experience, functions like set_time_limit and ini_set( 'max_execution_time' ) don't always work. They are often overridden by shared hosting providers. Also, we may still have to deal with memory_limit, if there's a large database.

$row->meta_value = maybe_unserialize( $meta_value );
}
// update_metadata will serialize it back
wp_stream_update_meta( $row->record_id, 'author_meta', $row->meta_value );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would adding another is_serialized check help to avoid updating every meta record in the db?

if ( is_serialized( $row->meta_value ) ) {
    $row->meta_value = @unserialize( $row->meta_value );
    if ( is_serialized( $row->meta_value ) ) {
        $row->meta_value = @unserialize( $meta_value );
    }
    // update_metadata will serialize it back
    wp_stream_update_meta( $row->record_id, 'author_meta', $row->meta_value );
}

Reference: wp-includes/functions.php:252-256

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lukecarbis Yea, that makes perfect sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 215e388

@frankiejarrett
Copy link
Contributor

@lukecarbis After considering this for a while, I do think an update routine is necessary because this is fixing malformed data caused by a code bug.

If this were a case of adding new record meta or something we would not do any routine, saying that records from now on would have the new data, but this is simply not the case here.

Furthermore, we're only going to alter existing records which contain the malformed data, and that makes sense to me.

/cc @shadyvb @westonruter

set_time_limit( 0 ); // This will probably take abit of time!
global $wpdb;
// Get all author_meta meta values, serialize them properly
$sql = "SELECT record_id, meta_value WHERE meta_key = 'author_meta'";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shadyvb we could speed this up a huge amount, and skip entries that aren't already malformed, by doing a LIKE query like so:

$sql  = "SELECT record_id, meta_value WHERE meta_key = 'author_meta' AND meta_value LIKE 's:%'";

Or a SUBSTRING(meta_value…) could be used.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@westonruter /five!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@westonruter Added in 4164e81, removed the redundant check ( since they'll all be double serialized )
/five!

@lukecarbis
Copy link
Contributor

Here's an example of the author_meta value in the wp_stream_meta table after running the database update:

s:180:"a:5:{s:10:"user_email";s:22:"luke.carbis@x-team.com";s:12:"display_name";s:5:"admin";s:10:"user_login";s:5:"admin";s:15:"user_role_label";s:13:"Administrator";s:9:"is_wp_cli";b:0;}";

It hasn't changed. This is on multisite.

global $wpdb;

// Get only the author_meta values that are double-serialized
$sql = "SELECT record_id, meta_value WHERE meta_key = 'author_meta' AND meta_value LIKE 's:%'";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shadyvb @westonruter Where in this statement is the table name wp_stream_meta indicated?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should be:

$prefix = WP_Stream_Install::$table_prefix;
$sql = "SELECT record_id, meta_value FROM {$prefix}stream_meta WHERE meta_key = 'author_meta' AND meta_value LIKE 's:%'";

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in a646c1b

@lukecarbis
Copy link
Contributor

Looks good. Worked for me on multisite. Tried upgrading from 1.4.3, too. /five

Author meta is being saved properly now.

frankiejarrett added a commit that referenced this pull request May 15, 2014
Do not serialize already serialized values
@frankiejarrett frankiejarrett merged commit 6c24f2d into develop May 15, 2014
@frankiejarrett frankiejarrett deleted the issue-477 branch May 15, 2014 03:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants