WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 14 months ago

Last modified 14 months ago

#12930 closed defect (bug) (fixed)

Unserialize error?

Reported by: ipstenu Owned by:
Milestone: 3.0 Priority: high
Severity: major Version:
Component: General Keywords:
Focuses: Cc:

Description

I upgraded to 3.0 beta for testing and found that a couple plugins are giving me an odd error:

Warning: unserialize() expects parameter 1 to be string, array given in ...

This happens in two different plugins (JournalPress and Google Analyticator). WordPress works with the error, and JournalPress even posts to my blog AND my LJ.

Change History (18)

comment:1 ocean904 years ago

  • Milestone Unassigned deleted
  • Resolution set to invalid
  • Status changed from new to closed

unserialize() is a PHP function.

unserialize() takes a single serialized variable(string) and converts it back into a PHP value. 

Please inform the plugin developers, its not a WordPress issue.

comment:2 nacin4 years ago

It might be from core however, i.e. something that changed from 2.9 to 3.0, but we need to know the line number and more information on how it gets triggered.

Please re-open if you have steps to reproduce.

comment:3 ipstenu4 years ago

If it had only been ONE plugin, I'd have agreed ocean90, but once it comes up twice, I get suspicious. And it ONLY happened once I upgraded to 3.0 (worked FINE in 2.9.x).

I can reproduce the LJ plugin every time I try and save ANY post while it's activated. And the Google Analyticator one just by trying to view the dashboard 'plugin'.

The problem is I'm the user, not the dev. I'll ping them for support and ask them to come here.

comment:4 nacin4 years ago

We can't do anything unless we know more about the error. At the very least, line numbers?

comment:5 sivel4 years ago

If I am correct in my assumption it is something with get_post_meta. I had to update a plugin for WP 3.0 to use maybe_unserialize instead of unserialize because get_post_meta is unserializing the data it returns.

comment:6 ipstenu4 years ago

I went to some examples for you:

Warning: unserialize() expects parameter 1 to be string, array given in /home/ipstenu/public_html/blog/wp-content/plugins/journalpress/jpconfig.php  on line 696

Warning: unserialize() expects parameter 1 to be string, array given in /home/ipstenu/public_html/blog/wp-content/plugins/journalpress/jpconfig.php on line 703

Warning: unserialize() expects parameter 1 to be string, array given in /home/ipstenu/public_html/blog/wp-content/plugins/journalpress/jpconfig.php on line 709

And I saw sivel's comment. Changing unserialize to maybe_unserialize fixes everything.

Interesting, that. I'm passing it on to the devs.

comment:7 nacin4 years ago

*sigh*. maybe_unserialize is only half of it. That plugin is trying to unserialize a result from get_post_meta(). The meta, options and transients APIs all handle that transparently. There should be no unserialize call there at all.

comment:8 nacin4 years ago

  • Milestone set to 3.0
  • Resolution invalid deleted
  • Status changed from closed to reopened

After some investigating (thanks sivel), looks like there may be something going on here. I want to look at this some more.

comment:9 lupinek74 years ago

  • Keywords unserialize added
  • Priority changed from normal to high
  • Severity changed from normal to major

The same problem with get_option() – unexpectedly returns unserialized data in 3.0beta-1 (in 2.9 everything worked just fine).

comment:10 nacin4 years ago

We're looking into this. It comes out of a change to maybe_serialize() in r13673, which for a long while serialized already serialized data, and now no longer does. We'll probably revert this.

Bottom line is, plugin developers are using the _option, _meta, and _transient APIs horribly incorrectly for this change to cause problems.

These APIs already serialize and unserialize objects and arrays transparently. Thus, this works:

update_option( 'my_plugin_options', array( 'blah', 'foo', bar' ) );
get_option( 'my_update_plugins' ); // returns the array

Instead, far too many are doing this:

update_option( 'my_plugin_options', serialize( array( 'blah', 'foo', bar' ) ) );
unserialize( get_option( 'my_update_plugins' ) );

More or less, that means that you're serializing the data, then update_option is serializing serialized data, then get_option is unserializing it once, and unserialize is unserializing it again. r13673 breaks this, as update_option doesn't serialize the data a second time any more, causing the plugin's unserialize() to attempt to perform a second unserialize() on data that was only serialized once.

I will revert the changes in r13673. But please, update your plugins, spread the word.

comment:11 nacin4 years ago

(In [14074]) Revert changes to maybe_serialize() made in r13673. Do not prevent double-serialization of data. see #12930. see also #12416. xref #7347, r8100, r8372, and others.

comment:12 ipstenu4 years ago

nacin, thank you very much for tolerating my stupid-head explanation :) I am passing this on to every dev I know!

comment:13 cavemonkey504 years ago

I apologize nacin for the incorrect serialization usage in Google Analyticator. Normally I'm aware of that usage of of the options API, but the code that caused a problem was written during a quick coding session and it must have slipped my mind.

Anyway, Google Analyticator 6.0.3 will have this fixed, regardless of what happens in core.

comment:14 nacin4 years ago

:-) No problems folks. We don't like breaking plugins even if the plugin is 'Doing It Wrong' in the eyes of the API. The change to maybe_serialize() was unintentional so we've switched it back.

comment:15 nacin4 years ago

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

comment:16 hexalys14 months ago

  • Cc themacmaster@… added
  • Keywords needs-patch added
  • Resolution fixed deleted
  • Status changed from closed to reopened
  • Version set to 3.5.1

Nacin, r13673 may have addressed those plugin error at the time, but you introduced a new cache defect.

In update_option() where your changed to $alloptions[$option] = $_newvalue; and wp_cache_set( $option, $_newvalue, 'options' );
$_newvalue should be kept $newvalue when it come to the cache, to remain consistent with the way add_option() works.

Right now add_option() store options as serialized while update_option() stores them as arrays which is very inconsistent.
It really messes things up, especially with a memory cache, where you suddenly may get unexpected arrays instead of serialized values, but can't really know for sure what you will get.

Last edited 14 months ago by hexalys (previous) (diff)

comment:17 follow-up: ocean9014 months ago

  • Keywords 3.0-beta unserialize needs-patch removed
  • Resolution set to fixed
  • Status changed from reopened to closed
  • Version 3.5.1 deleted

As this ticket was closed as fixed on a completed milestone, please open a new ticket or take a look at #23381

comment:18 in reply to: ↑ 17 hexalys14 months ago

Replying to ocean90:

As this ticket was closed as fixed on a completed milestone, please open a new ticket or take a look at #23381

OK thanks, I missed that ticket.

Note: See TracTickets for help on using tickets.