NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[Mime] Fix attached inline part not related#43255

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.

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

Already on GitHub?Sign in to your account

Jump to bottom

Closed

keksa wants to merge1 commit intosymfony:4.4fromkeksa:mime-attached-inline-part-not-related

Closed

[Mime] Fix attached inline part not related#43255

keksa wants to merge1 commit intosymfony:4.4fromkeksa:mime-attached-inline-part-not-related

Conversation

Copy link

Contributor

keksa commentedSep 30, 2021•
edited by fabpot
Loading

Q	A
Branch?	4.4
Bug fix?	yes
New feature?	no
Deprecations?	no
Tickets	Closes#42921
License	MIT
Doc PR	-

Attaching an inlineDataPart to email does not generate aRelatedPart when generating body, but onlyMixedPart.

The following code should work the same, but does not:

$email =newEmail();$email->embed($image,'test.gif');// vs.$email =newEmail();$email->attachPart((newDataPart($image,'test.gif'))->asInline());

This PR fixes the behavior.

carsonbot added Status: Needs Review Bug Mime labels

Sep 30, 2021

carsonbot added this to the4.4 milestone

Sep 30, 2021

[Mime] Fix attached inline part not related

2d69268

keksa force-pushed themime-attached-inline-part-not-related branch from9fcbc3d to2d69268Compare

September 30, 2021 11:53

Copy link

ContributorAuthor

keksa commentedSep 30, 2021

failing test looks unrelated

flack mentioned this pull request

Nov 24, 2021

Mime: Email::embed doesn't work for background images#42921

Closed

Copy link

Contributor

flack commentedNov 24, 2021

just FYI, I tested this change locally, and it fixes the issue mentioned here:#42921 (comment)

stof reviewed

Nov 24, 2021

View reviewed changes

Copy link

Member

stof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I think we should also fix the issue that happens when using->embed(). This one adds an attachment that hasinline: true, but this does not make it go into the$inlineParts list.

src/Symfony/Component/Mime/Tests/EmailTest.php

		$this->assertCount(2,$parts =$body->getParts());
		$this->assertStringMatchesFormat('html content <img src=3D"cid:%s@symfony">',$parts[0]->bodyToString());

		$e = (newEmail())->from('me@example.com')->to('you@example.com');

Copy link

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

would be better to create a new test instead of adding one more subtest into this test which does not respect the testing best practices. this has several advantages:

the tests will run independently of other tests for the body generation (instead of running only when previous assertions of that god test have passed)
the test will be named, which will make it clear what is being tested

fabpot previously approved these changes

Nov 25, 2021

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels

Nov 25, 2021

stof reviewed

Nov 25, 2021

View reviewed changes

