Hey!

Thanks for your PR. You are targeting branch "6.2" but it seems your PR description refers to branch "6.3 for features / 4.4, 5.4, 6.0, 6.1, or 6.2 for bug fixes ".
Could you update the PR description or change target branch? This helps core maintainers a lot.

Cheers!

Carsonbot

Hey!

I think @MasterRO94 has recently worked with this code. Maybe they can help review this?

Cheers!

Carsonbot

Thank you @ovrflo.

@ovrflo This PR is a huge breaking change as it breaks the lazy load feature.

When this File class was introduced in #47462 the whole point was to keep the lazy load feature.

This PR simplifies all of that and introduces a way to have a file for TextPart as well (via the new file:// notation) and it keeps the lazy-loading feature which was why those methods were introduced in the first place.

So before the change in this PR only the file path got serialized into the Messenger message after attaching an attachment to the email via \Symfony\Component\Mime\Email::attachFromPath. After this PR it doesn't matter anymore if \Symfony\Component\Mime\Email::attach or \Symfony\Component\Mime\Email::attachFromPath was used as the entire file contents will be serialized into the Messenger message. We just updated our Symfony mailer and mime packages from 5.4 to 6.3 and it completely broke our application since the maximum allowed message payload size for AWS SQS is only 256 KiB and pretty much all our attachments hit that limit on their own so essentially all email sending is broken. We have switched to another queue driver for the moment but I think that this PR never should've gotten merged and that another solution needs to be found for the original issue that this PR tried to fix.

cc @nicolas-grekas @fabpot

Hi @X-Coder264

My rationale was that we can't make assumptions about the machine receiving the message. That machine may not have access to any of those files or, at least, not on the same paths. While this happens to work in your use-case, it may not be valid for all implementations. Maybe what we need is a LazyFile implementation, backed by tests that guarantee it only stores the path and assertions that blow up when files doesn't exist?

I didn't know about this laziness feature when I was trying to fix the bug. Maybe it's just me, but when serializing the email I would expect it to be in a complete state that doesn't rely on a specific filesystem. I understand and fully appreciate the usefulness of such a feature, though I wouldn't have this as the default as it would break somebody else's expectations. Hence, I propose adding an explicit way of attaching lazy files.

I'm looking forward to hearing maintainers' thoughts on this and I can also help with drafting a PR to fix this (as soon as we agree on a path moving forward).

@ovrflo The issue that this PR was supposed to fix was just the BadMethodCallException that happens during the email unserialization, the issue was NOT that the attachFromPath / File implementation makes assumptions about the machine consuming the message (quite the opposite - that was the intended implementation from the beginning of that feature -> https://github.com/symfony/symfony/blob/v5.4.27/src/Symfony/Component/Mime/Part/DataPart.php#L64-L70).
So while this PR was supposed to be a bug fix PR it's turned out into a feature PR that introduced a big breaking change.

What this PR does is that it essentially makes the attachFromPath behave in the same way as the attach method (so attachFromPath basically has no reason for existing anymore in the current codebase) aka both include the entire file contents in the serialized message so by doing that it completely kills the entire point of the attachFromPath method which was to be able to serialize only the file path, not the entire file content. So this seems more of a documentation issue where the documentation just needs to mention that if the machine consuming the message does not have access to the file path that you need to use the attach method. Or if we really want that attachFromPath can behave like the attach method then we need a new class or a new flag when calling attachFromPath or something - and whatever that is it needs to be done in a non breaking way as Symfony allows breaking changes only on major versions (and 6.2 was not a major version).

@X-Coder264 the mailer component can be used without serializing the message (when you don't use the messenger integration). In such case, attaching a path will still read the file only when needed.

(quite the opposite - that was the intended implementation from the beginning of that feature

The link you gave is a link to the constructor. Deserialization does not involve the constructor, so I don't understand your sentence. In the old implementation, serializing a message on one machine and deserializing it on the other one (which is the expected use case of using messenger to actually send the message instead of doing it synchronously) would give you a broken message that cannot be sent.

@stof The link I gave is just to demonstrate that the attachFromPath API even on 5.4 and right until this change hit in 6.2 always worked on the premise that the attached File instance had to work with the is_file, is_readable and fopen PHP native functions so the file had to be accessible on the machine that sends the email (whether that's the same machine when sending the email without Messenger or a potentially different machine that consumes the message when using the Messenger integration is irrelevant). In our project setup we run workers on different machines and we had to setup a shared folder between the machines serving HTTP traffic and the machines on which the workers run so that the file can be read from there (as that was a known behavior of the attachFromPath method in both 5.4 and 6.1 so everyone using it with the Messenger integration had to do it that way).

This was a bug fix PR and the linked issue (#47991) that this PR was supposed to fix was the unserialization of the email which was broken due to #47462, but the way it was fixed has broken that original premise that you've mentioned that the file would be read only when actually needed. The linked issue is not about how attachFromPath works and what "assumptions" it makes, but nonetheless this behavior has been changed here and it was done in breaking way which as per Symfony policies is not allowed on a minor version (updating from 6.1 to 6.2 breaks our app).

So before this PR when using the Messenger integration the file would be read on the consumer side as the message would only have the path in it (so that means that the file read operation happens AFTER the message is already on the queue). After this change the entire file content becomes part of the message (so the file is read BEFORE the message gets dispatched to the queue).

I've sent a PR that fixes #47991 without the breaking change that this PR introduced.

PR link -> #51489