Skip to content

[Mime] MessagePart does not support serialization - Profiler fails if Message with MessagePart is collected #48313

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
Amunak opened this issue Nov 24, 2022 · 0 comments
Closed

[Mime] MessagePart does not support serialization - Profiler fails if Message with MessagePart is collected #48313

Amunak opened this issue Nov 24, 2022 · 0 comments
Labels
Bug Mime Status: Needs Review

Comments

@Amunak
Copy link
Contributor

Amunak commented Nov 24, 2022 •
edited
Loading

Symfony version(s) affected

4.3.0-6.2 (all)

Description

Object Symfony\Component\Mime\Part\MessagePart does not support serialization.

This will most likely manifest in Profiler failing to save the collected MessagePart, but it could break other (user) code as well.

There are really two problems:

  • The object is missing the __sleep magic method. Parent __sleep gets called, but that returns wrong/different property names than exist in the object (because parent's are private), so the most important property for this object ($message) is not serialized, and when unserialized methods like getBody fail unexpectadly because property $message is null.
  • Because parent's properties are private, serialization fails with error Warning: serialize(): "filename" returned as member variable from __sleep() but does not exist, as from the point of view of this child object that property truly does not exist. However that property is required for the object to work properly, and it should either be saved or parent constructor needs to be called correctly on unserialization (__wakeup call).

How to reproduce

Add the following test to MessagePartTest:

    public function testSerialize()
    {
        $email = (new Email())->from('fabien@symfony.com')->to('you@example.com')->text('content');
        $email->getHeaders()->addIdHeader('Message-ID', $email->generateMessageId());

        $p = new MessagePart($email);
        $expected = clone $p;
        $this->assertEquals($expected->toString(), unserialize(serialize($p))->toString());
    }

You will observe it will fail.

Possible Solution (see #48314)

Implement magic __sleep and __wakeup methods, for example like so:

class MessagePart extends DataPart
{
    // ...

    /**
     * @return array
     */
    public function __sleep()
    {
        return ['message'];
    }

    public function __wakeup()
    {
        $this->__construct($this->message);
    }
}

Additionally I think it would be a great idea to improve FileProfilerStorage so that it fails gracefully if (one of) the Collectors has an issue and fails the serialization. In testing I used something like this for the write function (and inverse for doRead):

        try {
            set_error_handler(static function (int $severity, string $message, string $file, int $line) use (&$errorMessages): bool { return true; });

            foreach ($data['data'] as $key => $value) {
                $data['data'][$key] = serialize($value);
            }

        } catch (Throwable) {
            trigger_error(sprintf('Failed serializing collector %s (%s). ', $key, $value::class), E_USER_WARNING);
        } finally {
            restore_error_handler();
        }

        $data = serialize($data);

which still allows all but the one failing Collector to save so that you have at least something to debug with.
@Amunak Amunak added the Bug label Nov 24, 2022
Amunak added a commit to Amunak/symfony that referenced this issue Nov 24, 2022
@Amunak
bug symfony#48313 [Mime] Fix MessagePart serialization
91a80eb 
Fixes symfony#48313
@Amunak Amunak mentioned this issue Nov 24, 2022
Merged
nicolas-grekas pushed a commit to Amunak/symfony that referenced this issue Dec 13, 2022
@Amunak @nicolas-grekas
bug symfony#48313 [Mime] Fix MessagePart serialization
8cb094c 
Fixes symfony#48313
nicolas-grekas added a commit that referenced this issue Dec 13, 2022
@nicolas-grekas
bug #48314 [Mime] Fix MessagePart serialization (Amunak)
ed1ab3f 
This PR was merged into the 5.4 branch.

Discussion
----------

[Mime] Fix MessagePart serialization

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #48313
| License       | MIT
| Doc PR        | n/a

Fixes serialization of Symfony\Component\Mime\Part\MessagePart and adds test coverage.

Commits
-------

8cb094c bug #48313 [Mime] Fix MessagePart serialization
nicolas-grekas added a commit that referenced this issue Dec 14, 2022
@nicolas-grekas
Merge branch '5.4' into 6.0
6c0e925 
* 5.4:
  [Mailer] Include all transports' debug messages in RoundRobin transport exception
  [FrameworkBundle] fix: fix help message
  Use relative timestamps
  [Cache] Fix dealing with ext-redis' multi/exec returning a bool
  [Messenger][Amqp] Added missing rpc_timeout option
  [Serializer] Prevent GetSetMethodNormalizer from creating invalid magic method call
  [HttpFoundation] Fix dumping array cookies
  [WebProfilerBundle] Fix dump header not being displayed
  TraceableHttpClient: increase decorator's priority
  Use static methods inside data providers
  [FrameworkBundle] Allow configuring `framework.exceptions` with a config builder
  bug #48313 [Mime] Fix MessagePart serialization
  [ErrorHandler][DebugClassLoader] Fix some new return types support
  Fix getting the name of closures on PHP 8.1.11+
  [Translator] Fix typo "internal" / "interval"
  fix dumping top-level tagged values
nicolas-grekas added a commit that referenced this issue Dec 14, 2022
@nicolas-grekas
Merge branch '6.0' into 6.1
e872a0f 
* 6.0:
  [Mailer] Include all transports' debug messages in RoundRobin transport exception
  [FrameworkBundle] fix: fix help message
  Use relative timestamps
  [Cache] Fix dealing with ext-redis' multi/exec returning a bool
  [Messenger][Amqp] Added missing rpc_timeout option
  [Serializer] Prevent GetSetMethodNormalizer from creating invalid magic method call
  [HttpFoundation] Fix dumping array cookies
  [WebProfilerBundle] Fix dump header not being displayed
  TraceableHttpClient: increase decorator's priority
  Use static methods inside data providers
  [FrameworkBundle] Allow configuring `framework.exceptions` with a config builder
  bug #48313 [Mime] Fix MessagePart serialization
  [ErrorHandler][DebugClassLoader] Fix some new return types support
  Fix getting the name of closures on PHP 8.1.11+
  [Translator] Fix typo "internal" / "interval"
  fix dumping top-level tagged values
nicolas-grekas added a commit that referenced this issue Dec 14, 2022
@nicolas-grekas
Merge branch '6.1' into 6.2
52bdd04 
* 6.1:
  [Mailer] Include all transports' debug messages in RoundRobin transport exception
  [FrameworkBundle] fix: fix help message
  Fix HtmlSanitizer default configuration behavior for allowed schemes
  Use relative timestamps
  [Cache] Fix dealing with ext-redis' multi/exec returning a bool
  [Messenger][Amqp] Added missing rpc_timeout option
  [Serializer] Prevent GetSetMethodNormalizer from creating invalid magic method call
  [HttpFoundation] Fix dumping array cookies
  [WebProfilerBundle] Fix dump header not being displayed
  TraceableHttpClient: increase decorator's priority
  Use static methods inside data providers
  [FrameworkBundle] Allow configuring `framework.exceptions` with a config builder
  bug #48313 [Mime] Fix MessagePart serialization
  [HttpKernel][ErrorHandler] Fix reading the SYMFONY_IDE env var
  [ErrorHandler][DebugClassLoader] Fix some new return types support
  Fix getting the name of closures on PHP 8.1.11+
  [Translator] Fix typo "internal" / "interval"
  fix dumping top-level tagged values
  [Console] Fix clear line with question in section
nicolas-grekas added a commit that referenced this issue Dec 14, 2022
@nicolas-grekas
Merge branch '6.2' into 6.3
79547b0 
* 6.2: (22 commits)
  [Mailer] Include all transports' debug messages in RoundRobin transport exception
  [FrameworkBundle] fix: fix help message
  Hide excluded services from container debug list
  [Validator] Allow opt-out of EmailValidator deprecation when using Validation::createValidatorBuilder()
  Fix HtmlSanitizer default configuration behavior for allowed schemes
  Use relative timestamps
  [Translation] add tests + fix
  [Cache] Fix dealing with ext-redis' multi/exec returning a bool
  [Messenger][Amqp] Added missing rpc_timeout option
  [Serializer] Prevent GetSetMethodNormalizer from creating invalid magic method call
  [HttpFoundation] Fix dumping array cookies
  [WebProfilerBundle] Fix dump header not being displayed
  TraceableHttpClient: increase decorator's priority
  Use static methods inside data providers
  [FrameworkBundle] Allow configuring `framework.exceptions` with a config builder
  bug #48313 [Mime] Fix MessagePart serialization
  [HttpKernel][ErrorHandler] Fix reading the SYMFONY_IDE env var
  [ErrorHandler][DebugClassLoader] Fix some new return types support
  Fix getting the name of closures on PHP 8.1.11+
  [Translator] Fix typo "internal" / "interval"
  ...
PhilETaylor pushed a commit to PhilETaylor/symfony that referenced this issue Sep 6, 2023
@nicolas-grekas
Merge branch '5.4' into 6.0
1113c4b 
* 5.4:
  [Mailer] Include all transports' debug messages in RoundRobin transport exception
  [FrameworkBundle] fix: fix help message
  Use relative timestamps
  [Cache] Fix dealing with ext-redis' multi/exec returning a bool
  [Messenger][Amqp] Added missing rpc_timeout option
  [Serializer] Prevent GetSetMethodNormalizer from creating invalid magic method call
  [HttpFoundation] Fix dumping array cookies
  [WebProfilerBundle] Fix dump header not being displayed
  TraceableHttpClient: increase decorator's priority
  Use static methods inside data providers
  [FrameworkBundle] Allow configuring `framework.exceptions` with a config builder
  bug symfony#48313 [Mime] Fix MessagePart serialization
  [ErrorHandler][DebugClassLoader] Fix some new return types support
  Fix getting the name of closures on PHP 8.1.11+
  [Translator] Fix typo "internal" / "interval"
  fix dumping top-level tagged values
PhilETaylor pushed a commit to PhilETaylor/symfony that referenced this issue Sep 6, 2023
@nicolas-grekas
Merge branch '6.0' into 6.1
6e1416f 
* 6.0:
  [Mailer] Include all transports' debug messages in RoundRobin transport exception
  [FrameworkBundle] fix: fix help message
  Use relative timestamps
  [Cache] Fix dealing with ext-redis' multi/exec returning a bool
  [Messenger][Amqp] Added missing rpc_timeout option
  [Serializer] Prevent GetSetMethodNormalizer from creating invalid magic method call
  [HttpFoundation] Fix dumping array cookies
  [WebProfilerBundle] Fix dump header not being displayed
  TraceableHttpClient: increase decorator's priority
  Use static methods inside data providers
  [FrameworkBundle] Allow configuring `framework.exceptions` with a config builder
  bug symfony#48313 [Mime] Fix MessagePart serialization
  [ErrorHandler][DebugClassLoader] Fix some new return types support
  Fix getting the name of closures on PHP 8.1.11+
  [Translator] Fix typo "internal" / "interval"
  fix dumping top-level tagged values
PhilETaylor pushed a commit to PhilETaylor/symfony that referenced this issue Sep 6, 2023
@nicolas-grekas
Merge branch '6.1' into 6.2
12a25d0 
* 6.1:
  [Mailer] Include all transports' debug messages in RoundRobin transport exception
  [FrameworkBundle] fix: fix help message
  Fix HtmlSanitizer default configuration behavior for allowed schemes
  Use relative timestamps
  [Cache] Fix dealing with ext-redis' multi/exec returning a bool
  [Messenger][Amqp] Added missing rpc_timeout option
  [Serializer] Prevent GetSetMethodNormalizer from creating invalid magic method call
  [HttpFoundation] Fix dumping array cookies
  [WebProfilerBundle] Fix dump header not being displayed
  TraceableHttpClient: increase decorator's priority
  Use static methods inside data providers
  [FrameworkBundle] Allow configuring `framework.exceptions` with a config builder
  bug symfony#48313 [Mime] Fix MessagePart serialization
  [HttpKernel][ErrorHandler] Fix reading the SYMFONY_IDE env var
  [ErrorHandler][DebugClassLoader] Fix some new return types support
  Fix getting the name of closures on PHP 8.1.11+
  [Translator] Fix typo "internal" / "interval"
  fix dumping top-level tagged values
  [Console] Fix clear line with question in section
PhilETaylor pushed a commit to PhilETaylor/symfony that referenced this issue Sep 6, 2023
@nicolas-grekas
Merge branch '6.2' into 6.3
9d4c505 
* 6.2: (22 commits)
  [Mailer] Include all transports' debug messages in RoundRobin transport exception
  [FrameworkBundle] fix: fix help message
  Hide excluded services from container debug list
  [Validator] Allow opt-out of EmailValidator deprecation when using Validation::createValidatorBuilder()
  Fix HtmlSanitizer default configuration behavior for allowed schemes
  Use relative timestamps
  [Translation] add tests + fix
  [Cache] Fix dealing with ext-redis' multi/exec returning a bool
  [Messenger][Amqp] Added missing rpc_timeout option
  [Serializer] Prevent GetSetMethodNormalizer from creating invalid magic method call
  [HttpFoundation] Fix dumping array cookies
  [WebProfilerBundle] Fix dump header not being displayed
  TraceableHttpClient: increase decorator's priority
  Use static methods inside data providers
  [FrameworkBundle] Allow configuring `framework.exceptions` with a config builder
  bug symfony#48313 [Mime] Fix MessagePart serialization
  [HttpKernel][ErrorHandler] Fix reading the SYMFONY_IDE env var
  [ErrorHandler][DebugClassLoader] Fix some new return types support
  Fix getting the name of closures on PHP 8.1.11+
  [Translator] Fix typo "internal" / "interval"
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug Mime Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@nicolas-grekas @Amunak @carsonbot