php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Request #69959 unserialize() needlessly requires boilerplate
Submitted: 2015-06-29 07:08 UTC Modified: 2016-04-09 10:57 UTC
Votes:2
Avg. Score:3.5 ± 0.5
Reproduced:1 of 2 (50.0%)
Same Version:1 (100.0%)
Same OS:1 (100.0%)
From: lucas at threeamdesign dot com dot au Assigned:
Status: Suspended Package: *General Issues
PHP Version: 7.0.0alpha2 OS:
Private report: No CVE-ID: None
View Add Comment Developer Edit
Welcome! If you don't have a Git account, you can't do anything here.
You can add a comment by following this link or if you reported this bug, you can edit this bug over here.
(description)
Block user comment
Status: Assign to:
Package:
Bug Type:
Summary:
From: lucas at threeamdesign dot com dot au
New email:
PHP Version: OS:

 

 [2015-06-29 07:08 UTC] lucas at threeamdesign dot com dot au
Description:
------------
I'm aware that the docs have this to say:
---
Warning

FALSE is returned both in the case of an error and if unserializing the serialized FALSE value. It is possible to catch this special case by comparing str with serialize(false) or by catching the issued E_NOTICE.
---

Using the return value for failure and data is just lazy. This problem could be done away with, very easily if unserialize() were changed to accept a second parameter.

This parameter would be a reference variable. If the function was called with the second parameter, the reference would be filled with either the unserialized data, or the success/failure boolean of the process. The return value of the function would then be the other.

The latter form would be most backwards-compatible though seems more unintuitive.


Patches

Add a Patch

Pull Requests

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2015-06-29 18:29 UTC] kalle@php.net
We could potentially make it throw an Exception with the whole Engine Exceptions transition going on.

But it would break BC as you would have to write code like:
try {
 $s = unserialize('something invalid');
} catch(Exception $e) {
 echo $e->getMessage();
}

vs.
if(!($s = @unserialize('something invalid'))) {
 echo 'Error: Unable to un-serialize';
}
 [2015-06-30 01:07 UTC] lucas at threeamdesign dot com dot au
My point is

$data = unserialize('something invalid', $unserialized);
if (!$unserialized) {
    //handle error
}

or

if (!unserialize('something invalid', $data)) {
    //handle error
}

would be far more reliable, and obviate the need for error handling/suppression.
 [2015-10-06 04:27 UTC] lucas at threeamdesign dot com dot au
We currently have this

function funserialize($serialized, &$into) {
	static $sfalse;
	if (is_string($serialized)) {
		if ($sfalse === null) {
			$sfalse = serialize(false);
		}
		$into = @unserialize($serialized);
		return $into !== false || rtrim($serialized) === $sfalse;
	}
	$into = false;
	return false;
}

but this is convoluted and isn't guaranteed to be future-proof.
 [2016-03-27 06:58 UTC] krakjoe@php.net
-Status: Open +Status: Suspended
 [2016-03-27 06:58 UTC] krakjoe@php.net
Because of the widespread implications of changing the behaviour of unserialize, this discussion must go through the RFC process in order to make any changes.

Please see: https://wiki.php.net/rfc/howto
 [2016-04-08 23:07 UTC] lucas at threeamdesign dot com dot au
I'm sure you're time poor, but it seems you haven't really read my proposal. I offered a perfectly sound and *completely backwards-compatible* solution.

Adding an *optional* second parameter, that is a reference to whether the deserialization was successful, means the function's return value can stay the same, thus no updates are required to userland code. This mirrors the behaviour of `exec` where the shell return value is not the return value of the function.
 [2016-04-09 10:41 UTC] krakjoe@php.net
I read the proposal.

Changing the signature of serialize/unserialize, and or otherwise introducing new behaviour for a core function, even if it is optional behaviour, is exactly the kind of thing that requires proper discussion, by everybody.

My suspending the bug was not to end the conversation, rather to encourage you to push for the change using the correct process.

Writing up an RFC is easy, defining new behaviour is easy, but to make these kinds of changes, to argue the case for them, requires a hard working champion ... it could be you :)

Since we are allowed to throw exceptions from the engine, I personally think that the best solution is to throw exceptions in exceptional circumstances. 

I also thought, and think, you wouldn't be suggesting that we do something really strange with parameter and return values, if you were only aware that you can actually break BC in the pursuit of more elegant behaviour (we can't do it here on this bug tracker).

But my personal opinion doesn't actually matter ... 

What matters is process, and the opinion of everybody else.

Push forward with your idea to improve behaviour, RFC the best solution you can think of - I think it's exceptions - I'll even write the patch for you if you are not able to do it yourself.

Just showing you the way is all ...
 [2016-04-09 10:57 UTC] krakjoe@php.net
Another quick note ... 

Backward compatibility isn't the only kind of compatibility we have to consider.

Say we implemented your idea to have a truly optional parameter, this can break BC for anyone reflecting on the function, but let's ignore that completely. 

A forward compatibility problem is looming: What if we want to change serialize/unserialize to throw exceptions in the future, do we still have to populate these pointless parameters with state ? Do we remove the pointless parameters and break more code ?

It is rarely as simple as "let's just add this optional parameter" ... 

I want you to know that I thought about it before changing the status of the bug, and it really wasn't to end the conversation, at all. 

I appreciate you don't have to be here, like you appreciate I don't have to be here.
 
PHP Copyright © 2001-2024 The PHP Group
All rights reserved.
Last updated: Tue Mar 19 09:01:30 2024 UTC