Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
My drupal 7.15 lives in a sub dir http://www.example.com/drupal
I have some images outside drupal under http://www.example.com/img
After the update to the latest version image urls like <img src="/img/test.jpg">
are now converted by pathologic into <img src="http://www.example.com/drupal/img/test.jpg">
and images are broken.
Setting base_path and rewrite_base in drupal doesn't solve the problem.
Any idea?
Comment | File | Size | Author |
---|---|---|---|
#16 | filepath_handling-1763696-16.patch | 5.87 KB | azinck |
#14 | filepath_handling-1763696-14.patch | 5.86 KB | azinck |
#6 | filepath_handling-1763696-6.patch | 5.13 KB | azinck |
Comments
Comment #1
Garrett Albright CreditAttribution: Garrett Albright commentedDo you have a slash (or anything else) in your "Also considered local" field in the filter's configuration settings?
Comment #2
casta CreditAttribution: casta commentedThe field "All base paths for this site" in the text format is empty, no other path is considered local
Comment #3
Garrett Albright CreditAttribution: Garrett Albright commentedOkay, I was able to replicate this bug, and got a fix in in the most recent release. Please give it a download and a try. If it still seems broken, please reopen this issue.
Comment #5
azinck CreditAttribution: azinck commentedThe commit, for reference: http://drupalcode.org/project/pathologic.git/commitdiff/94a085869959c427...
I think this is the wrong logic. We need to be comparing the url paths to determine if the file's in the Drupal root, not the system paths. The use of realpath here means that a variety of use-cases (in my case, having a files directory that's symbolically linked to elsewhere on the filesystem) break. I'll try to whip up a patch this afternoon.
Comment #6
azinck CreditAttribution: azinck commentedThis patch changes more than I would have really liked, but I felt it was necessary. It needs plenty of review because there are a lot of edge-cases out there with URLs that may have escaped me. Patch is against 7.x-2.x-dev
Comment #7
azinck CreditAttribution: azinck commentedComment #8
azinck CreditAttribution: azinck commentedUpdated the title to hopefully make it clearer.
Comment #9
Garrett Albright CreditAttribution: Garrett Albright commentedYeah… applying your patch causes a lot of Pathologic's tests to fail. It would cause more problems than it would solve, I'm afraid. (If you're unfamiliar with Drupal's testing system, the easiest way to check if your code breaks Pathologic's tests is to enable the Simpletest module, then run
drush test-run PathologicTestCase
and give it a minute (testing is slow). If tests fail, you can search pathologic.test for the text of the fail message to see what sort of input Pathologic is trying to process what is causing the failure.)Thanks for the lead that the cause of this is indeed symlinks outside of the Drupal root directory. Going off that, I was able to easily recreate the problem. I'll work from there and hopefully have a fix in place before you even have time to post a new patch. :)
Comment #10
Garrett Albright CreditAttribution: Garrett Albright commentedOkay, I tried this tweak, and it seems to work, at least insofar as I was able to replicate the problem, even though it's much simpler. Do you think you could give it a try on your end once a new dev release becomes available?
Comment #11
azinck CreditAttribution: azinck commentedThanks for reviewing the patch.
Frankly, I didn't even think about running it through tests! Haha. Oops. I'll look into that.
Your follow-up doesn't seem to account for all cases. Consider a case where $_SERVER['DOCUMENT_ROOT'] is symlinked *and* DRUPAL_ROOT is symlinked from somewhere else. Then, $settings['current_settings']['docroot'] won't match DRUPAL_ROOT down on line 224.
adding realpath() like so still doesn't fix the problem because DRUPAL_ROOT could be symlinked from an entirely different part of the filesystem.
And you still have a problem if Drupal is being run from inside a subdirectory as, at that point you haven't run through the host-and-path-matching foreach loop. So you don't know if $parts['path'] is relative to the drupal root or the web root, thus making the comparison unreliable (especially so if this comparison is being done with drush and we don't have access to $_SERVER['DOCUMENT_ROOT'] which is a very big deal).
There are a lot of nuances to the logic here, but I think we should try to stick to parsing the web path itself rather than attempting to figure things out at the file system level. My patch tries to repair what I saw as faulty logic in the foreach loop for determining if something's really inside the Drupal root. I think I'll take a stab at what tests it's failing before giving up on it.
Comment #12
azinck CreditAttribution: azinck commentedGarrett: I took a look at the test failures and they reflect a difference in how we detect whether a given path references a file.
Your original logic tries to determine if the file exists on the filesystem to determine if something is a file.
My patch considers something a file if menu_get_item() doesn't find anything in the Drupal menu system to handle the request.
I think an argument could be made in favor of either perspective.
I'll see if I can update my patch to match your logic.
Comment #13
azinck CreditAttribution: azinck commentedComment #14
azinck CreditAttribution: azinck commentedOk, that was easy. Try this patch on for size.
Comment #15
azinck CreditAttribution: azinck commentedI should mention that the patch in #14 passes all Pathologic tests for me.
Comment #16
azinck CreditAttribution: azinck commentedFixed comment that references old logic.
Comment #17
Garrett Albright CreditAttribution: Garrett Albright commentedazinck, I presume you're not running a multi-lingual site, because some of the stuff your patch took out was breaking that - taking out the "fake" language object when $settings['is_file'] causes paths to images to get prefixed with the language prefix. There was some other stuff I had to clean up, but for the most part, I'm going to take your patch and hope it clears things up with this problem, because I'm pretty much at a loss as to how to move forward, so maybe you're on to something.
(Yes, all the tests pass, but there's some stuff I don't know how to test for, such as multi-lingual-related stuff, and others which I'm not sure it's possible to write tests for, like the symlink issues…)
Comment #18
joelcollinsdc CreditAttribution: joelcollinsdc commentedi can confirm this commit fixes pathologic for our setup with a spider web of symlinks. thanks!!
Comment #19
azinck CreditAttribution: azinck commentedGarrett,
Thanks for continuing to look into this. I did some testing with multi-lingual when I was tweaking that part of the code. I thought I had it sorted but maybe I missed something. I'll look at it again when I get back to work (just had a new baby).
Comment #20.0
(not verified) CreditAttribution: commentedEscape HTML.