Closed (fixed)
Project:
Doubleclick for Publishers (DFP)
Version:
7.x-1.x-dev
Component:
Code
Priority:
Normal
Category:
Feature request
Assigned:
Unassigned
Reporter:
Created:
13 Dec 2012 at 22:06 UTC
Updated:
6 Feb 2013 at 12:00 UTC
Jump to comment: Most recent file
Comments
Comment #1
flacoman91 commentedsorry, disregard the above patch. it was working on the rc version, but for the dev version it's not working.
Comment #2
flacoman91 commentedOkay again, here's a complete patch. for the context addition.
I left some things out accidentally the first time (I'm somewhat of a noob).
Comment #3
bleen commentedI like this idea a lot ... so much so that i improved it a bit. The attached patch (based on #2) adds the ability to choose which tags should be overridden by offering checkboxes and allowing for tokens in the overridden ad unit:
I wish we could have simpletests for this, but since context is not a dependency, alas...
Comment #4
gurrmag commentedI think this is a great idea... Any chance you could have the option to override the ad unit size that is sent?
This might be a simple way of supporting advertising on reactive theming? It seems you could do the same by targetting custom ad units in this patch as it stands, but if you could just change the ad unit size, you wouldn't need to create ad units for different breakpoints/layouts....
Comment #5
bleen commented@gurrmag ... did you test the patch in #3 ? Is it RTBC?
As for adding similar functionality for ad size... there are some subtle differences with that (like you would never want to change two tags in one "context" because the new sizes would be different for each tag) such that I think that should be in its own issue. Please feel free to open one.
Comment #6
gurrmag commentedLimited local testing... which worked as expected.
I need our advertising guys to set up new units in DFP, but they're not so quick when there's no sniff of commission!
I take the point about opening a separate issue - will do so as soon as I've posted this.
Link to issue: http://drupal.org/node/1884052
Comment #7
gurrmag commentedI can confirm that this works...
For our implementation, we surfaced global tokens for targeting and ad unit patterns so that the standard URL tokens and tokens from the Custom Token module were available.
Not sure if you would be interested in a combined patch for this?
Comment #8
bleen commentedsure .. please share your patch
Comment #9
soulston commented@bleen18, do you want me to run the patch against dev (7.x-1.x) or your 7.x-1.0-rc2?
See http://drupal.org/node/1895522 - One other issue we are having is that we need to surface node tokens in the adunit pattern and currently they are not being processed. Can you see any problem in doing this? We probably need to work on the login for checking for "pattern" in dfp_format_targeting() (thanks @gurrmag)
template_preprocess_dfp_tag()
dfp_format_targeting()
Comment #10
bleen commentedAlways role patches against -dev. Thats true for all Drupal projects...
I'm confused here ... node based tokens are not working for adunit?? That doesnt sound right. Can you give me an example of exactly what value you are using for adunit, what you expect it SHOULD evaluate to and then what it actually evaluates to. I'd like to try and reproduce... BUT, please do this by opening a new issue. I dont want to confuse the current issue any more then we already have... :)
This sentence I simply dont understand... "work on the login"?
As for your code changes, when you open that new issue, feel free to include them as a patch. I cant tell you for sure if Id accept it because I dont yet grok the problem, but assuming there is an issue and this is a possible fix, I dont see anything crazy in there just yet.
Comment #11
bleen commentedBased on #7 I committed the patch from #3... thanks @flacoman91
@gurmag, feel free to role a new patch for your idea about global tokens in a new issue
@soulston, feel free to role a patch (and give more info) about your comment in #9 in new issue
Comment #12
soulston commentedThanks for the feedback @bleen18,
I re-read my post and confused myself - login should be logic!
Patch on it's way
Comment #13
flacoman91 commentedAwesome. thanks a bunch !
Comment #14
soulston commentedPatched
* Brought global tokens into scope
* Fixed a few typos - Implements of hook_... -> Implements hook_...
* Fixed typo $tagets[] = check_plain($data['target']) . '=' . check_... -> $targets[] = check_plain($data['target']) . '=' . check_...
I also updated my repo to include the patch from http://drupal.org/node/1895522.
Comment #15
soulston commentedComment #16
bleen commentedsoulston ... would you mind creating a new issue "Allow global tokens" or something. I just dont want to confuse this issue.
Thanks!
Comment #17
soulston commentedNo probs - http://drupal.org/node/1896188