php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Bug #79521 `__set_state` structure not been checked
Submitted: 2020-04-26 02:59 UTC Modified: 2020-04-28 15:05 UTC
From: carusogabriel@php.net Assigned: carusogabriel (profile)
Status: Closed Package: Class/Object related
PHP Version: Next Major Version OS: *
Private report: No CVE-ID: None
 [2020-04-26 02:59 UTC] carusogabriel@php.net
Description:
------------
The documentation (https://php.net/manual/en/language.oop5.magic.php#object.set-state) of the magic method `__set_state` says:

- it MUST take an array argument
- it MUST return an object

Ok, two problems:

- it CAN take an argument, is not mandatory, e.g.: https://3v4l.org/gTlmV
- it CAN return, is not mandatory: https://3v4l.org/SG8Hs

Should we fix its documentation? Something like

-static __set_state ( array $properties ) : object
+static __set_state ( [array $properties] ) : mixed


Patches

Add a Patch

Pull Requests

Pull requests:

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2020-04-26 03:06 UTC] requinix@php.net
-Package: Documentation problem +Package: Class/Object related
 [2020-04-26 03:06 UTC] requinix@php.net
The fact that you can get away without arguments is for the usual reasons: the signature of __set_state is not enforced by the engine, and it is not an error to pass more arguments than are defined in a function signature.

The return value is simply being ignored. Also not an error.

Related: the magic-methods-signature RFC
https://wiki.php.net/rfc/magic-methods-signature
It doesn't include __set_state, but this is likely just an oversight.

See also bug #69718
 [2020-04-26 03:13 UTC] carusogabriel@php.net
> It doesn't include __set_state, but this is likely just an oversight.

Yeah, I'm working on that right now, that's why I come up with this bug report.

> The return value is simply being ignored. Also not an error.

It isn't: it does change the result based on the type:

- https://3v4l.org/jt3jL (string)
- https://3v4l.org/tIHi3 (object)

This is supported by the engine: https://github.com/php/php-src/blob/d21d23afef54f152d9ca661909c6d042d6bfcf48/ext/standard/var.c#L472-L593

> and it is not an error to pass more arguments than are defined in a function signature.

It is not an error, this is why the documentation needs to be updated, right?
 [2020-04-26 03:26 UTC] requinix@php.net
Sorry, by ignored I meant that the type doesn't matter.
It really should return an object, though. It's what the method is for.

And I totally didn't notice the RFC was yours. I'll shut up now :)
 [2020-04-26 03:39 UTC] carusogabriel@php.net
> Sorry, by ignored I meant that the type doesn't matter.

:+1:


> It really should return an object, though. It's what the method is for.


In this case, we'd need an RFC removing that functionality, right?

> And I totally didn't notice the RFC was yours. I'll shut up now :)

Comments are always welcome, don't shut up ;)
 [2020-04-26 13:08 UTC] carusogabriel@php.net
-Summary: Fix `__set_state` documentation +Summary: `__set_state` structure not been checked -Type: Documentation Problem +Type: Bug
 [2020-04-26 13:08 UTC] carusogabriel@php.net
I've opened https://github.com/php/php-src/pull/5462 to fix this. Indeed, not a documentation problem, we need to fix it.
 [2020-04-27 15:08 UTC] carusogabriel@php.net
-Assigned To: +Assigned To: carusogabriel
 [2020-04-27 15:08 UTC] carusogabriel@php.net
The following pull request has been associated:

Patch Name: [#79521] Check `__set_state` structure
On GitHub:  https://github.com/php/php-src/pull/5462
Patch:      https://github.com/php/php-src/pull/5462.patch
 [2020-04-28 15:05 UTC] carusogabriel@php.net
-Status: Assigned +Status: Closed -Operating System: +Operating System: * -PHP Version: Irrelevant +PHP Version: Next Major Version
 
PHP Copyright © 2001-2020 The PHP Group
All rights reserved.
Last updated: Sat Sep 26 09:01:24 2020 UTC