src/Symfony/Component/Mime/Email.php

		foreach ($namesas$name) {
		if (isset($attachment['part'])) {
		if ('inline' ===$attachment['part']->getPreparedHeaders()->getHeaderBody('Content-Disposition')) {
		$inlineParts[] =$attachment['part'];

Copy link

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

inline parts is an array indexed by name.

src/Symfony/Component/Mime/Email.php

		foreach ($this->attachmentsas$attachment) {
		foreach ($namesas$name) {
		if (isset($attachment['part'])) {
		if ('inline' ===$attachment['part']->getPreparedHeaders()->getHeaderBody('Content-Disposition')) {

Copy link

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

this should be done before the loop over names (to do it even when there is no names being found for<img> tags) and it needs to replace thecid references in the HTML too. Otherwise, the fix is incomplete.

and as said in my comment, we also need a fix for attachments usinginline: true without a part already.

Copy link

Contributor

flackNov 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

@stof maybe I'm missing something, but aren't inline parts handled by this already?

symfony/src/Symfony/Component/Mime/Email.php

Lines 485 to 488 in7c531f5

	$attachment['inline'] =true;
	$inlineParts[$name] =$part =$this->createDataPart($attachment);
	$html =str_replace('cid:'.$name,'cid:'.$part->getContentId(),$html);
	$part->setName($part->getContentId());

Or do you mean that someone callsembed but the regex doesn't find the$name in the HTML body so that it drops out of the loop here?

symfony/src/Symfony/Component/Mime/Email.php

Lines 479 to 481 in7c531f5

	if ($name !==$attachment['name']) {
	continue;
	}

Because in that case (at least in my uninformed opinion) it seems correct to not make it an inline attachment, since it's actually not embedded anywhere.

Copy link

Member

stofNov 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

@flack no. this handles attachments that are added with a name that matches a name detected in the HTML. It does not handle the case of attachments that are added asinline: true explicitly through->embed().

And$names is not detecting all usages of CID in the HTML (that's precisely the issue you are marking as closed by this PR, which isnot solved by your PR)

Copy link

Contributor

flackNov 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

@stof well, I only said this fixes the issuein the second comment of#42921, i.e. the allows me to build a workaround for the original problem in that ticket. It was@fabpot who added "closes#42921" to the description here, don't blame me for that :-)

But I still don't get it: Attachments that are added asinline: true explicitly through->embed() cannot have$attachment['part'] set:

symfony/src/Symfony/Component/Mime/Email.php

Lines 353 to 358 in7c531f5

	publicfunctionembed($body,string$name =null,string$contentType =null)
	{
	$this->attachments[] = ['body' =>$body,'name' =>$name,'content-type' =>$contentType,'inline' =>true];

	return$this;
	}

So the handling of those doesn't change in this PR. Of course, if they were ignored before (like in the OP of#42921), they are still ignored. But that seems like a separate issue to me. This PR fixes a bug where you can't add an instantiatedDataPart as inline. The other ticket is about the name detection logic being too narrow.

Copy link

Member

stofNov 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

So the handling of those doesn't change in this PR.

and that's precisely what I'm asking to change, by actually fixing the reported issue instead of fixing only the workaround

fabpot dismissed theirstale review

November 25, 2021 13:54

fabpot removed the Status: Reviewed label

Nov 25, 2021

Copy link

Contributor

flack commentedFeb 3, 2022

@stof in the interest of moving this forward, could you outline what exactly you would expect this PR to do in order to be acceptable? If@keksa lost interest, I might give it a try. See also the newer comments in#42921, people are stumbling across this in the wild, so it would be good to come up with a solution, esp. since Swiftmailer ist abandoned

Copy link

Member

fabpot commentedFeb 4, 2022

@flack I think we need to have a fix that covers all cases, and that's what@stof explained in the comments. If you have time to have a look at that, that would be wonderful as I would love to have a PR that is mergeable for the referenced issue.

Copy link

Contributor

flack commentedFeb 4, 2022

@fabpot the way I see it, there are three issues:

Mime\Email only embeds intoimg tags (the original issue inMime: Email::embed doesn't work for background images #42921). This could be solved by making the regex looser, so that it doesn't only matchimg andsrc. I don't know if that would be acceptable as looser matching could have side effects
asInline parts added withattachPart don't show up ininlineParts. That is solved by[Mime] Fix attached inline part not related #43255 (this PR)
attachments added withembed that are not found by CID in the HTML are not treated as inline. That is IIUC the issue@stof was talking about above. Not sure what the solution to this should look like (hence my question)

Then, there ist also the suggestion in this comment:#42921 (comment) which asks for asetContentId() method on theDataPart. I've had that idea before, too, and I think it could work, but I would have to re-check.

So, I don't know. I could make a PR that proposes a solution for 1 and incorporates this PR as a solution for 2, but I don't know what to do about 3. What would be the correct behavior here?

Copy link

Contributor

flack commentedFeb 4, 2022

Reading all the comments again, It sounds to me like the desired logic seems to be

attachments (bothDataPart and array variant) withinline: true should go toinlineParts unconditionally
the others go through the regex matching magic as it currently exists already

So we could

loop over all attachments first and populateinlineParts with the ones that are marked as inline
find candidate names by regex, look for equally named attachments and add them toinlineParts, too
loop over the collectedinlineParts to do the CID replacement in HTML body

Is that roughly what you had in mind,@stof?

Copy link

Member

fabpot commentedFeb 4, 2022

You can definitely do a separate PR for 1., I would only cover your use case to avoid matching too much.

flack mentioned this pull request

Feb 9, 2022

[Mime] Fix embed logic for background attributes#45376

Merged

fabpot added a commit that referenced this pull request

Feb 10, 2022

feature#45376[Mime] Fix embed logic for background attributes (flack)

f12943d

This PR was submitted for the 4.4 branch but it was squashed and merged into the 6.1 branch instead.Discussion----------[Mime] Fix embed logic for background attributes| Q             | A| ------------- | ---| Branch?       | 4.4| Bug fix?      | yes| New feature?  | no| Deprecations? | no| Tickets       |Fix#42921| License       | MIT| Doc PR        |As suggested by@fabpot here#43255 (comment), this is the most minimal fix for the linked issue. I guess the same problem can happen if you use e.g.```html<div></div>```I have changed it so that there is now an array of regexes to look for embedded images, so at least code for the `background-image` case (or others that might be found later on) can easily be added.Commits-------d4a87d2 [Mime] Fix embed logic for background attributes

Copy link

Member

nicolas-grekas commentedMar 29, 2022•
edited
Loading

What's the status or the next step here?

Copy link

Contributor

flack commentedJun 21, 2022

@nicolas-grekas speaking for myself, I'm still stuck on Swiftmailer due to this issue (and due to the fact that my fix#42921 only landed in 6.1+, which I won't be able to use in the next two or three years due to other dependencies).

I would be open to working on this issue if there are clear instructions on how an acceptable solution should look like, and if there are assurances that the fix would get backported to the 5.x series, because otherwise it's not all that useful to me

Copy link

Member

fabpot commentedJul 17, 2022

Somewhat related to this PR (mentioned at least in a comment),#46962 addsDataPart::setContentId().

Copy link

Contributor

flack commentedJul 17, 2022

@fabpot thx for the mention! Unfortunately, I had tried thesetContentId method already (with the method mentioned in#42921 (comment)), but it's not enough: The problem is that the attachment doesn't end up in$inlineParts because of

symfony/src/Symfony/Component/Mime/Email.php

Lines 507 to 509 in5bc5827

	if (isset($attachment['part'])) {
	continue;
	}

So there won't be anyRelatedPart created for it

Copy link

Member

fabpot commentedJul 17, 2022

See#46963 for another PR trying to fix the issue.
@flack Can you confirm that it works for you?

Copy link

Contributor

flack commentedJul 18, 2022

@fabpot I took your patch and applied it to my symfony/mime 5.4.10 installation, and so far it looks good! All the inline attachments now show up as expected. Thanks again!

flack mentioned this pull request

Jul 18, 2022

[Mime] Fix inline parts when added via attachPart()#46963

Merged

fabpot closed this

Jul 19, 2022

keksa deleted the mime-attached-inline-part-not-related branch

September 7, 2022 12:58

Labels

Bug Mime

Movatterモバイル変換

Uh oh!

[Mime] Fix attached inline part not related#43255

[Mime] Fix attached inline part not related#43255

Uh oh!

Conversation

keksa commentedSep 30, 2021• edited by fabpotLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

keksa commentedSep 30, 2021

Uh oh!

flack commentedNov 24, 2021

Uh oh!

stof left a comment

Choose a reason for hiding this comment

Uh oh!

stofNov 24, 2021

Choose a reason for hiding this comment

Uh oh!

stofNov 25, 2021

Choose a reason for hiding this comment

Uh oh!

stofNov 25, 2021

Choose a reason for hiding this comment

Uh oh!

flackNov 25, 2021

Choose a reason for hiding this comment

Uh oh!

stofNov 25, 2021

Choose a reason for hiding this comment

Uh oh!

flackNov 25, 2021

Choose a reason for hiding this comment

Uh oh!

stofNov 25, 2021

Choose a reason for hiding this comment

Uh oh!

flack commentedFeb 3, 2022

Uh oh!

fabpot commentedFeb 4, 2022

Uh oh!

flack commentedFeb 4, 2022

Uh oh!

flack commentedFeb 4, 2022

Uh oh!

fabpot commentedFeb 4, 2022

Uh oh!

nicolas-grekas commentedMar 29, 2022• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

flack commentedJun 21, 2022

Uh oh!

fabpot commentedJul 17, 2022

Uh oh!

flack commentedJul 17, 2022

Uh oh!

fabpot commentedJul 17, 2022

Uh oh!

flack commentedJul 18, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

keksa commentedSep 30, 2021•
edited by fabpot
Loading

nicolas-grekas commentedMar 29, 2022•
edited
Loading