First of all, great job on this module Wim. Very impressed with your WPO work overall, including your BS/MS work & contribution @FB.
We are migrating our website into Drupal 6.26. We use EdgeCast for our reverse-proxy CDN and rather than re-code my existing WPO code for Drupal, I looked into using your CDN contrib module. I was glad to find that your module reproduced many of the best-practices we employ (eg. url-altering, domain sharding, cache-busting, far-future expiration, etc.).
Installing and configuring the CDN module was a breeze. I patched Drupal Core using your patch file. Image URLs within CSS files were altered sucessfully, but for some reason Image URLs inside the node body were not being altered.
I dug into your code and found that cdn.module::cdn_nodeapi()::line409 cdn_html_alter_image_urls($node->body); was not working.
I modified the code to cdn_html_alter_image_urls($node->content['body']['#value']); and IMG URLs in the node body were successfully being altered.
Being a bit new to Drupal, I'm not sure if this issue is specific to our install or an actual bug since I didn't find an existing report in the issue queue.
I can submit a patch if you feel this is a bug, or else if this issue is specific to our install and nobody can replicate, then at least this may help someone else.
Keep up the great work Wim.
Best,
HenryLTV
| Comment | File | Size | Author |
|---|---|---|---|
| #25 | cdn-fix_rewrite_body_content-1664686-25.patch | 592 bytes | JStarcher |
| #23 | cdn-fix_rewrite_body_content-1664686-23.patch | 592 bytes | JStarcher |
Comments
Comment #1
wim leersVery odd that nobody encountered or reported this problem…
What should happen to
$node->teaserthen, according to you? An analogous change?FYI, this was written and confirmed to be working right here: #971712: Rewrite image URLs in the node body, http://drupalcode.org/project/cdn.git/commit/c6f10a36
Comment #2
HenryLTV commentedI found that odd as well, which is why I'm posting this in hopes others can either shed some light or replicate my finding.
Yes, #971712: Rewrite image URLs in the node body was the first link I looked at and was surprised that nobody else tested.
Entering "node content body value" in Google Search yields many results that seem to suggest that using
node->bodygets the body content, but if one is looking to alter/print the body content, such as in this case, many people have suggested usingnode->content['body']['#value'].Sorry, but I can't find an exact 'analogous' solution for
node->teaser.What did your own local tests reveal? Can anyone else either replicate or shed some light?
Thanks,
HenryLTV
Comment #3
HenryLTV commentedI couldn't find in either Status Settings for an Issue or Use the Issue Queue the proper etiquette for whether I should leave the status as "Postponed (Maintainer needs more info)" or change it back to "Active" after providing more info; I'll do the latter for lack of info.
Comment #4
wim leersI'd imagine we also need to alter
$node->teaser['#value'], that's what I meant by "analogous change".It's been a while since I did testing on D6, so unless somebody confirms that the change you proposed is correct, and also works for the teaser, this will have to wait until the next time I have time to work on the CDN module.
This will most definitely get fixed, of course :)
Comment #5
kmontyEdit: upon a second look, the code I posted wasn't particularly useful. Removed.
Comment #6
HenryLTV commentedok cool, sounds good Wim. In the meantime I'll do some testing on
node->teaserto ascertain whether it exhibits similar behavior.Comment #7
wim leersGreat, thanks! :) Let's get this fixed!
Comment #8
wim leersAny progress? :)
Comment #9
wim leersPinged HenryLTV.
Comment #10
HenryLTV commentedHi Wim,
Thanks for being patient. Here's an update:
node->bodydoes actually work andnode->content['body']['#value']does nothing at all, but for other content types the opposite is true wherenode->bodydoes not work as reported in this thread. I'm still trying to investigate why this is happening and what differences in the content types could be causing this inconsistent behavior. Unfortunately I don't yet fully understand the circumstances as to why one is used to render the resulting page over the other.node->teaserbut will get on that as soon as my current sprint is finishedAgain, I'll try to keep this thread updated with my progress.
Thanks!
HenryLTV
Comment #11
wim leersThat was my initial suspicion: it definitely worked at some point, so there must be *something* about your set-up that is causing this.
So, I'm pretty confident there is some module interfering here. The best way to track this down is by looking for modules that implement
hook_nodeapi(), and then specifically$op = alter, like the CDN module does: http://drupalcode.org/project/cdn.git/blobdiff/d7a74113a1f79fc7e17f250b4....Comment #12
wim leersClosing due to lack of response. Feel free to reopen when there is news.
Comment #13
fizk commentedI'm having the same issue. Using $node->content['body']['#value'] instead of $node->body fixed the issue for me.
I'm using the WYSIWYG and IMCE Wysiwyg API bridge modules with the TinyMCE editor. When I insert an image into the body of the node, it does not get rewritten to use the CDN URL. However, the node's CCK image field does get a CDN URL.
I think we should leave this issue active until the root problem is found.
Comment #14
wim leersI only leave issues active if people actually respond, or when it's not doubtful that the CDN module is actually the cause.
So, to confirm this is a bug in the CDN module: can you please verify this on a vanilla D6 install? Thanks!
Comment #15
HenryLTV commentedWell I'm certainly glad someone else has reported this issue so I know it's not just our setup or configuration.
I think the difficulty with determining whether the CDN module is the cause of this inconsistent behavior or if it's something else (core/module/theme), is because it seems none of us have a 100% grasp of when it's appropriate to use node->body v.s. node->content['body']['#value']. It's obvious these are used for different purposes in the page lifecycle or we wouldn't be encountering this.
I finally enabled the CDN module for our live production site today just to get it out there (since it works perfectly for 90% of our paths), and plan on patching later depending on progress of this thread. A hacky fix would be just to pass both into cdn_html_alter_image_urls(), but I'm hesitant to do this until I locate the root of the problem.
I apologize for the delay in between updates. Due to our aggressive sprint schedule I haven't been able to devote as much time into researching this bug as I'd like, but my next task will be to replicate this on a vanilla D6 install (as Wim suggested) on my next sprint break & report back as soon as I'm able. If someone else in the meantime gets to this before me, even better.
Thanks!
HenryLTV
Comment #16
wim leersIt's been 20 days without an update. Decreasing priority. I'd have loved to included the bugfix for this in the upcoming 2.5 release.
Comment #17
rjbrown99 commentedFWIW, I was seeing something similar here related to node teaser output in views. #1022464: View field output of nodes does not rewrite CDN paths.
Comment #18
wim leersChanging to task instead of bug report. This has been sitting here for well over 5 months without further complaints, which indicates that it's not really a bug.
Comment #19
JStarcher commentedI'm running into the exact same issue. The change above does indeed fix the problem. I'm still digging into why this is happening and I agree that it's mostly likely another module causing the problem. I too am using WYSIWYG so there is a possibility of something going on there.
Comment #20
JStarcher commentedComment #21
wim leersComment #22
JStarcher commentedComment removed.
Comment #23
JStarcher commentedI am still having this issue and without spending more time investigating exactly why $node->content['body'] hasn't been rewritten I was able to confirm that the fix above works. I inspected the $node variable and did not find any instances of any special cases for teaser.
Comment #24
wim leersAny news, JStarcher?
Comment #25
JStarcher commentedThe patch I created worked great for me. I'm reattaching the patch and setting to Needs Review to queue for testing as I couldn't find any ill behaviors created by the change.
Comment #27
wim leersAlright, committed #25. However, I'm not too fond of the fact that this got quite little testing, and that it only seems to occur on a subset of sites.