In several places, Mime Mail needs to determine if a URL or a file attachment is an image. Currently, a regular expression is used to do this. This is a problem for several reasons:
Drupal 8 has a perfectly good service 'file.mime_type.guesser' which is an implementation of the Symfony MimeTypeGuesserInterface. This service provides a way to determine the MIME type of a file that is way more capable than our simple regular expression, and since it is core Drupal / Symfony it is being used and updated and supported by way more people. (Mime Mail's regular expression hasn't changed in 10 years).
A future problem is that Mime Mail is currently trying to determine the MIME type in four distinct places - we should eventually refactor this code so that doesn't have to happen, but the first step is to do the type determination the same way in all four places so that at least we always get the same answer.
| Comment | File | Size | Author |
|---|---|---|---|
| #10 | 3155325-10-type-guesser.patch | 5.3 KB | tr |
| #10 | interdiff-8-10.txt | 1.5 KB | tr |
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #2
tr commentedComment #4
tr commentedThe test correctly fails because the mime type guesser only accepts paths, not URLs with query strings on the end. This points out a problem with URL handling in Mime Mail, because we freely intermix relative paths, absolute paths, stream wrapper paths, URLs, URIs, etc. and perform our own custom parsing on all of these.
We need to make this uniform and use core Drupal classes to handle parsing of the URL parts. But that's another issue. Here, we just need to make sure we pass a valid value to the type guesser so that it can do its job.
Comment #5
tr commentedHere's a re-roll of the patch in #2 against the current HEAD. I expect the tests will still fail for the reasons I described in #4, because nothing has been done to fix that.
Comment #7
tr commentedCorrected two @var declarations to use the current version of Symfony.
Comment #8
tr commentedHave to also update
guess()to beguessMimeType()for D10. Becauseguess()was deprecated in D9.1 this change will also work with our current minimum core version of 9.3.Note the fact that the D9 tests did not catch this is a direct result of our lack of testing for certain key methods. In this case,
guess()is used inMimeMailFormatHelper::mimeMailFileandMimeMailFormatHelper::mimeMailMultipartBody, but neither of those are tested. See:#3145400: Add test cases for MimeMailFormatHelper::mimeMailFile()
#3145401: Add test cases for MimeMailFormatHelper::mimeMailMultipartBody()
Comment #9
tr commentedOK, #8 fixed the new errors related to
guess(), so we're back to needing to fix the one test failure described in #4.Comment #10
tr commentedRe-rolled against HEAD and included a fix for #4.
This issue is blocking a D10 release because
MimeTypeGuesserInterface::guess()has been removed in D10. This patch replaces that usage.Comment #12
tr commentedWell that did fix the issue from #4 but it caused another issue, which I will now have to look into.
Comment #13
tr commentedComment #16
scott_earnest commentedUpdates rolled into issue fork.
Unfortunately, PHPUnit is throwing an error, need to look into this:
https://git.drupalcode.org/issue/mimemail-3155325/-/jobs/2799865
---- Drupal\Tests\mimemail\Functional\MimeMailWebTest ----
Otherwise this works for me in Drupal 10 and Drupal 11.
Comment #17
scott_earnest commentedComment #18
jfurnas commentedEven with this patch in place, I am still getting the same error message related to using 'guess' as the method name.
Comment #19
luke.leberSo to summarize the immediate problem to move this forward, when trying to determine if something is an image, there's a string input that's basically in a random format. It might be a
scheme://path.extesion?query#fragmentwith any part present, not present, or even malformed. Anticipate that the string might even be something like a string of random emojis and some unprintable / null control characters, or even characters that aren't allowed in a URL at all that may or may not be encoded - Some of these methods seem to be general utility methods which may be called outside of Mime Mail given that the utility class is not marked @internal!.This string has to be either:
Even given Drupal's URL utilities, that's likely extremely tricky as this issue stands. IMO it might be worth checking the various call sites and seeing if there's an opportunity to normalize things. That would of course likely need a B/C break in the utility class, but I do fear that tackling the problem naively may be a very disturbing game of whack-a-mole in the wild. Passing a nullable
\Drupal\Core\Urlobject into the various utility methods would make this a trivial problem to solve.Comment #20
drupgirl commentedThis issue could reduce its scope by remove the work that has been accomplished in https://www.drupal.org/project/mimemail/issues/3155325.
Comment #22
andras_szilagyi commentedI've added more test coverage for us to see the way mimeMailUrl behaves with the scenarios mentioned in #19, after I refactored the image detection using a centralized method that uses more core API
Comment #23
andras_szilagyi commentedready for review
Comment #24
saidatomGreat! Looks good, moving to RTBC.
Comment #25
mark_fullmerAssigning to myself for a second review...
Comment #27
mark_fullmerThanks, all! I functionally tested these improvements, and the new test coverage looks comprehensive. This is merge and will be included in the next release.