in2code-de / luxletter

Newsletter system for TYPO3
https://www.in2code.de/agentur/typo3-extensions/luxletter/
23 stars 25 forks source link

[FEATURE] image embed without duplicates #134

Closed rr-it closed 2 years ago

rr-it commented 2 years ago

Embed each unique image only once to save on time for mail generation and on total size of mail.

This fixes https://github.com/in2code-de/luxletter/issues/133

rr-it commented 2 years ago

The newly added unit test fails - as expected:

https://github.com/in2code-de/luxletter/runs/7913400328?check_suite_focus=true#step:5:18

 1) In2code\Luxletter\Tests\Unit\Domain\Service\BodytextManipulation\ImageEmbedding\ExecutionTest::testGetImages
Failed asserting that 4 matches expected 2.
rr-it commented 2 years ago

The newly added unit test for no duplicates passes.

Everything is working correctly, but some old unit tests fail now caused by new CID syntax: cid:name_00000001 --> cidname_0123456789abcdef0123456789abcdef

Please give feedback, if this feature " image embed without duplicates" should be implemented. If confirmed, I will go on fixing the unit tests.

einpraegsam commented 2 years ago

Hi,

first of all I like your approach and would tend to merge such a feature - thx for your time. I just tested your PR by duplicating a textmedia content element with the same image. After that I created a newsletter and use the testmail function. Now the second image is not shown any more in the mail: duplicate

In addition the test (see above) are faiiling and this should be fixed.

Alex

rr-it commented 2 years ago

Now the second image is not shown any more in the mail:

This is just a bug in mailhog: https://github.com/mailhog/MailHog-UI/commit/d245cfbf4721c3325f64d92f93fa3169b08b1702

Real world mail clients like Thunderbird, Outlook, K9-Mail and also webmail-clients like Roundcube and commercial webmail service providers handle multiple uses of the same CID correctly.

einpraegsam commented 2 years ago

Thx, I'm going to check this, can you please meanwhile have a look to my second argument: "In addition the test (see above) are faiiling and this should be fixed."

einpraegsam commented 2 years ago

Thx for your proposal, I just merged your PR