Adds more flexibility for controlling SiteCatalyst code in the page source by including fields in the admin settings section. Also addresses an issue with global scope of the $script variable.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

greggles’s picture

Title: Proposed enhancements » hook_uninstall, use define() for omniture variables, comment typo fix, rearchitect variable addition

There's no need to alter omniture-new/omniture.info, that will get fixed by the packaging code.

Ditto on the $Id line which is handled by CVS.

This fixes a ton of stuff which makes it harder to review/commit.

I don't have the ability to debug it myself, so I'll wait for a more thorough review by another maintainer.

uberhacker’s picture

Hi Greg,

Thanks for the feedback. I'm new to patching and do not have a CVS account yet [ie. hint ;)] so I appreciate your patience while I get up to speed. If you think there are a ton of fixes here, you should hop on over to the omniture_node module and examine what I contributed to the issue queue there. ;P

Thanks again

crimsondryad’s picture

Greggles,
I work with uberhacker and we are using this code in a production corporate environment. We'd really like to get it committed a) so we can get more eyes on it b) so it eases future updates for us. Obviously with D7 about to hit RC1 you are very busy. :) Is there any way we can get this module reviewed by another maintainer? Or anything else we can do to help this process along?

We are very excited to be using Drupal (we have it running on 7 corporate sites at the moment) and we are actively contributing back as much code as we can. Having said that, we're still wet behind the ears for contributing. We'd appreciate your advice on getting started the right way. :)

greggles’s picture

Status: Needs review » Needs work

The easiest way to get this reviewed is to fix the things I identified in #1 and separate the changes into smaller more focused patches.

I (along with the other module maintainers) also support this module for a variety of "production corporate environments" and they expect us to review things thoroughly before committing.

Further review:
db_query('DELETE FROM {variable} WHERE name LIKE "omniture_%"');

We should delete individually to avoid potential problems with this.

The function fix_omniture_script_path is not a good name. omniture_fix_script_path_and_protocol is more descriptive and starts with the right namespace.

You completely remove the omniture_js_file_location variable and the omniture_image_file_location and it seems you have replaced them with other variables but you don't provide an upgrade path for the variables that were in use.

I stopped the review on the settings form. I think this should really be split up into specific discrete patches so we can review their purpose and bugginess one piece at a time.

crimsondryad’s picture

Thank you so much for taking the time to look at this. We will review what you said and see what we can do. :)

Please don't be offended when I mentioned corporate production environments...I understand that you are expected to review especially since others are using this. I would be disappointed if the maintainers didn't review it thoroughly. I only meant that the module is getting substantial traffic for testing purposes and so far it has worked for us. :)

uberhacker’s picture

Status: Needs work » Needs review
FileSize
13.72 KB
1.04 KB

Hi Greg,

Thanks for the feedback. I really appreciate you taking the time to review my enhancements. I've attached 2 new patches that address your suggestions. I renamed the variables back to their original names so that an upgrade path is not required. Let me know if you need anything else.