The current implementation of the code, has 2 urls, one for the js code and one for the location of the image (used in the no script)

If you have a larger site that uses SSL for some requests you would have to put these both to ssl to avoid issue with users seeing page insecure issues thought the browser.

To fix this I am suggesting putting in another 2 fields that get called if the page called is in secure (HTTPS) mode. If you have not filled these in then the default will be your existing values.

CommentFileSizeAuthor
#6 omniture-559988.patch4.26 KBDeeLay
#3 omniture-559988.patch3.58 KBRace.it
#1 omniture-559988.patch3.17 KBRace.it

Comments

Race.it’s picture

Race.it commented
Status: Active » Needs review
StatusFileSize
new omniture-559988.patch3.17 KB

Here is a patch to include an extra 2 variables that allow for the secure locations of the files, the module will now switch to use the secure variables if the page is requested in a https session

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented
Status: Needs review » Needs work

variable_get should always have a default value, even if it is variable_get('something', '');

Otherwise, looks good.

Race.it’s picture

Race.it commented
Status: Needs work » Needs review
StatusFileSize
new omniture-559988.patch3.58 KB

Re-rolled patch, to include the default values for variable_get on the new https variables.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented

I'm not so sure about this change:

-      if (isset($omniture_hooked_vars['variables'])) {
+      if (($omniture_hooked_vars['variables'])) {

The isset should prevent E_NOTICE warnings from PHP.

Race.it’s picture

Race.it commented

the removal of the isset was an error in the patch, ill reroll it to leave that in

DeeLay’s picture

DeeLay commented
StatusFileSize
new omniture-559988.patch4.26 KB

Hey,

Just found this having already come up with an alternate fix, so posting the patch for you to consider versus the existing one.

This patch doesn't have the extra form fields, but does make the assumption that the SSL location for the image and js will be the same as the non-SSL.

DeeLay’s picture

DeeLay commented

Just found out from pifantastic that you can have protocol relative URLs:

//site.com/path_to_omniture_script.js

as mentioned here:

http://blog.httpwatch.com/2010/02/10/using-protocol-relative-urls-to-swi...

I'll rework the patch to just include some informational messaging about this.

Race.it’s picture

Race.it commented

Does this method work across all browsers correctly as that article mentions issues with ie and css. I know we are talking about js here but would need to be checked, maybe an option flag if you want full urls still in there?

sperks’s picture

sperks commented

Protocol relative URLs are absolutely fine for JS files, and I can't imagine the need for two different URLs should you be using https or not. My suggestion would be to close this issue as "won't fix" and possibly add some comments to use either a locally absolute URL (/sites/js/s_code.js), or a protocol relative URL.