WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 8 months ago

#18488 closed enhancement (fixed)

set_transient crashes when value parameter is a SimpleXMLElement object

Reported by: bobbingwide Owned by: wonderboymusic
Milestone: 3.7 Priority: normal
Severity: normal Version: 3.2.1
Component: Cache API Keywords: has-patch
Focuses: Cc:

Description

I have been developing a plugin that obtains information about high and low tides in the UK. The data is obtained from another site as a SimpleXMLElement. I found that when I passed a value to set_transient that had been assigned directly from the SimpleXMLObject then it was being received as an object and somewhere in the innards of the function the routine crashed. My workaround was to cast the assignment.
e.g. this failed

$title = $channel->item->title;
$store = '1';
$secs = bw_time_of_day_secs();
$secs = 86400 - $secs;
$set_transient( "bw_tides_title_" . $store, $title, $secs);

but change line 1 as below and it works

$title = (string) $channel->item->title;

Two questions therefore.

  1. Am I misinterpreting the documentation that says I don't need to serialise the value? What does mixed actually mean.
  2. Does set_transient have to crash when the parameters are wrong?

The problem is completely reproducable. I now have a working solution so I don't need a fix right now. I just want to understand.
Herb

Attachments (2)

18488.diff (5.4 KB) - added by scribu 20 months ago.
18488.2.diff (5.8 KB) - added by wonderboymusic 8 months ago.

Download all attachments as: .zip

Change History (22)

comment:1 johnbillion3 years ago

  • Cc johnbillion@… added
  • Component changed from General to Cache

comment:2 dd323 years ago

I'm pretty sure you'll find you can't serialize SimpleXML objects (And therefor, can't store in a Object Cache which WordPress uses)

The method you'd use to store an element would be to convert it to XML (It has a toXML() method IIRC) and create the new object afterwards from that xml. However, I don't think you can pull a node out of a SimpleXML object either reliably, as child nodes need the context of the parents too.

I'm not sure there's anything we can do here myself?

comment:3 dd323 years ago

Did some googling:
http://stackoverflow.com/questions/2970957/how-to-serialize-unserialize-a-simplexml-object

$s = new SimpleXmlElement('<foo>bar</foo>');
$ss = serialize($s);
$su = unserialize($ss);

Results in: Fatal error: Uncaught exception 'Exception' with message 'Serialization of 'SimpleXMLElement' is not allowed'

End result? You can't store a SimpleXML object serialized.

comment:4 follow-up: bobbingwide3 years ago

Hi Dion, thanks for your reply. I know that the code crashed and now I know that other people who have tried to serialize SimpleXML objects have not had any luck.
BUT, could you revisit my questions.
Can't the routine detect that I'm passing an object and cast it to a string?... because that's all I had to do after all.
Are core functions supposed to crash when passed invalid parameters?
Who should be updating the codex to document certain limitations?

comment:5 in reply to: ↑ 4 azaozz3 years ago

Replying to bobbingwide:

... Are core functions supposed to crash when passed invalid parameters?

Most functions in all software applications would "crash" if passed invalid parameters.

Who should be updating the codex to document certain limitations?

The codex is a wiki, feel free to drop a note there :) despite that this is a PHP limitation, not WordPress one.

comment:6 dd323 years ago

Can't the routine detect that I'm passing an object and cast it to a string?

Well, no, You can't just cast an object to a string, SimpleXML provides a nice toString() functionality though which allows you to do that to SimpleXML objects.

WordPress calls PHP's Serialize function to convert the object to a string notation for storage, and then the unserialize() function to convert that string back into the object upon retrieval.

When you're using SimpleXML, you need to be aware that it's not a "simple" class instance you're using, SimpleXML classes are much more complex than that, they're pretty much a representation of a file handle, and as such, are only usable within the context they're created, They can't be saved, if you need to cache them, then you need to convert it to a string (be that via the toXML() method, or by casting the value to a string)

Since we're using PHP5 now, We could wrap all serialize() calls in a try {} catch(e) {} block and return false to prevent fatal errors, but that'll most likely only cause more confusion as people don't understand why the functions not storing their objects, As it is, the error message should be descriptive to PHP Developers as to what the problem is and should only ever occur at development time, never to be seen in production environments.

comment:7 bobbingwide3 years ago

Replying to azaozz. I would have hoped that most functions would produce an error message if they were able to detect a problem with the parameters or API usage. Before I update the wiki I'd like to find out what the WordPress strategy is towards parameter validation and error handling.

