Broken images URLs - missing itok

I don't think #6 is the right way either. The itok is needed when using images stored in the private file system. I have a site where the image caches are already cached, but without the itok I get a Access Denied, but with the itok they work fine. This is with Open Atrium which stores all images in the private file system and implements access control to ensure a user has access to the image.

So I'm back to #7 where:

I think that we should never remove the itok from a URL.

I'll probably post a patch that un-does the #2312747: URLs to linked images broken patch just so I can run a new build of Atrium that uses the latest release of mimemail since we need the latest security updates.

I'm also tempted to mark it as "major" because this was a regression and effects more people than mentioned in #5 if they are using the private file system.

Here is the patch to undo #2312747: URLs to linked images broken until/unless a better solution is found.

Hmm, actually this is worse than I thought. As mentioned in #5, this affects images no matter what. So I had to completely remove the preg_replace for itok. So here is the new patch that just eliminates the removal of itok.

#11 works for me, but I haven't tested all your patch.
"itok" parameters are preserved and it's mainly what is required in my case.
Thanks !

#11 works fine. But we needed images from private filesystem and extended patch #11.

@AdamPS: In _mimemail_file you try to open the file. But in case of private filesystem the filepath ist 'system/files/myfile.txt' and you cannot open this file in local filesystem. I replace 'system/files/' with 'private://' an so drupal can find the correct path for private file:
$file = drupal_realpath($local_path);

+ // Check whether private file
+ $local_path = preg_replace("!^system/files/!", 'private://', $local_path);

Todo: add handling for language prefix

I've just tested the patch in #18; it didn't apply cleanly to the latest release, but it was easy enough to make the changes manually. Everything is working well and it solves the issue we were having with itok parameters getting stripped. Thank you for the patch!

Thank you @AdamPS for your feedback. I created a new issue https://www.drupal.org/node/2860557 and added a new patch based on your current patch no. 14.

Changing this to "needs work" because according to #19 it does not apply to the latest release.

In fact the patch from #22 solves the issue cleanly. A new dev release should be created ASAP. Thank you all. RTBC!

I confirm that the patch works well and fix broken images URLs in emails. The generation of the itok parameter in the url works as expected with the patch.

Would be great to get it merged in a new release =) .

I adapted patch #18 to new version 7.x-1.1
I will also upload a new patch for private files in #2860557.

The last patch in #26 works for me.

#26 works for me too.

Resetting status to RTBC. Any chance to get this committed finally?

"Patch Failed to Apply"

The patch in #22 failed tests, the patch in #26 does not apply. So not RTBC.

At a minimum, we need a patch that applies and passes the tests.

Hi @TR, sorry my mistake. We were using the patch from #22 in several projects since 2017 successfully but I didn't look close enough into the results and thought it was an unrelated error. Attached is the rerolled patch against latest 7.x-1.x-dev. Hopefully testbot likes it :)

Could someone else now review this?

OK, well here's the current situation as I see it:

There are a number of other open and closed issues about itok. I added some related issues to collect them all here. All of them are at least somewhat related and touch the same code. I have no desire to fix one, only to have it break the others. I think any patch here should take into account all that information and make sure that any change we make here fixes those other issues as well. And the only way to ensure that is to have test cases which test the _mimemail_file() and _mimemail_url() functions so that we can be convinced that these and future changes are doing the right things.

The problem with tests is, I really don't have a clue as to what *exactly* these methods are supposed to do: _mimemail_file() is documented as "Helper function to extract local files." and _mimemail_url() as "Helper function to format URLs.". Crap documentation leads to crap code, and that's what we currently have. We can't say the current code is wrong or right unless we know what it's *supposed* to do. In particular, functions should never be designed so that their functionality and return types depends on which input parameters you pass - each of these functions should probably be split into two or three different functions which do one and only one thing each.

I made a lot of progress on this code in D8 and D9, and I documented the MimeMailFormatHelper::mimeMailFile() method which is the successor to _mimemail_file(). I didn't refactor the method yet - it is still trying to do way too much and do many different things, but at least its expected behavior is written down so it can be tested (that's the next step, see #3145400: Add test cases for MimeMailFormatHelper::mimeMailFile()). Reading that documentation makes it clear what I said above. For example, it returns a string content id or a complex array indexed by content id or a string containing a URL which is indistinguishable from the string content id unless you parse the string trying to identify it as a URL. How are you supposed to meaningfully use a method like this? And why is it called mimeMailFile if it's used for images? There are many issues related to this method that can't be fixed until there are tests to show it's doing those things correctly (or not).

Another big problem is that D7 is nearing its end of life, while D8 and D9 are in active development. As a matter of both principle and practice, it is wrong to fix this first in D7. It really should be fixed in D8/D9 first, then any changes should be backported to D7 if there is community interest. Fixing D7 first is almost certainly going to mean that it will never get fixed in D8/D9. I already have way too much work to do to try to figure out all the other D7 fixes that didn't make it into D8/D9 - I don't need yet another one. When the standard Drupal practices of fixing HEAD first and requiring a test case for each feature/bug fix aren't followed, projects become impossible to properly maintain. That is what happened here. The itok changes that were previously made didn't have tests, for example, so we have no way of demonstrating that those changes were needed, that those changes worked properly, and that the current patch doesn't break what the previous change supposedly fixed.

Likewise, this patch makes an API change, which is generally not allowed or desired between minor versions. API changes should be limited to major releases, by definition. If we are going to change the API and/or refactor these functions to make more sense, it should be done in a new major branch. This is why my D8/D9 work has been limited to documenting and writing tests for the code rather than refactoring - I want to get a stable release made before branching a new major version and switching to semantic versioning.

At this point my feeling is that this and the other itok issues need to be moved to 8.x-1.x, and that the fix should be done in 8.x-1.x first, accompanied by proper testing of the functions involved. This includes a test that demonstrates the problem in the current code so that we can evaluate whether a proposed patch fixes that problem, but also we need complete tests to make sure these functions are doing what they're supposed to.