Several generated urls miss a base_path() prefix, so the module currently doesn't work when the drupal install is in a subdir of the webroot.

Attached patch fixes some of them. Others are probably missing (maybe in the tinymce plugin ?)

CommentFileSizeAuthor
#26 asset.patch2.32 KByched
#21 asset_2.patch12.67 KByched
#17 asset.patch12.35 KByched
#2 asset.patch11.12 KByched
asset.patch4.98 KByched

Comments

yched’s picture

yched commented
Status: Active » Needs review
yched’s picture

yched commented
StatusFileSize
new asset.patch11.12 KB

a few more...

true-pal’s picture

true-pal commented
Title: works only from webroot » patch won't work

I'm not able to apply the patch:
- Tried the Windows Patch Tool and cygwin
- asset 5.x-1.x-dev 2007-12-20
- patch above in root folder of the asset module
- using command: patch < asset.patch

I got following message:
patching file asset.js
Assertion failed: hunk, file ../patch-2.5.9-src/patch.c, line 354
...

help please

yched’s picture

yched commented

Setting title back.

I'm not too familiar with applying patches from the command line (win/Eclipse here), unfortunately.
Maybe the patch simply needs a reroll. I'll check.

yched’s picture

yched commented
Title: patch won't work » works only from webroot

Er, actually setting back title...

jmlane’s picture

jmlane commented

Are the errors you mentioned the same ones as I am having here: http://drupal.org/node/212653

If so, I will try your patch.

true-pal’s picture

true-pal commented

HELP PLEASE,
could someone provide here an already patched asset.module? please

Thanks in advance

wmostrey’s picture

wmostrey commented
Assigned: Unassigned » wmostrey
Priority: Normal » Critical

Hey Yves, thanks a lot for your patch. Did you try it yourself using a subdir? For instance there's no need to use both base_path() and drupal_get_path('module','asset'). If the site is in a subdir test, this will result in /test//test/sites/all/modules/....

I will work forward from your patch, test it a lot and then bring out a new patch for testing.

wmostrey’s picture

wmostrey commented
Status: Needs review » Needs work
yched’s picture

yched commented

there's no need to use both base_path() and drupal_get_path('module','asset'). If the site is in a subdir test, this will result in /test//test/sites/all/modules/....

No, it results in a path relative to drupal's index.php - guaranteed :-)
Thus, all paths generated to be used as client side urls (for images, js files...) need to care about base_path().

However, my patch is wrong, because
- the preferred way is to let url() do that, instead of prepending base_path() manually.
- there are a couple places where it does add base_path when it shouldn't ;-)
- it misses a few more occurrences.
I'll try to reroll it tomorrow.

wmostrey’s picture

wmostrey commented

OK, thanks for this. Please make sure you run a quick test on an installation that doesn't use a subdirectory in order to see that you're not getting double slashes at the beginning of a file's directory (e.g. //sites/all/...).

true-pal’s picture

true-pal commented
Assigned: wmostrey » Unassigned

thanks for the new dev-release (2008-01-23) but the release is not patched, or am I wrong?
Is there a patched release on schedule?

since (see above #3 & #7) and all my sites in subfolders (not on webroot)
I have the problem that all images (cck asset field) are not clickable i.e. page 404
or a never ending revolving thickbox animation.

hoping for good news
JK

wmostrey’s picture

wmostrey commented

There is no patch yet, so the new release does not include a fix for this yet, we're working on it.

yched’s picture

yched commented

Updated patch, uses url() instead of base_path() previously, and tries to target minimal change.
url($path) handles the case where $path is already an absolute URL (http://www.example.com), so this also simplifies the code in asset_img_preview().

Please make sure you run a quick test on an installation that doesn't use a subdirectory
Truly sorry, but I can't afford that time right now... :-(
I don't see why this would break, though.

Side note : rolling patches for asset would be highly simplified if the files complied with core conventions about trailing whitespaces and space as tabs :-) Many people have their IDE configured to automatically take care of that on file save, which simplify core patching work, but also raises lots of unwanted diff lines when patching files with trailing spaces.

yched’s picture

yched commented
Status: Needs work » Needs review

change status

wmostrey’s picture

wmostrey commented

yched, thanks so much for your work on this. Could it be that you forgot to attach the patch?

yched’s picture

yched commented
StatusFileSize
new asset.patch12.35 KB

LOL - could be :-)

wmostrey’s picture

wmostrey commented

The issue with url() is of course that if you have i18n enabled, such urls will be created: /en/sites/all/modules/asset/lullacons/doc-option-add.png. Note the prepended "en" for the English language. This file doesn't exist of course. I think the easiest way is to just prepend base_path(). If you want I can take it from here, it looks like you got all places covered where this needs to happen so I can build onwards from your patch.

For instance:

print '<img src="' . base_path() . drupal_get_path('module', 'asset') . '/lullacons/doc-option-add.png">';
yched’s picture

yched commented

Hm, that's right, base_path() is the way to go...

wmostrey’s picture

wmostrey commented
Status: Needs review » Needs work
yched’s picture

yched commented
Status: Needs work » Needs review
StatusFileSize
new asset_2.patch12.67 KB

back to base_path(), then

wmostrey’s picture

wmostrey commented
Status: Needs review » Fixed

Works as advertised now. Patch will be committed tonight, thanks so much for this!

As a warning to other users: this patch doesn't apply cleanly to the newest dev version. Some hunks will fail but since those fails are not part of the base_url fixes, you can just leave those be.

true-pal’s picture

true-pal commented

using asset with lightbox I have already had an error.
Since the sub-directory has not been taken into account.
Original Line in asset.module
$link = '<a href="/'.$a->filepath.'" rel="lightbox[gallery]" title="' . str_replace("\"", "'", $asset['caption']);

I have changed to:
$link = '<a href="'.file_create_url($a->filepath).'" rel="lightbox[gallery]" title="' . str_replace("\"", "'", $asset['caption']);

Now I'm happy with the asset module :-)

since this is my first bug-fix in a drupal module, please confirm or dismiss it.

yched’s picture

yched commented
Status: Fixed » Reviewed & tested by the community

True, I missed that one (I don't use thickbox).

I didn't actually apply the fix, but this is obviously right, so marking as RTBC (even though there's no actual patch)

wmostrey’s picture

wmostrey commented
Status: Reviewed & tested by the community » Fixed

Thanks for this Juergen, committed.

yched’s picture

yched commented
Status: Fixed » Reviewed & tested by the community
StatusFileSize
new asset.patch2.32 KB

The parts in asset.module from patch #21 didn't get committed.
Here's an updated patch.

wmostrey’s picture

wmostrey commented
Assigned: Unassigned » wmostrey

Thanks for reporting this yched, it will be rolled out tonight.

wmostrey’s picture

wmostrey commented
Status: Reviewed & tested by the community » Fixed

Committed, thanks again.

true-pal’s picture

true-pal commented
Status: Fixed » Needs work

hello again,

sorry but the bug mentioned in #23 is still open. --> Delete the slash in front of the URL and you will be released :-)

true-pal’s picture

true-pal commented

Sorry for sending twice, how to delete this comment?

"hello again,

sorry but the bug mentioned in #23 is still open. --> Delete the slash in front of the URL and you will be released :-)"

wmostrey’s picture

wmostrey commented
Status: Needs work » Fixed

Applied, thanks!

Anonymous’s picture

Anonymous (not verified) commented
Status: Fixed » Closed (fixed)

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