Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Hello,
I am working on a omniture for context integration, and want to see if you all would be interested in having it part of the omniture module?
it is still growing but is here for now
Comment | File | Size | Author |
---|---|---|---|
#20 | omniture-context-1223462-20.patch | 4.28 KB | lucascaro |
#20 | interdiff.txt | 1001 bytes | lucascaro |
#19 | omniture-context.patch | 4.17 KB | bleen |
#17 | ominuture-context-support.patch | 4.19 KB | bleen |
#16 | omniture-context.patch | 9.31 KB | bleen |
Comments
Comment #1
Race.it CreditAttribution: Race.it commentedI would say as a sub module that would be handy
Comment #2
e2thex CreditAttribution: e2thex commentedYeah that was what I was thinking, but it is also small, and nothing bad happens if context is not installed, so it could also be add to the core module. and then would only be used when someone also installed context
Comment #3
gregglesEither way seems good to me. The code is less than 100 lines including white space and comments.
Comment #4
e2thex CreditAttribution: e2thex commentedOk I will work it up as a patch and then you all can take it whatever way you want
Comment #5
febbraro CreditAttribution: febbraro commentedsub
Comment #6
e2thex CreditAttribution: e2thex commentedOk I ported it to a patch (the second patch is on that can be used with drush make)
I am wandering if I am setting the variables incorrectly, I am setting then by passing the variables and values back from hook_omniture_variables.
Should I be instead setting then each using omniture_set_variables?
Comment #7
e2thex CreditAttribution: e2thex commentedUpdate the patch and add on check to the variable adding
Comment #8
e2thex CreditAttribution: e2thex commentedThe last patch droped the context reaction class
Comment #9
e2thex CreditAttribution: e2thex commentedOk that had an extra file in it
Comment #10
e2thex CreditAttribution: e2thex commentedRemoved param form execute method of context reaction as it was not used
Comment #11
bleen CreditAttribution: bleen commentedsame patch as #10 w/o white space issues
Comment #12
bleen CreditAttribution: bleen commentedComment #13
bleen CreditAttribution: bleen commentedThis patch includes the code from the patch found here: #1791670: Allow sites to include variables (with token replacement) from the Omniture settings form ...
I included that code because it already handles token integration and the variable forms from that patch can be reused here.
Comment #14
bleen CreditAttribution: bleen commentedthe patch in #13 does not apply correctly. Please use this patch instead
Comment #15
bleen CreditAttribution: bleen commented...and now without the whitespace. Its weird ... every once in a while I get these hard tabs when I apply a patch and it makes me sad. Anyway...
Comment #16
bleen CreditAttribution: bleen commentedThis is a re-roll of #15 with a bug fix that prevented the "add another variable" button from working in the context reaction form. It includes the latest patch from #1791670: Allow sites to include variables (with token replacement) from the Omniture settings form as much of the code is shared. I would not commit this patch with first committing that one and then re-rolling
Comment #17
bleen CreditAttribution: bleen commentedre-rolling now that #1791670: Allow sites to include variables (with token replacement) from the Omniture settings form has been committed
Comment #18
lucascaro CreditAttribution: lucascaro commentedThis is great!
It applied without problems to the 7.x-1.x. Did not interfere with the token variables I had set.
I've tried adding a global (always active) context with variables and it worked fine.
I've also tried adding custom variables for nodes of type article and checked that they were showing on articles an not on other content types.
The rendering of tokens in variables seemed to be working.
@bleen18 this is indeed a great feature and does add even more awesomer flexibility :)
I'm not sure why this is passed by ref and returned as a value here. Maybe this is the "context way". I think it would be clearer if this used either option instead of both at the same time (removing the & from the function signature would be the easiest option since the phpdoc won't need to be updated).
Other than that, I think this is totally RTBC
Comment #19
bleen CreditAttribution: bleen commentedYou are correct ... no reason to have both a return value AND pass-by-ref...
This patch should do it then.
Comment #20
lucascaro CreditAttribution: lucascaro commentedAttaching a patch as a suggestion. (removes the return and an unnecessary variable and updates the comments, see interdiff).
Marking as RTBC since this is just a minor detail :)
Comment #21
bleen CreditAttribution: bleen commentedAwesome!! Looks great...
Committed: http://drupalcode.org/project/omniture.git/commit/1f8320311c045c0b7460cf...