Hey:
I was trying play with OroCRM. then I find a BC break in PHP-5.6
the usage is :
/**
* Creates a new instance of the mapped class, without invoking
the constructor.
*
* @return object
*/
public function newInstance()
{
if ($this->_prototype === null) {
$this->_prototype = unserialize(sprintf('O:%d:"%s":0:{}',
strlen($this->name), $this->name));
}
return clone $this->_prototype;
}
it works fine in PHP-5.5, but in 5.6 , the serialize will fail
after fix 5328d4289946e260232f3195ba2e0f0eb173d5ef
then I found, Stas did a re-fix for that:
342240fd7fb6ac0a287eb6f912c4d61d6274d68c
but it seems doesn't go into 5.6..
I am not sure now. should this usage be supported?
thanks
--
Laruence Xinchen Hui
http://www.laruence.com/
Le 17/07/2014 06:01, Laruence a écrit :
$this->_prototype = unserialize(sprintf('O:%d:"%s":0:{}',strlen($this->name), $this->name));
I am not sure now. should this usage be supported?
In think unserialize is an horrible hack which should have never be used.
This is described in UPGRADINGS
First, try to fix the code, using newInstanceWithoutConstructor() when
available (5.4+) and possible (userland classes)
Remi.
Le 17/07/2014 06:01, Laruence a écrit :
$this->_prototype = unserialize(sprintf('O:%d:"%s":0:{}',strlen($this->name), $this->name));
I am not sure now. should this usage be supported?
In think unserialize is an horrible hack which should have never be used.
This is described in UPGRADINGS
First, try to fix the code, using newInstanceWithoutConstructor() when
available (5.4+) and possible (userland classes)
Hey:
serialize maybe a sensitive area, in my opinion we should very
careful about the BC issue in such area..
if a user want to migrate it's codes from 5.5 to 5.6, but he has
multi servers, he might want to migrate them one by one.
in such case, the serialized data could be shared by 5.5 and 5.6...
so...
thanks
Remi.
--
--
Laruence Xinchen Hui
http://www.laruence.com/
Hi!
in such case, the serialized data could be shared by 5.5 and 5.6...
This is true, but what you presented is not serialized data. Serialized
data will be fine. But the code you shown instead tries to use
serializer as a roundabout way of instantiating objects. This is not the
right thing to do, this was never the purpose of the serializer and was
never guaranteed to work.
--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
-----Original Message-----
From: Stas Malyshev [mailto:smalyshev@sugarcrm.com]
Sent: Thursday, July 17, 2014 9:08 AM
To: Laruence; Remi Collet
Cc: PHP Internals
Subject: Re: [PHP-DEV] An BC issue in unserializeHi!
in such case, the serialized data could be shared by 5.5 and 5.6...This is true, but what you presented is not serialized data. Serialized
data will
be fine. But the code you shown instead tries to use serializer as a
roundabout
way of instantiating objects. This is not the right thing to do, this was
never the
purpose of the serializer and was never guaranteed to work.
My key concern is that I've now bumped into two applications - both OroCRM
and also Taskada - that work fine with 5.5 but stop working with 5.6 because
of that issue. Both are Symfony apps so it might be something that can be
solved centrally in Symfony, but it may also be just coincidence. Either
way, it's an indication that this would make upgrading to 5.6 harder than it
needs to be, and a key tenet of our yearly release cycle was guaranteed
pain-free upgrades.
IMHO this isn't something we should change in a 2nd digit release, but on a
major version, even if it wasn't documented.
Zeev
Hi!
IMHO this isn't something we should change in a 2nd digit release, but on a
major version, even if it wasn't documented.
We don't have much option here. Keeping it leads to a remote triggerable
segfaults. We've discussed this here just recently. This is a hack that
does not work properly with internal classes, and should not work too -
the whole reason C: was created is because O: can not work with such
classes.
--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
On Thu, Jul 17, 2014 at 8:43 AM, Stas Malyshev smalyshev@sugarcrm.com
wrote:
Hi!
IMHO this isn't something we should change in a 2nd digit release, but
on a
major version, even if it wasn't documented.We don't have much option here. Keeping it leads to a remote triggerable
segfaults. We've discussed this here just recently. This is a hack that
does not work properly with internal classes, and should not work too -
the whole reason C: was created is because O: can not work with such
classes.
As discussed in previous threads about this failure, we (doctrine) can move
away from the unserialize() hack if
ReflectionClass#newInstanceWithoutConstructor() provides support for
internal classes.
It doesn't need to cover ALL internal classes, just the most commonly
extended ones.
Marco Pivetta
Hi!
As discussed in previous threads about this failure, we (doctrine) can
move away from theunserialize()hack if
ReflectionClass#newInstanceWithoutConstructor()provides support for
internal classes.
Could you explain why it is needed to instantiate internal classes
without calling the ctor? I'd like to understand the use case more.
It doesn't need to cover ALL internal classes, just the most commonly
extended ones.
The problem is we do not know which of these classes may fail if
instantiated without initializing. With PHP classes, it's easy since the
engine takes care of the basic plumbing. With C classes, if you don't
call the function, you get nulls or worse, garbage, in places where
values are expected.
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
On Thu, Jul 17, 2014 at 9:58 AM, Stas Malyshev smalyshev@sugarcrm.com
wrote:
Hi!
As discussed in previous threads about this failure, we (doctrine) can
move away from theunserialize()hack if
ReflectionClass#newInstanceWithoutConstructor()provides support for
internal classes.Could you explain why it is needed to instantiate internal classes
without calling the ctor? I'd like to understand the use case more.
I honestly don't have a use-case for it myself, and I also strongly
disagree with extending internal PHP classes due to their fragility.
What I know is that many Doctrine users extend ArrayObject in entities,
which is what caused us to apply a hotfix only for 5.4.29 and 5.5.13, and
NOT 5.6.0.
It doesn't need to cover ALL internal classes, just the most commonly
extended ones.The problem is we do not know which of these classes may fail if
instantiated without initializing. With PHP classes, it's easy since the
engine takes care of the basic plumbing. With C classes, if you don't
call the function, you get nulls or worse, garbage, in places where
values are expected.
Yes, that indeed.
What I am suggesting is to provide a small map of internal PHP classes that
are supported by ReflectionClass#newInstanceWithoutConstructor(), so that
their internal data can be initialized even if the userland constructor is
not invoked.
Marco Pivetta
> /**
> * Creates a new instance of the mapped class, without invoking
> the constructor.
> *
> * @return object
> */
> public function newInstance()
> {
> if ($this->_prototype === null) {
> $this->_prototype = unserialize(sprintf('O:%d:"%s":0:{}',
> strlen($this->name), $this->name));
> }
>
> return clone $this->_prototype;
> }
>
Just curious.
This code is trying to initialize object with __wakeup() method
rather than __construct(). Do you know what kind of object is initialized?
What's the reason why it needs to use __wakeup() or avoid __construct()?
If the usage is valid, we may try to find solutions.
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net