WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

#16597 closed defect (bug) (fixed)

Bug in the maybe_serialize() function - it serializing also serailize objects.

Reported by: maorb Owned by: nacin
Milestone: 3.3 Priority: normal
Severity: major Version: 3.0.5
Component: Formatting Keywords: has-patch
Focuses: Cc:

Description

Hi,

I have discovered this bug by accident, when trying to import posts using the csv importer plugin.
One of my lines contained a serialized array to be saved.
The serialized data was ok, but saved to the db with a prefix of s:196:" and suffix of ";.
The reason is that the add_post_meta() function uses the add_metadata() , which uses the maybe_serialize() function to determine if need to serialize or not.
The thing is that it is serializing anyway!

Look at wp-includes/functions.php, lines 1005-1013 -

function maybe_serialize( $data ) {
	if ( is_array( $data ) || is_object( $data ) )
		return serialize( $data );

	if ( is_serialized( $data ) )
		return serialize( $data );

	return $data;
}

It says to serialize also if the data is_serialized....

if ( is_serialized( $data ) )
		return serialize( $data );

The correct should be -

function maybe_serialize( $data ) {
	if ( is_array( $data ) || is_object( $data ) )
		return serialize( $data );

	if ( is_serialized( $data ) )
		return $data ;

	return $data;
}

Otherwise, the data is being serialized twice and then WP can't show the real unserialized data ..

After I changed it and rechecked passing serialized data to the add_meta_data() function - it all went ok.

This should be fixed in core.

Here is a reference also in the forums
http://wordpress.org/support/topic/plugin-csv-importer-escaping-quotation-marks

Thanks,
Maor

Attachments (1)

16597.patch (484 bytes) - added by SergeyBiryukov 3 years ago.

Download all attachments as: .zip

Change History (11)

comment:1 greuben3 years ago

Dupe of #15057
see [14074]

comment:2 nacin3 years ago

  • Owner set to nacin
  • Status changed from new to accepted

Double-serialize is intentional.

In 3.2 I will add a quick comment there letting people know it is intentional and I'll cross-reference r14074. Let's just leave this open for a few days before I forget.

comment:3 maorb3 years ago

Why it should be double serialized?
This means that if one will pass serialized data to the add_metadata() function, it will never be parsed ok, since it will add the second serialization.
This causes breaking of the data when pulling out from the db..
If double serialize is a must, maybe should have addition to the get_metadata() to handle it better?

Thanks

comment:4 nacin3 years ago

If you serialize something then pass it to add_metadata(), then you should be expected to unserialize it when you fetch it from get_metadata(). Whatever goes in, comes out.

In turn, the functions already serialize -- that's the point of maybe_serialize(), it gets used in the transient, option, site option, and metadata APIs. Just pass an array or object and it'll take care of it. In a nutshell, you're thinking too hard. :-)

When this last came up, I wrote a blog post on it: http://andrewnacin.com/2010/04/18/wordpress-serializing-data/.

comment:5 maorb3 years ago

I agree with you that what goes in should come out.
Anyway, there are situations of different methods to insert data and thus it sometimes will go as an array and sometimes as a serialized data.
For this example I encountered - there are some meta keys that are indeed being passed as an array, where the end user just saves data from the admin panel.
But, in this case for the same site, there is a way of importing csv file to the database.
All these custom fields that are being saved into one meta_key and the meta_value is the serialized array, cannot be passed as an object when it comes to a csv file, since you cannot pass an array in a csv, thus when data comes from csv it must be serialized first.

I thought that the maybe_serialize() should check if the data passes it alreay serialized and not to serialize it again..

comment:6 hakre3 years ago

If you import a CSV, the import script should make use of the is_serailized() function on each field. In case this function returns true, unserialize it. Then give only unserialized values to the add_post_meta()/add_metadata() functions and you should be fine.

Edit: Or use maybe_unserialize() on CSV data fields which might be more straight forward (and does what I wrote).

Last edited 3 years ago by hakre (previous) (diff)

comment:7 SergeyBiryukov3 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to duplicate
  • Status changed from accepted to closed

SergeyBiryukov3 years ago

comment:8 SergeyBiryukov3 years ago

  • Keywords has-patch added

Added patch with a comment, as per nacin's suggestion (just in case).

comment:9 nacin3 years ago

  • Milestone set to 3.3
  • Resolution duplicate deleted
  • Status changed from closed to reopened

comment:10 markjaquith3 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In [18807]:

Add note about why double-serialization is done. props SergeyBiryukov. fixes #16597

Note: See TracTickets for help on using tickets.