To all readers: the main problem I had was that I didn't see any messages when the error occurred. Not even in log files. I have now tracked this down to some over zealous code in the wpStoreCart plugin. When I deactivate that plugin, then I do get to see:
Fatal error: Uncaught exception 'Exception' with message 'Serialization of 'SimpleXMLElement' is not allowed'

So the lesson learned is - always try to reduce problems to the simplest case,remove any excess baggage. PS. I had a different problem just a couple of days ago... and wrote about it at http://fobbonghide.co.uk/dotcom/2011/09/09/fob-warning-missing-argument-message-missing/

comment:8 nacin20 months ago

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

comment:9 bobbingwide20 months ago

OK, resolution accepted.
But I never did find out the WordPress strategy towards parameter validation and error handling. I do hope it's not as bad as wpStoreCart's was! Perhaps there's an opportunity to add some notes to the developer plugin to say that setting WP_DEBUG to true doesn't help you catch all the problems!

In reply to Andrew Ozz's ( azaozz ) comment see http://solongandthanksforalltheghoti.blogspot.co.uk/2009/09/what-did-you-expect.html

The upshot of the story in the link was that we developed very powerful exception handling for our APIs.

comment:10 mikeschinkel20 months ago

  • Cc mikeschinkel@… added
  • Keywords dev-feedback added
  • Resolution invalid deleted
  • Status changed from closed to reopened
  • Type changed from defect (bug) to enhancement

Proposed potential solution? I recently had exactly this need, which I think is common need for caching return values of APIs whose data is slow moving. The solution I found was to convert the SimpleAPI object an array of objects using a variation of this function:

function SimpleXMLElement_to_stdClass( $xml ) {
  $arr = array();
  foreach ($xml as $element) {
    $tag = $element->getName();
    $e = get_object_vars($element);
    if (!empty($e)) {
      $arr[$tag] = $element instanceof SimpleXMLElement ? $this->SimpleXMLElement_to_stdClass($element) : $e;
    }
    else {
      $arr[$tag] = trim($element);
    }
  }
  return (object)$arr;
}

Could we add a similar function to WordPress (maybe wp_SimpleXMLElement_to_stdClass()) so that people can easily convert SimpleXMLElements to usable formats and also have the set_transient() use this function to enable SimpleXMLElements to be saved without requiring the developer to handle it explicitly? The SimpleXMLElement could be reconstituted in get_transient() for parity between set and get.

Alternately still include the function but throw an error indicating the user is trying to save a SimpleXMLElement and tell them they need to use wp_SimpleXMLElement_to_stdClass() to transform it first?

Last edited 20 months ago by mikeschinkel (previous) (diff)

comment:11 SergeyBiryukov20 months ago

  • Milestone set to Awaiting Review

comment:12 nacin20 months ago

WordPress accepts scalar values for its option, transient, and meta APIs. It also accepts non-scalar values that are serializable. An array is, as are most objects. However, I see no reason to have custom code to handle SinpleXML objects, or any other non-serializable elements. They cannot be serialized for whichever reason the writer decided.

If you need to store such objects, I suggest you employ a custom workaround.

I don't see what else we should do here.

Version 0, edited 20 months ago by nacin (next)

comment:13 scribu20 months ago

  • Keywords needs-patch added; dev-feedback removed

I agree. The developer using set_transient() is in the best position to decide how to convert an unserializable object into something that is serializable.

The only thing we should do here is make it clear in the docs that the value needs to be serializable.

comment:14 scribu20 months ago

As for SimpleXMLElement_to_stdClass(), it shouldn't be inside set_transient(). At most, it should be a helper you call manually before passing the value to set_transient(), but I'm not convinced it needs to be included in Core.

scribu20 months ago

comment:15 scribu20 months ago

  • Keywords has-patch added; needs-patch removed

18488.diff changes 'mixed' to 'serializable' in the doc blocks.

comment:16 nacin20 months ago

"mixed" is a formal term used by PHP, phpdoc, etc. See http://us.php.net/manual/en/language.pseudo-types.php#language.types.mixed.

We should simply add a note that the argument must be serializable if non-scalar. (Obviously, a scalar value is serializable, but I don't want someone to read the doc and get the idea that we serialize scalar values.)

comment:17 scribu20 months ago

  • Keywords needs-patch added; has-patch removed

comment:18 DrewAPicture20 months ago

  • Cc xoodrew@… added

wonderboymusic8 months ago

comment:19 wonderboymusic8 months ago

  • Keywords has-patch added; needs-patch removed
  • Milestone changed from Awaiting Review to 3.7

Updated inline docz

comment:20 wonderboymusic8 months ago

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

In 25076:

Improve inline docs for function params that can be serialized when non-scalar. Fixes #18488.

Note: See TracTickets for help on using tickets.