Properly convert file URLs to paths

Well, I did read through a few issues, but I could never determine if they were really experiencing the same problem as me. Anyway, I've got a second version here. I think I'm done with new versions, so please review this one. This one includes code to prevent people from ../ing their way out of drupal. The helper function can also translate relative paths when given a path to base them off of. This should come in handy when implementing CSS parsing.

This is a new issue, but what its doing, taking a file name from the user, reading it off the server and spitting it into an email, probably needs more safeguarding. I think you could, for instance, email yourself a config file with a carefully crafted image tag. Unless filtering already exists in the code and I just need to look harder.

Well, its a lot shorter. I guess rejecting any url with a .. in it could work, but there are valid uses for it. Themes might use it in their stylesheets, for instance. I guess that could become a separate patch if its needed?

Well, I tested it. It had a parenthesis in the wrong place, but after that it worked. It no longer attaches files with absolute URLs, but I guess that was intended. (though I thought we really did want to attach everything?) I'm a fan of functions with well defined inputs and outputs; your function sometimes returns NULL, sometimes returns paths, and sometimes returns urls. I changed it so that it either returns a path if it could be found, or NULL if not. The NULL will cause _mimemail_file() to pass the URL through anyway. Returning the URL as a filesystem path will still cause some servers with url_fopen on to potentially download files from itself and attach them.

Anyway, this works on my system, but I did make a few minor changes (its even shorter now), so someone else should probably test it too.

Hmm, issue when $relpath is set and the URL is absolute.

I really need to read my code more carefully.. This is my final answer.

Just took a quick look, and I think we need a urldecode(). I can't test or make a patch right now, so someone else might have to take a look. And theres some minor code style violations. there needs to be a newline before an else, and 'if(' should be 'if ('.

Works great on my subdirectory test system. Further testing shows that a urldecode() is in fact needed, because otherwise it will look for a file containing the %20 or whatever instead of a space, so I added that in. Also fixed the code style issues.

Jerdavis, I just don't understand the insistence on using url(). Its API documentation is "Generate a URL from a Drupal menu path". That's completely not what we need. All we need to do is convert a domain-relative URL to an absolute one. The most direct way to do this is simply prepend "http://www.domain.com" which is exactly what my previous patch does. I guess if you tested it enough we can hack it in there, but it seems like we had to devote more code to fixing the things that we don't need url() to do than it would take to simply code what we do need to do myself (my patch turned a domain relative into an absolute in two lines of code). But, I'm all for getting a d6 out, so I say stick with it if it works.

If the answer to the above is that you'd really rather be the only one contributing code, please let me know and I'll stick to testing, because it seems like every time I uncover an issue, write a fix and test it, spending plenty of time to ensure the code is clean and logical (IMO at least), you go and rewrite it with something that is less clean and logical (again, IMO), for no real benefit (that I can discern), and sometimes even break it. I'm going to repeat all the tests I did in my previous post, despite that fact that I've already taken the time to fix all of them, simply because you rewrote my patch.

Its not like I expect every patch I write to go straight in, and I know that my patches are not perfect, but if you can find problems with them, or even don't like how I accomplish things, I'd appreciate an explanation and a correction, or if you don't have time I could correct it. It is frustrating to post code that one has spent time perfecting only to find that it was completely replaced with code that is completely different and doesn't fully fix the issue.

On the subject of _mimemail_url_to_file(), the $relpath argument, which looking back I didn't do a good job explaining, is used to resolved relative URLs "dir/file". Since the function has no knowledge of what the relative URL is based off of, it has to be provided by an external argument. You can't assume it is the domain, and with clean urls on, it even most likely is not the domain. It was going to be part of the CSS url() rewriting (which are relative to the CSS file). One disadvantage of combining two unrelated functions into one mega function is that now doing this will require adding another argument that is only used in certain circumstances.

Anyway, redoing all permutations of clean urls + domain relative/absolute + properly encoded spaces/literal spaces with both images and links with your patch: (this time including just the failures)

Clean URLs off, domain relative URL, literal space: Image encoded correctly, link became http://domain.com/drupalfolder/index.phptestfolder/SPACE%20FILE.txt
Clean URLs off, domain relative URL, encoded space: Same as above

The original link (the space was changed to %20 in the encoded space test): /drupalfolder/testfolder/SPACE FILE.txt

All other combinations got expected results. The space as + issue in gmail is fixed. I'm not sure its a direct result of the patch, but all attached images with spaces in them were shown visibly as attachments (IE, they could be downloaded and viewed on their own) in gmail. I'd post a fix for the clean url bugs, except I still feel like I'd probably be wasting my time, doubly so considering I've already fixed them once before.