Ultimate SEO v3 (and v2.200) [Support Thread]

[John Large]

@lat9

in commit #42, in the usu class, it seems you have deleted line 100 which had the following code:

// Determine canonical (if needed)
        $this->canonical(); 

was this on purpose?

in my testing, this changes causes the automatic redirects to not happen.

now, whether that line of code is pertinent to the original redirect problem, i can not say, as i did not reproduce the original error.

john,
what happens if you add that line back?

starting at line 94 of includes/classes/usu.php, make it look like so:

            } elseif (IS_ADMIN_FLAG) { is was is causing the
                // Message Stack will be available when loaded from the admin
                $GLOBALS['messageStack']->add(sprintf(USU_PLUGIN_WARNING_TABLE, TABLE_USU_CACHE), 'warning');
            }
        }

        // Determine canonical (if needed)
        $this->canonical();

        // Start logging
        $this->debug = false;
        if (defined('USU_DEBUG') && USU_DEBUG == 'true') { 

does this fix your redirect as well as preserve the fix to your original problem?

Can confirm that if this line is added back in, it fixes the 301 redirects for products that exist but introduces back the redirection loop for deleted products. You can test it yourself, visit a product on your website & add a few numbers to the product id to create a product that doesn't exist. With that line added back, you will/should get the redirection loop. Without that line the loop is fixed, but the 301's are broken for canonicals.

[carlwhat]

Can confirm that if this line is added back in, it fixes the 301 redirects for products that exist but introduces back the redirection loop for deleted products.

You can test it yourself, visit a product on your website & add a few numbers to the product id to create a product that doesn't exist. With that line added back, you will/should get the redirection loop. Without that line the loop is fixed, but the 301's are broken for canonicals.

who knew?

Well, it only took me 3 hours to figure out what the heck is going on! I'll call that difficult.

The underlying issue is the (unwanted) help provided by /includes/init_includes/init_sanitize.php when an invalid products_id variable is included in the URL. That module, for the current v3.0.8 and earlier versions of USU, is loaded prior to the USU intercept of calls to the zen_href_link function. When that module finds an invalid products_id, it performs a redirect to main_page=product_info&products_id=xxx ... which doesn't match the USU-generated link for that invalid product.

That mismatch, once USU gets activated, causes USU to issue a redirect to its 'proper' URL form which results in init_sanitize (on the redirect) to redirect to the base form ... you get the drift.

The update's going to be a tad complicated due to prerequisites, since the base language session variables aren't (by default) loaded until later in the start-up processing. Essentially, the USU start-up will now load itself prior to that init_sanitize module's load and will also register those base language variables (via init_languages.php) as part of its start-up.

I should have a beta version of USU v3.0.9 available shortly for 3rd party testing.

yeah, this problem has been around for a while. on my clients sites, i just comment out line 105 of the includes/init_includes/init_sanitize.php, ie:

      /**
       * do not recheck redirect
       */
      $_SESSION['check_valid_prod'] = false;
      //zen_redirect(zen_href_link($_GET['main_page'], 'products_id=' . $_GET['products_id']));
    } 

not ideal, but gets the job done.

best.

[John Large]

who knew?



yeah, this problem has been around for a while. on my clients sites, i just comment out line 105 of the includes/init_includes/init_sanitize.php, ie:

      /**
       * do not recheck redirect
       */
      $_SESSION['check_valid_prod'] = false;
      //zen_redirect(zen_href_link($_GET['main_page'], 'products_id=' . $_GET['products_id']));
    } 

not ideal, but gets the job done.

best.

Is that safe to do? I'm assuming the sanitzie functions are there to stop scripting and other such things in the url (could be wrong, not soemthign I've ever dug into). Also, does it break anything else that you have noticed?

I've just checked my own init_sanitize.php and dont have that line of code in mine.

[John Large]

Is that safe to do? I'm assuming the sanitize functions are there to stop scripting and other such things in the url (could be wrong, not soemthign I've ever dug into). Also, does it break anything else that you have noticed?

I've just checked my own init_sanitize.php and dont have that line of code in mine.

Well on my version of 1.5.6c (which seems to use a different version of init_sanitize.php) I've just edited out around line 140 and reverted USU to 3.0.8 and now both the 404's and the 301 redirects work.

 * do not recheck redirect
       */
      $_SESSION['check_valid'] = 'false';
      //commented out by john
      //zen_redirect(zen_href_link($_GET['main_page'], 'products_id=' . $_GET['products_id']));
    }

My only question, is this a safe way to acheive this? Will it stop urls being sanitized and leave us more vulnerable?

[lat9]

Is that safe to do? I'm assuming the sanitzie functions are there to stop scripting and other such things in the url (could be wrong, not soemthign I've ever dug into). Also, does it break anything else that you have noticed?

I've just checked my own init_sanitize.php and dont have that line of code in mine.

I wouldn't suggest that change, since I'm assuming that it's there for a good reason. Let me rework the redirect-loop changes to get the USU/Zen Cart integration working properly.

[carlwhat]

Is that safe to do? I'm assuming the sanitzie functions are there to stop scripting and other such things in the url (could be wrong, not soemthign I've ever dug into). Also, does it break anything else that you have noticed?

I've just checked my own init_sanitize.php and dont have that line of code in mine.

john,
sorry i did not notice you are on 156c. that file should look like this one:

