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.

Sign up for GitHub

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

Jump to bottom

Do not serialize already serialized values #515

Merged
merged 15 commits into from
May 15, 2014
Merged

Do not serialize already serialized values #515

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
be84467
Do not serialize already serialized values
shadyvb May 7, 2014
e045e21
Let DB Adapter do the serializing, make sure associate arrays gets se…
shadyvb May 7, 2014
98d7b0d
Add update routine to fix incorrect author_meta values
shadyvb May 7, 2014
8416dcd
Alignment fix
shadyvb May 7, 2014
741fe44
Remove redundant version check
shadyvb May 7, 2014
c0ab326
Version bump to 1.4.5
shadyvb May 7, 2014
215e388
Do not update correct values in the new update routine
shadyvb May 8, 2014
4164e81
Target double-serialized values only in SQL, remove the redundant check
shadyvb May 10, 2014
9dfe61d
Code formatting
frankiejarrett May 10, 2014
98ed116
Trivial comment typo
frankiejarrett May 10, 2014
acb80aa
Removing set_time_limit
frankiejarrett May 10, 2014
a646c1b
Add missing table name from SQL statement query
frankiejarrett May 15, 2014
f62ede3
Being OCD about code format
frankiejarrett May 15, 2014
6620d7e
Whoops :-)
frankiejarrett May 15, 2014
bfc2170
Merge branch 'develop' into issue-477
frankiejarrett May 15, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions includes/db-updates.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,32 @@
<?php

/**
* Version 1.4.5
*
* Fix double serialized values in DB
*
* @param string $db_version Database version updating from
* @param string $current_version Database version updating to
*
* @return string $current_version if updated correctly
*/
function wp_stream_update_145( $db_version, $current_version ) {
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!

$rows = $wpdb->get_results( $sql );
foreach ( $rows as $row ) {
$row->meta_value = maybe_unserialize( $row->meta_value );
if ( is_serialized( $row->meta_value ) ) {
$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

}
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.

* Version 1.4.2
*
Expand Down
4 changes: 4 additions & 0 deletions includes/db.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,10 @@ public function insert( $recordarr ) {
}

foreach ( $recordarr['meta'] as $key => $vals ) {
// If associative array, serialize it, otherwise loop on its memebers
if ( is_array( $vals ) && 0 !== key( $vals ) ) {
$vals = array( $vals );
}
foreach ( (array) $vals as $val ) {
$val = maybe_serialize( $val );
$this->insert_meta( $record_id, $key, $val );
Expand Down
1 change: 1 addition & 0 deletions includes/install.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,7 @@ public static function db_update_versions() {
'1.3.1' /* @version 1.3.1 Update records of Installer to Theme Editor connector */,
'1.4.0' /* @version 1.4.0 Add the author_role column and prepare tables for multisite support */,
'1.4.2' /* @version 1.4.2 Patch to fix rare multisite upgrade not triggering */,
'1.4.5' /* @version 1.4.5 Patch to fix author_meta broken values */,
);

return apply_filters( 'wp_stream_db_update_versions', $db_update_versions );
Expand Down
3 changes: 0 additions & 3 deletions includes/log.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,9 +80,6 @@ public function log( $connector, $message, $args, $object_id, $contexts, $user_i
'agent' => WP_Stream_Author::get_current_agent(),
);
}
if ( isset( $args['author_meta'] ) ) {
$args['author_meta'] = maybe_serialize( $args['author_meta'] );
}

// Remove meta with null values from being logged
Copy link
Contributor

Choose a reason for hiding this comment

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

Without this serialization, I recall seeing that multiple stream meta records would get entered for each value in author_meta, instead of storing one single serialized array.

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 I fixed that in the PR i issued, now the insert method will serialize associative arrays properly. This is better as DB adapters should be the ones handling data storage, ie: in cloud no serializing should happen.

Copy link
Contributor

Choose a reason for hiding this comment

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

@shadyvb It looks like #497 is still a WIP. Could we make sure that the fix is in this PR so that we can get this bug fix out in the next release?

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 Sorry, not following. The PR i mentioned in my last comment is #515 ( this one ).

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, sorry. Ignore my last.

$meta = array_filter(
Expand Down
4 changes: 2 additions & 2 deletions stream.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
* Plugin Name: Stream
* Plugin URI: https://wp-stream.com/
* Description: Stream tracks logged-in user activity so you can monitor every change made on your WordPress site in beautifully organized detail. All activity is organized by context, action and IP address for easy filtering. Developers can extend Stream with custom connectors to log any kind of action.
* Version: 1.4.4
* Version: 1.4.5
* Author: X-Team
* Author URI: https://wp-stream.com/
* License: GPLv2+
Expand Down Expand Up @@ -36,7 +36,7 @@ class WP_Stream {
*
* @const string
*/
const VERSION = '1.4.4';
const VERSION = '1.4.5';

/**
* Hold Stream instance
Expand Down