__sleep() added to Message class causes that subclass cannot be serialized
karelvasicek opened this issue · 20 comments
... and yii\base\ErrorException: serialize(): "email" returned as member variable from __sleep() but does not exist is shown instead.
It seems to be caused by the fact that 'email' item is returned from __sleep() method and corresponding $email property is private and is not accessible in subclasses. charset is the same case IMHO.
What steps will reproduce the problem?
- create subclass of
yii\symfonymailer\Message serialize()object of subclass
What's expected?
Serialized object.
What do you get instead?
Error mentioned above.
Additional info
| Q | A |
|---|---|
| Yii version | 2.0.48.1 |
| Yii SymfonyMailer version | 3.0.0 |
| SymfonyMailer version | 5.4.22 |
| PHP version | 7.4.33 |
| Operating system | Fedora Linux |
Hi,
can you provide sample snippet of how you use the new sub-class?
can you provide sample snippet of how you use the new sub-class?
class InvoiceMessage extends \yii\symfonymailer\Message
{
public ?int $invoiceId = null;
}
and
$message = new InvoiceMessage(['invoiceId' => 123]);
serialize($message);
Tested your code
$message = new InvoiceMessage(['invoiceId' => 123]);
var_dump(serialize($message));
die;
string(438) "O:30:"app\controllers\InvoiceMessage":4:{s:6:"mailer";N;s:32:"yii\symfonymailer\Messageemail";O:28:"Symfony\Component\Mime\Email":6:{i:0;N;i:1;N;i:2;N;i:3;N;i:4;a:0:{}i:5;a:2:{i:0;O:37:"Symfony\Component\Mime\Header\Headers":2:{s:46:"Symfony\Component\Mime\Header\Headersheaders";a:0:{}s:49:"Symfony\Component\Mime\Header\HeaderslineLength";i:76;}i:1;N;}}s:34:"yii\symfonymailer\Messagecharset";s:5:"utf-8";s:9:"invoiceId";i:123;}"```
I have in composer.json `"yiisoft/yii2-symfonymailer": "^2.0.0"`
What is your version?
What is your version?
As mentioned in issue description, I use v3.0.0. v2 works as intended.
Sorry, my bad. I must have looked better. Let me try v3.x
So I checked and the referenced variables in sleep are private and hence cannot be inherited. I'm going to make a PR
Thanks for such quick resolution @mtangoo .
Thanks for reporting. You can use "dev-master" as version to temporaliy fix the issue until a release
@mtangoo , thanks, dev-master (more or less) works.
But could you please consider removing the __sleep() method completely? Problem is that serializing sublasses still requires overriding the __sleep() method. Either all serializable (all, in fact) properties must be listed there manually or (slow) reflection must be used. Luckily private properties do not exist any more so it is solvable, but it's still a little bit toilsome. What was the purpose of adding that __sleep() method to the Message class?
But could you please consider removing the __sleep() method completely? Problem is that serializing sublasses still requires overriding the __sleep()
If you are adding attributes that should be serialized, then you must override it to add them. If you are not adding anything that need to be serialized, you do not need to override it. What is the problem?
What was the purpose of adding that __sleep() method to the Message class?
for default serialization handling.
public function __sleep(): array
{
return ['email', 'charset'];
}then you must override it to add them
Yes, I understand it. That's what I wrote in my previous comment.
What is the problem?
Problem is that we have more subclasses and we create new ones from time to time and adding any new property requires overriding the __sleep() method again. The __sleep() method needeed not be overridden in subclasses at all if it was not defined in yii\symfonymailer\Message. Serializing would work out of the box automatically.
for default serialization handling.
Yes, I see. So the only excluded property is mailer. Is this the reason for defining the __sleep() method in the yii\symfonymailer\Message class?
Problem is that we have more subclasses and we create new ones from time to time and adding any new property requires overriding the __sleep() method again. The __sleep() method needeed not be overridden in subclasses at all if it was not defined in yii\symfonymailer\Message. Serializing would work out of the box automatically.
I do not get it. In case it is not defined, how do you handle default serialization?
I do not get it. In case it is not defined, how do you handle default serialization?
All object's properties are serialized automatically using serialize() function when __sleep() is not defined.
The intended use of __sleep() is to commit pending data or perform similar cleanup tasks. Also, the function is useful if a very large object doesn't need to be saved completely.
You do not want to serialize every nonsese in the object, In message class, it needs obly to serialize the two not every single one. Hence sleep. I do not see a simple solution to cater for both use case. Do you have a suggestion that's not dropping __sleep()?
You do not want to serialize every nonsese in the object
In case of subclasses, I don't understand much which "every nonsense" you meant. TBH I cannot imagine many examples of properties which a subclass' developer would not need to be serialized. If some properties from a subclass needed not be serialized, the developer could define the __sleep() method in the subclass.
In message class, it needs obly to serialize the two not every single one. Hence sleep.
As I wrote, the only message class' property that is not serialized now (when this default yii\symfonymailer\Message::__sleep() method is defined) is yii\mail\BaseMessage::$mailer. If excluding the only $mailer property from serialization was the reason for defining the __sleep() method, then we can't do anything with it.
Do you have a suggestion that's not dropping __sleep()?
I overrode the __sleep() method in our "parent" subclass, but using reflection is not recommended as it might be slow.
public function __sleep(): array
{
$reflection = new ReflectionClass($this);
$properties = $reflection->getProperties(ReflectionProperty::IS_PUBLIC | ReflectionProperty::IS_PROTECTED);
return array_map(function (ReflectionProperty $property) {
return $property->getName();
}, $properties);
}
In case of subclasses, I don't understand much which "every nonsense" you meant.
That was in reference to message class. Definitely wasn't meant to be rude nor to apply in your classes.
I overrode the __sleep() method in our "parent" subclass, but using reflection is not recommended as it might be slow.
I would love to see some numbers on how slow is that going to be compared to having no sleep. That will make it easy to see if overhead introduced by __sleep is worthy reconsideration that will break BC. Do you have time to do the profiling of two cases?
That was in reference to message class.
Yes, I understood it. But since I wrote about the actual message class in separate paragraph, I mentioned subclasses too.
Definitely wasn't meant to be rude nor to apply in your classes.
Yes, of course, it wasn't rude.
I would love to see some numbers on how slow
It's a general warning that using reflection might be slow. Anyway, I've just made some performance tests locally and __sleep() + reflection serialization is a little bit slower than no __sleep() at all case
Serializing 100000 message objects took:
__sleep() + reflection: 63s, 57s, 54sno __sleep() at all: 55s, 54s, 54s
But it's probably not worth removing the __sleep() method, even though...
that will break BC
...I think removing the __sleep() method won't break BC, will it?
Anyway, we can proceed with the reflection solution IMO.
Given 13s for such huge number of object, we can safely conclude that performance bottleneck is negligible with reflection. Let us just leave this as it is!
..I think removing the __sleep() method won't break BC, will it?
Yes it will, not in that there will be a crash, but result of serialization will be different, since we serialize everything and not just two properties. If anyone depends on this behavior then his code will break. Not sure why would someone depend on this behavior but then that is beside the point.
Anyway, we can proceed with the reflection solution IMO.
Yes please. Feel free to reopen this if the performance proves to be bad enough to warrant removal of __sleep!