don't store complete redirect URL in database

If you copied the database data from your test site to your live site, then you've got stale URLs in your ads table. You'll need to go through and manually fix the "redirect" column in that table for each ad, fixing the base url of each.

I was looking at this section of the code earlier during an unrelated fix and believe it would be possible to not store the entire URL in the database. This would solve your problem as then the data would not be tied to a specific domain, so moving this to a feature request.

Maybe I missed something but I don't understand the needs for storing the whole redirect path in the db.

Let's look the db contents:

| aid | uid | gid | adstatus | adtype | redirect                      |
|  19 |   1 |   1 | pending  | flash  | /drupal-5.1/?q=ad/redirect/19 |
|  20 |   1 |   1 | pending  | flash  | /drupal-5.1/?q=ad/redirect/20 |

Given an aid=NNN the link is always the result of url('ad/redirect/NNN').
The redirect link could be created at load time.

I'm still not an expert in the ad module but maybe this could be done
changing the line

$ad = db_fetch_array(db_query('SELECT * FROM {ads} WHERE aid = %d', $node->nid));

into

$ad = db_fetch_array(db_query('SELECT * FROM {ads} WHERE aid = %d', $node->nid));
$ad['redirect'] = url('ad/redirect/'.$node->nid);

Having path hardcoded into the database is IMHO a bad approach as it will give problems
any time a drupal site is moved.

Hope this helps,

Fabio Varesano

I plan to clean this up soon, assigning to myself as a reminder.

Though certainly possible, there's some extra logic needing to be added if we don't pre-build the redirect URL. The problem being that there are multiple ways to serve ads, and they may come from several different base paths -- I need to give more thought on how to cleanly handle this (simply calling url() when we want to display an ad is not sufficient). I'd hoped to include this in the next release, but it looks like not.

Interesting. Could you please elaborate on why calling url() is not sufficient?

Because the base_path will be different depending on if you call url() from the main drupal/ directory, or if you call it from drupal/sites/domain/modules/ad/ subdirectory (such as when serving advertisements from adserve.inc).

Just an idea: How about registering a menu item like this, to allow serving ads both from ad/redirect/# and path/to/adserve.inc/redirect/# ?

$items[] = array(
  'path' => drupal_get_path('module', 'ad') .'/redirect',
  'callback' => ... // same as path ad/redirect here
);

On or around line 948 (in the function: ad_nodeapi), change the line ...

db_query("INSERT INTO {ads} (aid, uid, adstatus, adtype, redirect, autoactivate, autoexpire, activated, maxviews, maxclicks) VALUES(%d, %d, '%s', '%s', '%s', %d, %d, %d, %d, %d)", $node->nid, $node->uid, $node->adstatus, $node->adtype, url("ad/redirect/$node->nid", array('absolute' => TRUE)), $node->autoactivate ? strtotime($node->autoactivate) : '', $node->autoexpire ? strtotime($node->autoexpire) : '', $activated, (int)$node->maxviews, (int)$node->maxclicks);

... to ...

db_query("INSERT INTO {ads} (aid, uid, adstatus, adtype, redirect, autoactivate, autoexpire, activated, maxviews, maxclicks) VALUES(%d, %d, '%s', '%s', '%s', %d, %d, %d, %d, %d)", $node->nid, $node->uid, $node->adstatus, $node->adtype, url("ad/redirect/$node->nid", array('absolute' => FALSE)), $node->autoactivate ? strtotime($node->autoactivate) : '', $node->autoexpire ? strtotime($node->autoexpire) : '', $activated, (int)$node->maxviews, (int)$node->maxclicks);

... that changes links to store as "/ad/redirect/1" instead of using the entire URL.

@dchaffin: please see #6 and #8 for details as to why this is not sufficient.

This still needs to happen. Moving to the latest development branch.

yup, i'm with sun, although not looked through code, not sure why url() isn't sufficient... but mostly just subscribing.

Here is a patch that assists with this.

It just makes the redirect url get updated on node update as well as node insert.

It doesn't fully address the issue but at least if you then re-save your ads (you can do it with views bulk operations if you have a lot of ads) the redirect url will get fixed.

It is for 5.x-1.x-dev.

I believe the D6 branch already has this functionality.

nope, this is missing in D6 version - which is why i think rvk posted this issue against 6.x

This is the sql for the update op in ad_nodeapi()

  db_query("UPDATE {ads} SET uid = %d, adstatus = '%s', adtype = '%s', redirect = '%s', autoactivate = %d, autoexpire = %d, activated = %d, maxviews = %d, maxclicks = %d, expired = %d WHERE aid = %d", $node->uid, $node->adstatus, $node->adtype, url('ad/redirect/'. $node->nid, array('absolute' => TRUE)), isset($node->autoactivate) && strlen($node->autoactivate) > 1 ? strtotime($node->autoactivate) : '', isset($node->autoexpire) && strlen($node->autoexpire) > 1 ? strtotime($node->autoexpire) : '', isset($activated) ? $activated : 0, isset($node->maxviews) ? (int)$node->maxviews : 0, isset($node->maxclicks) ? (int)$node->maxclicks : 0, isset($expired) ? $expired : 0, $node->nid);

The part for the redirect field is the same as it is for the insert op - url('ad/redirect/'. $node->nid, array('absolute' => TRUE))

This is in 6.x-2.0 and 6.x-2.x-dev

That is all I fixed for D5 in the patch in #14 and it is already done in D6.

There are still other issues being dealt with in this issue though as I believe the people in here are aiming to have this problem addressed at the point of serving the ad (so that the database doesn't store the full url).

This issue just bit me when I moved my new site to a live server.

Suggest that in the short term, this behavior should be prominently documented in the README and on /admin/content/ad/list, ie:

Note: The ad module currently stores a complete redirection path in each ad, not a relative path. If you move your site to a new domain, you should edit each ad, then save it; this will update the path.

Another possibility might be a kludge that lets you update all the paths at once. That would make pingponging between the live and test server less of a pain.

IMO it's a critical feature request! subscribing.

This is a bug report.

This really is a WTF in the module code -- it completely goes against Drupal conventions. Drupal NEVER stores full URLs to the site itself, ALWAYS paths. Otherwise the whole menu system would break.

I like this module, but I may have to not use it if this issue isn't fixed soon. This is definitely a no-no for a CMS IMHO. We're about to go live with our revamped site and with the urls pointing back to my test site from the live site, this will hurt. I can't go through each ad and save them individually to get the url to save correctly. There's too many ads and I'm only the developer, not the content editor. I can't expect content editors to edit ads in two places.

The redirects can be computed on the fly, but we need to remember the base URL of index.php, not serve.php. Attached patch is a first crack at it. I've tested it with the image and text ad types.

It is great to see forward progress on fixing this issue, as it's quite an annoyance that I've been meaning to fix for ages...

Some feedback from an initial read through the patch (I've not tested it yet):

• dpm(), dpr() and dvr() are provided by the devel module, which is not an ad module requirement and thus should not be added into the code.
• You're adding tabs in your new code, you should instead use two spaces per the Drupal coding standards ( http://drupal.org/coding-standards#indenting )
• You have strange spacing in the return in ad_link_path().
• So if someone moves from a dev instance to a production instance, the idea is they flush their cache and the ad_base_path gets updated? It's worth explicitly documenting this in the README.

Overall it looks like it should work. Once you cleanup the above, I'd be happy to test it.

Rerolled with cited changes.

Has anyone else been able to test this?

Drupal 6 is EOL and no longer supported. Closing this as outdated for that reason. Thanks for your contribution!