Re: [PHP-DEV] [VOTE] Improve unserialize() error handling
Hi
Resending the message without attachments, because the mailing list
rejected the original due to exceeding 3 bytes:
ezmlm-reject: fatal: Sorry, I don't accept messages larger than 3 bytes
(#5.2.3)
You can find the attachments at:
https://gist.github.com/TimWolla/376dd162f7684daef38f76a07254871c
Original message below:
--
On 10/15/22 11:06, Nicolas Grekas wrote:
I'm not surprised by the “no” on the first vote based on the previous
discussion. I am surprised however that you also disagree with raising
the E_NOTICE to E_WARNING for consistency.
I don't plan on repeating all the previous discussion points with you,
but as you mention the Symfony tests specifically: Please find the patch
attached. Do you believe the expectations in this new test would a
reasonable expectation by a Symfony user?
Since the beginning my point is not that the RFC doesn't have merits. It's
that the proposed approach breaks BC in a way that will affect the
community significantly. We have policies that say we should avoid BC
breaks and they should apply here.
To anyone wondering about the reality of the BC break if you're not
convinced there is one because the original code is broken anyway (which is
your main argument Tim IIUC): please have a look at the failures I reported
Yes, you understood this correctly: I consider the existing
implementation to be broken. Unfortunately you did not answer my
question whether the attached patch would be a reasonable expectation by
a Symfony user.
Let's presume it is: If I use the 'PhpSerializer' then I expect to
receive exactly the 'MessageDecodingFailedException' whenever the
message fails to decode. When decoding an erroneous DateTime (or an
erroneous SplDoublyLinkedList or whatever) a different Exception will be
thrown and not caught, thus the contract by the 'PhpSerializer' is violated.
Let's fix this using test driven development. We first add the failing
test cases. This is step1.diff (patches are in
https://gist.github.com/TimWolla/376dd162f7684daef38f76a07254871c). When
running the tests we get the following output:
There were 2 failures:
1)
Symfony\Component\Messenger\Tests\Transport\Serialization\PhpSerializerTest::testDecodingFailsWithBadDateTimeData
Failed asserting that exception of type "Error" matches expected exception
"Symfony\Component\Messenger\Exception\MessageDecodingFailedException". Message was: "Invalid
serialization data for DateTime object" at
/app/src/Symfony/Component/Messenger/Transport/Serialization/PhpSerializer.php:95
/app/src/Symfony/Component/Messenger/Transport/Serialization/PhpSerializer.php:54
/app/src/Symfony/Component/Messenger/Tests/Transport/Serialization/PhpSerializerTest.php:99
.
2)
Symfony\Component\Messenger\Tests\Transport\Serialization\PhpSerializerTest::testDecodingFailsWithBadDoublyLinkedListData
Failed asserting that exception of type "UnexpectedValueException" matches expected exception
"Symfony\Component\Messenger\Exception\MessageDecodingFailedException". Message was:
"Incomplete or ill-typed serialization data" at
/app/src/Symfony/Component/Messenger/Transport/Serialization/PhpSerializer.php:95
/app/src/Symfony/Component/Messenger/Transport/Serialization/PhpSerializer.php:54
/app/src/Symfony/Component/Messenger/Tests/Transport/Serialization/PhpSerializerTest.php:110
.
The tests fails (as expected) and we need to fix the code. To fix this
we add a new 'catch(Throwable)' to the existing 'try'. Within the catch
we throw a new 'MessageDecodingFailedException' that wraps the Throwable
we caught using the '$previous' parameter. You can find the change in
step2.diff. Let me re-run the tests:
There was 1 failure:
1)
Symfony\Component\Messenger\Tests\Transport\Serialization\PhpSerializerTest::testDecodingFailsWithBadClass
Failed asserting that exception message 'Could not decode message using PHP serialization:
O:13:"ReceivedSt0mp":0:{}.' matches '/class "ReceivedSt0mp" not found/'.
Okay, now the Exception message changed. Personally I do not consider
this a BC break: I believe Exception messages are meant for human
consumption, not for programs. Otherwise fixing a typo in the message
would be a BC break. If the code wants to learn about the cause, it
should either use the '$code' or different types of Exception should be
thrown to clarify the cause by entering a different catch() block.
There are two options here: We could fix the expectation (that's what I
would do, see step3a.diff), or we could use the message of the original
Exception as the message of the wrapper Exception (step3b.diff), or we
could rethrow the original exception, if it already is of the correct
class (step3c.diff).
In all three variants of Step 3 we now fixed the bug for DateTime and
SplDoublyLinkedList. PhpSerializer will now consistently throw
MessageDecodingFailedException in all cases, just the message might have
changed (in variant 'a').
above and wonder about the cha
Re: [PHP-DEV] Casting array to any class
On Sat, Oct 15, 2022, at 6:18 AM, Gianni Gentile wrote:
> Hi,
>
> in my every day experience, using custom DTO classes and different API,
> I often write code to instantiate my objects from associative arrays.
>
> What are your thoughts on introduce the `(AnyType)` cast to instantiate
> objects from associative array (or object properties also)?
>
> In my proposal, if AnyType implements __set_state(), casting should be
> syntactic sugar for:
>
> AnyType::_set_state((array) $data)
>
> just for (useless) example, consider DateTime or DateTimeImmutable (both
> have __set_state() implementation); when applied to objects of that
> type, we can write:
>
> $dt = new DateTime();
>
> $dtArray = (array) $dt; // 3 elements array
>
> $copyOfDt = (DateTime) $dtArray; // or simply $copyOfDt =
> (DateTime) $dt;
>
> In case AnyType does not implements __set_state(), casting should create
> a new instance, assigning values to properties, taken from array values
> with corresponding key (in a similar way class objects are created when
> row is fetched from database specifying PDO_FETCH_CLASS) eventually
> invoking __set() magic method for properties not declared;
>
> In this scenario,
>
> (object) ['one' => 1, 'two' => 2]
>
> and
>
> (StdClass) ['one' => 1, 'two' => 2]
>
> do exactly the same work.
>
>
> Reasons:
>
> - clear code
>
> - array to object conversion (casting) would be the simmetrical
> counterpart of object to array conversion
Most DTOs have a 1:1 match from constructor to properties. In current PHP
versions, that means they often look like this:
class Point
{
public function __construct(
public readonly int $x,
public readonly int $y,
public readonly int $z,
) {}
}
Which means, in current PHP versions, you can use named args and splat to
create one from an array.
$data = ['x' => 1, 'y' => 2, 'z' => 3];
$point = new Point(...$data);
No casting necessary.
True, that's not true of all such classes, but certainly the plurality. In the
cases where it's not, an explicit named constructor (via a Trait or otherwise)
is a more self-documenting approach.
--Larry Garfield
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php
Re: [PHP-DEV] [VOTE] Improve unserialize() error handling
El sáb., 15 de octubre de 2022 11:02, juan carlos morales < dev.juan.mora...@gmail.com> escribió: > As far as I read... Everyone is right. All agree is BC break change, but > this is being considered to be part of PHP 9.0, so if what We get is a > better PHP then We must move forward, and such steps should be done > in a major version change. > I just checked that the RFC says in the PROPOSED PHP VERSIONS section next PHP 8.X Maybe that should be adjusted for clarification and documentation reasons >
Re: [PHP-DEV] Casting array to any class
On Sat, 15 Oct 2022 at 13:14, Gianni Gentile
wrote:
>
> What are your thoughts on introduce the `(AnyType)` cast to instantiate
> objects from associative array (or object properties also)?
It seems a pretty bad idea.
In particular, it makes for hard to maintain code as it doesn't give a
place for error checking to occur.
Though I do actually use that pattern in code. Below is a trait that
accomplishes it that should only be used in prototype code. Having it
as a trait means at least I can debug through the initialization and
see if it's missing properties from the array,
But in general, you'd really need to make a strong argument for why it
should be in core, not just why anyone would be against it.
cheers
Dan
Ack
trait FromArray
{
/**
* @param array $data
* @return static
* @throws \ReflectionException
*/
public static function fromArray(array $data): self
{
$reflection = new \ReflectionClass(__CLASS__);
$instance = $reflection->newInstanceWithoutConstructor();
foreach ($instance as $key => &$property) {
$property = $data[$key];
}
return $instance;
}
}
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php
Re: [PHP-DEV] Casting array to any class
Hi,
Am 15.10.22 um 13:18 schrieb Gianni Gentile:
Hi,
in my every day experience, using custom DTO classes and different API,
I often write code to instantiate my objects from associative arrays.
What are your thoughts on introduce the `(AnyType)` cast to instantiate
objects from associative array (or object properties also)?
In my proposal, if AnyType implements __set_state(), casting should be
syntactic sugar for:
AnyType::_set_state((array) $data)
just for (useless) example, consider DateTime or DateTimeImmutable (both
have __set_state() implementation); when applied to objects of that
type, we can write:
$dt = new DateTime();
$dtArray = (array) $dt; // 3 elements array
$copyOfDt = (DateTime) $dtArray; // or simply $copyOfDt =
(DateTime) $dt;
In case AnyType does not implements __set_state(), casting should create
a new instance, assigning values to properties, taken from array values
with corresponding key (in a similar way class objects are created when
row is fetched from database specifying PDO_FETCH_CLASS) eventually
invoking __set() magic method for properties not declared;
In this scenario,
(object) ['one' => 1, 'two' => 2]
and
(StdClass) ['one' => 1, 'two' => 2]
do exactly the same work.
Reasons:
- clear code
- array to object conversion (casting) would be the simmetrical
counterpart of object to array conversion
I see your use case and I often have similar needs. Some thoughts:
* Are there any restrictions regarding the matching of array keys and
the class attributes? What if there are additional keys in the array
that do not have corresponding attributes? What if there are some
attributes in the class that do not exist in the array?
* Can I forbid a class to be created by casting from an array? When I
think about domain entities/aggregates, such a casting would allow to
create an invalid state of the object without any checks. (Okay, we have
similar issues when recreating them from a database via some ORM.)
BTW:
Will dump with modified array keys:
/home/thomas/test.php:12:
array(3) {
'\0A\0myPrivate' =>
string(7) "private"
'\0*\0myProtected' =>
string(9) "protected"
'myPublic' =>
string(6) "public"
}
So it's not as symmetric as intended.
Regards
Thomas
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php
Re: [PHP-DEV] [VOTE] Improve unserialize() error handling
As far as I read... Everyone is right. All agree is BC break change, but this is being considered to be part of PHP 9.0, so if what We get is a better PHP then We must move forward, and such steps should be done in a major version change.
[PHP-DEV] Casting array to any class
Hi, in my every day experience, using custom DTO classes and different API, I often write code to instantiate my objects from associative arrays. What are your thoughts on introduce the `(AnyType)` cast to instantiate objects from associative array (or object properties also)? In my proposal, if AnyType implements __set_state(), casting should be syntactic sugar for: AnyType::_set_state((array) $data) just for (useless) example, consider DateTime or DateTimeImmutable (both have __set_state() implementation); when applied to objects of that type, we can write: $dt = new DateTime(); $dtArray = (array) $dt; // 3 elements array $copyOfDt = (DateTime) $dtArray; // or simply $copyOfDt = (DateTime) $dt; In case AnyType does not implements __set_state(), casting should create a new instance, assigning values to properties, taken from array values with corresponding key (in a similar way class objects are created when row is fetched from database specifying PDO_FETCH_CLASS) eventually invoking __set() magic method for properties not declared; In this scenario, (object) ['one' => 1, 'two' => 2] and (StdClass) ['one' => 1, 'two' => 2] do exactly the same work. Reasons: - clear code - array to object conversion (casting) would be the simmetrical counterpart of object to array conversion Thanks, Gianni -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: https://www.php.net/unsub.php
[PHP-DEV] Casting array to any class
Hi, in my every day experience, using custom DTO classes and different API, I often write code to instantiate my objects from associative arrays. What are your thoughts on introduce the `(AnyType)` cast to instantiate objects from associative array (or object properties also)? In my proposal, if AnyType implements __set_state(), casting should be syntactic sugar for: AnyType::_set_state((array) $data) just for (useless) example, consider DateTime or DateTimeImmutable (both have __set_state() implementation); when applied to objects of that type, we can write: $dt = new DateTime(); $dtArray = (array) $dt; // 3 elements array $copyOfDt = (DateTime) $dtArray; // or simply $copyOfDt = (DateTime) $dt; In case AnyType does not implements __set_state(), casting should create a new instance, assigning values to properties, taken from array values with corresponding key (in a similar way class objects are created when row is fetched from database specifying PDO_FETCH_CLASS) eventually invoking __set() magic method for properties not declared; In this scenario, (object) ['one' => 1, 'two' => 2] and (StdClass) ['one' => 1, 'two' => 2] do exactly the same work. Reasons: - clear code - array to object conversion (casting) would be the simmetrical counterpart of object to array conversion Thanks, Gianni -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: https://www.php.net/unsub.php
Re: [PHP-DEV] [VOTE] Improve unserialize() error handling
On 15.10.2022 at 11:06, Nicolas Grekas wrote: >>> I'm therefore voting NO on the proposal. >> >> I'm not surprised by the “no” on the first vote based on the previous >> discussion. I am surprised however that you also disagree with raising >> the E_NOTICE to E_WARNING for consistency. > > Since the beginning my point is not that the RFC doesn't have merits. It's > that the proposed approach breaks BC in a way that will affect the > community significantly. We have policies that say we should avoid BC > breaks and they should apply here. Well, I consider the current behavior very unfortunate; sometimes E_NOTICE, sometimes E_WARNING, sometimes both, and sometimes a UnexpectedValueException – and this looks more like historic randomness than having any deeper reasoning behind it. I also understand that we need to be concerned about BC breaks. So for me the question is whether we want to stick with the current behavior *forever*, or if there is an acceptable way forward. I certainly hope for the latter. Would it really be a serious issue to promote E_NOTICE to E_WARNING in PHP 8.3? I don't think so, but I may be wrong. And would it really be a serious issue to promote E_WARNING to an exception in PHP 9? Maybe, but after all that would be a new major version, so some BC breaks are to be expected anyway. -- Christoph M. Becker -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: https://www.php.net/unsub.php
Re: [PHP-DEV] [VOTE] Improve unserialize() error handling
Tbh, this affects a new minor and a new major, and only in an unhappy path scenario, where the current PHP API is bad. PHP 9 will introduce a hard crash: good. It's a BC break: yes, native de-serialization is one of the most unsafe parts of the language, and it requires hardening. The fact that Symfony declared that it already supports PHP 8.3 and PHP 9.0 is your problem, and mostly your misunderstanding of SemVer On Sat, 15 Oct 2022, 11:06 Nicolas Grekas, wrote: > > > Not sure why I didn't think about it before but I just ran the test > suite > > > of Symfony after applying the patch proposed in the RFC to change the > way > > > exceptions are handled by unserialize. > > > > > > This change breaks the test suite of 5 separate components. I created > > this > > > gist to list all the failures: > > > > https://gist.github.com/nicolas-grekas/3da652a51669baa40c99bd20e4a1b4dd > > > > > > Maybe I wasn't convincing enough during the discussion period, but that > > > doesn't change the fact that the proposed behavior in the RFC is a very > > > clear BC break that will affect userland significantly. > > > > > > I'm therefore voting NO on the proposal. > > > > > > > I'm not surprised by the “no” on the first vote based on the previous > > discussion. I am surprised however that you also disagree with raising > > the E_NOTICE to E_WARNING for consistency. > > > > I don't plan on repeating all the previous discussion points with you, > > but as you mention the Symfony tests specifically: Please find the patch > > attached. Do you believe the expectations in this new test would a > > reasonable expectation by a Symfony user? > > > > Since the beginning my point is not that the RFC doesn't have merits. It's > that the proposed approach breaks BC in a way that will affect the > community significantly. We have policies that say we should avoid BC > breaks and they should apply here. > > To anyone wondering about the reality of the BC break if you're not > convinced there is one because the original code is broken anyway (which is > your main argument Tim IIUC): please have a look at the failures I reported > above and wonder about the changes the RFC would impose. I cannot think > about one that would not imply some sort of "if the version of PHP is >= > 8.3 then A, else B". This is the fact that highlights the BC break. > > Nicolas >
Re: [PHP-DEV] RFC [Discussion]: Randomizer Additions
Hi For completeness, it would be good to have nextBool() as well. I'm just wondering if that's really necessary. Generating a boolean is trivial with the nextFloat method (see example 1. Simulate a coinflip from the RFC). No, IMO. Mathematically it doesn't really make sense and talking about floats, it will also be a very corner case not reached in tests that might happen in production rarely and break things. Why does it not make mathematical sense? With the nextFloat method, I can understand the argument, because that is otherwise opaque, especially when you work with probabilities. With the getFloat method, however, I can imagine a few cases in which this makes sense. In our example, for example, we calculate a random longitude and latitude with the method. However, the value of Lat: +90.0 Lng: +180.0 cannot be generated, although it is a valid value. However, the value of Lat: -90.0 Lng: -180.0 is included. Am I missing something, or is there currently no simple mathematical way to implement this with an open-right interval? I am having another small issue. As the Randomizer class is final, I guess this will not be perfectly polyfillable in userland. So... , if accepted, would it be completely wrong to have these new methods in PHP 8.2? What can it break? I am relatively new and have little to no experience with contributing PHP functions, but I don't think this should be done. There is a good reason why this is not done and it only causes confusion if any functions are introduced after the feature freeze. In the end, however, I think it is the release manager who decides. Cheers Joshua Rüsweg -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: https://www.php.net/unsub.php
Re: [PHP-DEV] [VOTE] Improve unserialize() error handling
> > Not sure why I didn't think about it before but I just ran the test suite > > of Symfony after applying the patch proposed in the RFC to change the way > > exceptions are handled by unserialize. > > > > This change breaks the test suite of 5 separate components. I created > this > > gist to list all the failures: > > https://gist.github.com/nicolas-grekas/3da652a51669baa40c99bd20e4a1b4dd > > > > Maybe I wasn't convincing enough during the discussion period, but that > > doesn't change the fact that the proposed behavior in the RFC is a very > > clear BC break that will affect userland significantly. > > > > I'm therefore voting NO on the proposal. > > > > I'm not surprised by the “no” on the first vote based on the previous > discussion. I am surprised however that you also disagree with raising > the E_NOTICE to E_WARNING for consistency. > > I don't plan on repeating all the previous discussion points with you, > but as you mention the Symfony tests specifically: Please find the patch > attached. Do you believe the expectations in this new test would a > reasonable expectation by a Symfony user? > Since the beginning my point is not that the RFC doesn't have merits. It's that the proposed approach breaks BC in a way that will affect the community significantly. We have policies that say we should avoid BC breaks and they should apply here. To anyone wondering about the reality of the BC break if you're not convinced there is one because the original code is broken anyway (which is your main argument Tim IIUC): please have a look at the failures I reported above and wonder about the changes the RFC would impose. I cannot think about one that would not imply some sort of "if the version of PHP is >= 8.3 then A, else B". This is the fact that highlights the BC break. Nicolas