https://github.com/zencart/zencart/b...t_sanitize.php

the line you need to comment out is line 140.

as far as safe to do? i think so. it makes it problematic when you upgrade as you need to remember what you did; but i have that running on clients sites for quite some time with no ill effect.

if @lat9 comes up with a more elegant solution that does not involve changing ZC core code, i am all for it. i know i spent many hours myself looking at it (at one time), and this just seemed to be the easiest solution.

as far as @lat9s comment, total respect, but that code has been there for 9 years. i see no good reason for it, and again, empirically, i see no ill effect.

best.

[carlwhat]

I wouldn't suggest that change, since I'm assuming that it's there for a good reason. Let me rework the redirect-loop changes to get the USU/Zen Cart integration working properly.

lets look at the code for a quick second, and specifically this var $_SESSION['check_valid_prod']:

/**
 * validate products_id for search engines and bookmarks, etc.
 */
  if (isset($_GET['products_id']) && (!isset($_SESSION['check_valid_prod']) || $_SESSION['check_valid_prod'] != false)) {
    $check_valid = zen_products_id_valid($_GET['products_id']) && !empty($_GET['main_page']);
    if (!$check_valid) {
      $_GET['main_page'] = zen_get_info_page($_GET['products_id']);
      /**
       * do not recheck redirect
       */
      $_SESSION['check_valid_prod'] = false;
      zen_redirect(zen_href_link($_GET['main_page'], 'products_id=' . $_GET['products_id']));
    }
  } 

the redirect is only suppose to happen once! we are first checking if that session var is set and not false. if so, lets set it to false, and then redirect. now the next time here, we should NOT hit the redirect as the session var is now false. but empirically we know that is happening.

while done with good intentions, this bit of code seems adrift to me. and has been for years.

but i am completely open to another opinion on it.

[lat9]

@lat9

in commit #42, in the usu class, it seems you have deleted line 100 which had the following code:

// Determine canonical (if needed)
        $this->canonical(); 

was this on purpose?

in my testing, this changes causes the automatic redirects to not happen.

now, whether that line of code is pertinent to the original redirect problem, i can not say, as i did not reproduce the original error.

john,
what happens if you add that line back?

starting at line 94 of includes/classes/usu.php, make it look like so:

            } elseif (IS_ADMIN_FLAG) { is was is causing the
                // Message Stack will be available when loaded from the admin
                $GLOBALS['messageStack']->add(sprintf(USU_PLUGIN_WARNING_TABLE, TABLE_USU_CACHE), 'warning');
            }
        }

        // Determine canonical (if needed)
        $this->canonical();

        // Start logging
        $this->debug = false;
        if (defined('USU_DEBUG') && USU_DEBUG == 'true') { 

does this fix your redirect as well as preserve the fix to your original problem?

Noting that the call to $this->canonical in the usu class' constructor was moved (as part of the redirect-loop correction) to the USU's observer class upon receipt of a notification issued at the beginning of the init_canonical.php's processing.

The subsequent issue caused by that move is that the redirect handling for 'renamed' products/categories needs to move also to the canonical-link generation phase of a page's rendering.

[lat9]

lets look at the code for a quick second, and specifically this var $_SESSION['check_valid_prod']:

/**
 * validate products_id for search engines and bookmarks, etc.
 */
  if (isset($_GET['products_id']) && (!isset($_SESSION['check_valid_prod']) || $_SESSION['check_valid_prod'] != false)) {
    $check_valid = zen_products_id_valid($_GET['products_id']) && !empty($_GET['main_page']);
    if (!$check_valid) {
      $_GET['main_page'] = zen_get_info_page($_GET['products_id']);
      /**
       * do not recheck redirect
       */
      $_SESSION['check_valid_prod'] = false;
      zen_redirect(zen_href_link($_GET['main_page'], 'products_id=' . $_GET['products_id']));
    }
  } 

the redirect is only suppose to happen once! we are first checking if that session var is set and not false. if so, lets set it to false, and then redirect. now the next time here, we should NOT hit the redirect as the session var is now false. but empirically we know that is happening.

while done with good intentions, this bit of code seems adrift to me. and has been for years.

but i am completely open to another opinion on it.

@carlwhat, the more I look at that section of code, the more I wonder about it.

1. We're looking for a link with a products_id variable (once per session), so if someone's pounding on a site with multiple links all with invalid products_id values, we're only going to see the first one?

2. We've found a products_id that isn't valid ... so we're going to use that invalid value to look up its non-existent _info page and use that known invalid products_id as part of the redirection link?

I've got a headache ...

[carlwhat]

@carlwhat, the more I look at that section of code, the more I wonder about it.

1. We're looking for a link with a products_id variable (once per session), so if someone's pounding on a site with multiple links all with invalid products_id values, we're only going to see the first one?

2. We've found a products_id that isn't valid ... so we're going to use that invalid value to look up its non-existent _info page and use that known invalid products_id as part of the redirection link?

I've got a headache ...

yaa, i feel your pain. as i looked at this closer, i remember burning many hours on it many moons ago. pretty clear it does not work (or i could not find a solution) to me.

i've been running with that line commented out for a minimum of 6 years. that redirect code was in there starting 9 years ago (at least as far as i can tell from github).

perhaps off topic, but what does this have to do with sanitizing vars? is that not the script we are in?