On my site inserted links which used relative links were not getting updated. Since this was a crucial issue for me (and maintenance here seems pretty well stalled) I looked into it myself. The logic in _filefield_paths_replace_path made no sense to me - the regex constructed was never matching and I'm not really sure how it ever could work. Heck, it was adding "http" in the middle of the match. I think maybe it was trying to be too intelligent, but the result was nonsense. With a little rewriting of the matching logic, I got it to work.

The supplied patch (which includes the fixes from http://drupal.org/node/1475732) restores operation for absolute and relative paths and works without conflicts alongside all the internationalization goodies, pathauto cleanup, etc.. Installation in a subdirectory is untested, but in theory it should work - the relative match is created including the path portion of the absolute match. Styled matches and private files are also untested, as I was working with non-image files, but at least this is sure to work some of the time, and under typical usage. The code in beta3 and 1.x-dev are both completely non-functional.

CommentFileSizeAuthor
#6 filefield_paths.module.patch3.77 KBHonoredMule
#5 filefield_path_links_update-1866450-5.patch1.98 KBprinds
#3 filefield_path_links_update-1866450-#3.patch1.98 KBprinds
#2 filefield_paths.module.patch3.65 KBHonoredMule
filefield_paths.module.patch1.53 KBHonoredMule
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

HonoredMule’s picture

HonoredMule CreditAttribution: HonoredMule commented
Status: Patch (to be ported) » Needs work

I've encountered some issues with this patch...will update when the kinks are worked out.

HonoredMule’s picture

HonoredMule CreditAttribution: HonoredMule commented
Status: Needs work » Needs review
FileSize
filefield_paths.module.patch3.65 KB

Take two. it's a more comprehensive rewrite this time...still logically similar but less obtuse about it and fewer redundant calculations. The only definite problem with the old version was inability to handle filenames with apostrophes in them and the loss of handling for raw stream-wrapped urls, but it made sense to make the whole function more readable anyway.

prinds’s picture

prinds CreditAttribution: prinds commented
FileSize
filefield_path_links_update-1866450-#3.patch1.98 KB

I also encountered issues with _filefield_paths_replace_path()

The issues were
1. Inability to handle pathnames with special characters.
2. Bad matching when text had more than one image.

@HonoredMule, I tried to use your patched version as my starting point, but I found many new issues with your code. E.g. the $replacer string is trying to pass an array, but it gets converted to the string 'Array'. I think you need to pass it in as array('key'=>'value') in order for it to work. Also the $pattern string didn't work for me with image styles.

So in the end I started with the 7.x-1.x-dev version and rolled a patch that converts special characters to url encoded chars (so they match the output of file_create_url()) and that checks for [^/]+? in stead of .*? in the $pattern (which was still too greedy despite the ?).. And lastly I added a check, that makes sure we never save a NULL value if an error occures in preg_replace().

Perhaps you can test this patch, and see if it fixes the issues you had with _filefield_paths_replace_path(), and if not try to combine the two solutions..

Perhaps there's also a better way to urlencode the path parts..

tlarrieu’s picture

tlarrieu CreditAttribution: tlarrieu commented

I'm having the same issue and would like to see the contents of your patch, however the link to http://drupal.org/files/filefield_path_links_update-1866450-#3.patch doesn't seem to work for me.

prinds’s picture

prinds CreditAttribution: prinds commented
FileSize
filefield_path_links_update-1866450-5.patch1.98 KB

Here's a version of the above patch without the '#' in the name.. That should work better.

HonoredMule’s picture

HonoredMule CreditAttribution: HonoredMule commented
FileSize
filefield_paths.module.patch3.77 KB

Quite some time ago I did address some of the "stupid programmer" mistakes I made, which fixed one bug I was experiencing (which notably allowed some greedy matches that blanked the page when uploading and inserting a file in the same operation while also using this patch--not directly because of a regex error but because of empty variables) and left me with something that's working for files, images, and image styles. Looking back at what I now have, however, I see the most glaring mistake still present (trying to put an array in a string representation of a function call o_O). Yet somehow I'm getting correct behavior by some amazing force of serendipity despite not yet having corrected that (I'm pretty sure correct uri schemes are still being chosen, due to the choices not changing during a path update and thus being able to rely on the returned default).

Regarding this:

...and that checks for [^/]+? in stead of .*? in the $pattern (which was still too greedy despite the ?)...

Can we depend on image style paths only consisting of a single level of directory depth? If so I agree this is a safer looking solution, but will note that as ".*?" is actually a non-greedy match it should pose no problem and discard matches that could start later as well as those that could end earlier. (It's one of the things I actually checked carefully both against doc--http://docstore.mik.ua/orelly/webprog/pcook/ch13_05.htm--and in practical tests--i.e. multiple identical and varied pattern matches in a single string--in the initial effort.)

I would really like to revisit this for a proper solution and also review/incorporate your work as appropriate at some point, but I'm not sure when I'll be able to get to it--I don't get much opportunity to focus on issues that aren't actually affecting our sites, or enough free time to pursue it on my own. For now I'll submit the mostly fixed version of my patch that I'm currently using, noting that:
- the array passing issue is still present (and would affect things like switching between public and private file storage--though I could swear I tested specifically that...)
- issues with uri special characters are likely still present (I expect you'd have to disable pathauto cleanup in order to encounter them)
- issues with greedy matches may still be present (but I'd like to see some evidence since in theory they really shouldn't and in practice have not been)

All the issues are easily fixed with very quick and straightforward code changes, but thoroughly testing those changes will be a much bigger job and and one for which I'm not as well equipped or experienced--my dev and test environments here are pretty homogenous.

Anyway, hopefully I'll get back to this eventually. :/

prinds’s picture

prinds CreditAttribution: prinds commented

Regarding this:

...and that checks for [^/]+? in stead of .*? in the $pattern (which was still too greedy despite the ?)...

Can we depend on image style paths only consisting of a single level of directory depth? If so I agree this is a safer looking solution, but...

.. the [^/]+? matches the style name, so this should work in any case.. Did you experience any problems matching images below a single level of directory depth?

Deciphered’s picture

Deciphered CreditAttribution: Deciphered commented
Status: Needs review » Postponed (maintainer needs more info)

Hi,

I just ran a quick test with the latest dev with a relative path and it worked perfectly. It's possibly just an environment issue (PHP version or similar), but to be safe, can you provide me with a step-by-step guide on how you are getting the issue just to make sure I'm not interpreting the issue incorrectly.

Cheers,
Deciphered.

prinds’s picture

prinds CreditAttribution: prinds commented
Status: Active » Postponed (maintainer needs more info)

Try this.. (based on my description in #3)

1. rename an image to include special characters (e.g. the scandinavian æ,ø or å)
2. upload that image to your site, and insert it into a text field (relative or absolute shouldn't matter)
3. test to see if it updates correctly

If it fails, try the patch in #5..

I haven't tested my patch with non-image files.. That could be a problem..

Deciphered’s picture

Deciphered CreditAttribution: Deciphered commented
Status: Postponed (maintainer needs more info) » Active

Setting active, will test with that workflow, however, I was testing with images as well, the only thing I can see that might be related is the special characters.

Deciphered’s picture

Deciphered CreditAttribution: Deciphered commented
Status: Postponed (maintainer needs more info) » Active

Confirmed issue.

Simplest fix for this is just to have the Transliteration module enabled.

Looking into a proper fix though.

Deciphered’s picture

Deciphered CreditAttribution: Deciphered commented
Status: Active » Fixed

Fixed and committed.

Cheers,
Deciphered.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

(not verified) CreditAttribution: commented
Issue summary: View changes

Adding justification for applying a patch providing only partial fix.