Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Implement a fallback for redirect urls #10670

Merged
merged 7 commits into from
May 30, 2016
Merged

Implement a fallback for redirect urls #10670

Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
355033e
Implement fallback redirect url
matrikular May 27, 2016
9c20a24
Satisfy Travis (CodeSniffer)
matrikular May 29, 2016
b11e2d2
Change all references from $dbo to $db
matrikular May 29, 2016
12ab79b
Move foreach outside the try catch block
matrikular May 29, 2016
9e65dd5
Use long method names for nameQuote and quote instead of qn and q
matrikular May 29, 2016
bca68b5
Satisfy Travis (Messed up the last changes)
matrikular May 29, 2016
53b4277
Satisfy Travis (Multiple empty lines)
matrikular May 29, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
200 changes: 104 additions & 96 deletions plugins/system/redirect/redirect.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,11 +101,9 @@ public static function handleException($exception)
*/
private static function doErrorHandling($error)
{
// Get the application object.
$app = JFactory::getApplication();

// Make sure the error is a 404 and we are not in the administrator.
if ($app->isAdmin() || $error->getCode() != 404)
if ($app->isAdmin() || ((int) $error->getCode() !== 404))
{
// Proxy to the previous exception handler if available, otherwise just render the error page
if (self::$previousExceptionHandler)
Expand All @@ -118,125 +116,135 @@ private static function doErrorHandling($error)
}
}

// Get the full current URI.
$uri = JUri::getInstance();
$current = rawurldecode($uri->toString(array('scheme', 'host', 'port', 'path', 'query', 'fragment')));
$uri = JUri::getInstance();

// Attempt to ignore idiots.
if ((strpos($current, 'mosConfig_') !== false) || (strpos($current, '=http://') !== false))
$url = rawurldecode($uri->toString(array('scheme', 'host', 'port', 'path', 'query', 'fragment')));
$urlRel = rawurldecode($uri->toString(array('path', 'query', 'fragment')));

$urlWithoutQuery = rawurldecode($uri->toString(array('scheme', 'host', 'port', 'path', 'fragment')));
$urlRelWithoutQuery = rawurldecode($uri->toString(array('path', 'fragment')));

// Why is this (still) here?
if ((strpos($url, 'mosConfig_') !== false) || (strpos($url, '=http://') !== false))
{
// Render the error page.
JErrorPage::render($error);
}

// See if the current url exists in the database as a redirect.
$db = JFactory::getDbo();
$query = $db->getQuery(true)
->select($db->quoteName(array('new_url', 'header')))
->select($db->quoteName('published'))
->from($db->quoteName('#__redirect_links'))
->where($db->quoteName('old_url') . ' = ' . $db->quote($current));
$db->setQuery($query, 0, 1);
$link = $db->loadObject();

// If no published redirect was found try with the server-relative URL
if (!$link || ($link->published != 1))
$dbo = JFactory::getDbo();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Core had at one point standardized on using $db for all database object references.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though I've changed the lines to use $db instead of $dbo, we should then update / change the CodeSniffer rules since they discourage having variables shorter than three characters.

Copy link
Contributor

@photodude photodude May 29, 2016 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@matrikular there is a possibility that we might be able to customize the variable length check with an exception for $db. Generally, I think we should keep the standard to at least Three characters and make a few limited exceptions for things like $db. That or we should fix all of the $db references to $dbo so that it complies with the actual written standard and quite trying to have standards that violate our other standards.

left hand doesn't know what the right hand is doing...


$query = $dbo->getQuery(true);

$query->select('*')
->from($dbo->qn('#__redirect_links'))
->where(
'('
. $dbo->qn('old_url') . ' = ' . $dbo->q($url)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likewise, core had standardized on the long quoteName and quote method names over the shortcuts.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've also changed the code to use the long method names. I still count 452 matches for "->qn(" within a blank Joomla! installation though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

*sigh* I'm sure whoever went through the work to standardize that feels their time was wasted now.

. ' OR '
. $dbo->qn('old_url') . ' = ' . $dbo->q($urlRel)
. ' OR '
. $dbo->qn('old_url') . ' = ' . $dbo->q($urlWithoutQuery)
. ' OR '
. $dbo->qn('old_url') . ' = ' . $dbo->q($urlRelWithoutQuery)
. ')'
);

$dbo->setQuery($query);

$redirect = null;

try
{
$redirects = $dbo->loadAssocList();

$possibleMatches = array_unique(
array($url, $urlRel, $urlWithoutQuery, $urlRelWithoutQuery)
);

foreach ($possibleMatches as $match)
{
if (($index = array_search($match, array_column($redirects, 'old_url'))) !== false)
{
$redirect = (object) $redirects[$index];

if ((int) $redirect->published === 1)
{
break;
}
}
}
}
catch (Exception $e)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should only catch the new JDatabaseExceptionExecuting exception object, not all Exceptions. This catch is too eager because it tries to potentially handle errors it should have no awareness of.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, never was my intention to catch any other exceptions. That part should be outside the try block of course.

{
$currRel = rawurldecode($uri->toString(array('path', 'query', 'fragment')));
$query = $db->getQuery(true)
->select($db->quoteName(array('new_url', 'header')))
->select($db->quoteName('published'))
->from($db->quoteName('#__redirect_links'))
->where($db->quoteName('old_url') . ' = ' . $db->quote($currRel));
$db->setQuery($query, 0, 1);
$link = $db->loadObject();
JErrorPage::render(new Exception(JText::_('PLG_SYSTEM_REDIRECT_ERROR_UPDATING_DATABASE'), 500, $e));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why should the fact that querying the database table failed cause a 500 code to be sent in place of a 404? IMO given that at this point the system's established that it is a 404 error that should be used going forward regardless of whether the error handler hits any errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The result can never become that expected 404 if a database error occurs. Not only from the seo point of view you would want to know exactly whats wrong with that request.

Imagine the maybe far fetched example of the seo department getting in touch with you, stating that there are NNN 404 you should take a look at. Now, if you head over to your redirect rules nothing will look suspicious, causing you and the seo department to have long discussions about who is wrong and who is right. I'd rather be the guy who tells the seo department; no worries, something is wrong with the database / table, I'm already working on it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's an argument both ways. Considering this handler is only used when the system throws a 404 error, it seems a little odd that the handler would change to another code when handling an error unless a critical failure was reached while in the error handling closure. My gut says that since this handler is only recording additional information before displaying the error page that it should stay a 404 error; the user did hit a page that would result in a 404 but the recording of that data failed.

}

// If a redirect exists and is published, permanently redirect.
if ($link && ($link->published == 1))
// A redirect object was found and, if published, will be used
if (!is_null($redirect) && ((int) $redirect->published === 1))
{
// If no header is set use a 301 permanent redirect
if (!$link->header || JComponentHelper::getParams('com_redirect')->get('mode', 0) == false)
if (!$redirect->header || (bool) JComponentHelper::getParams('com_redirect')->get('mode', false) === false)
{
$link->header = 301;
$redirect->header = 301;
}

// If we have a redirect in the 300 range use JApplicationWeb::redirect().
if ($link->header < 400 && $link->header >= 300)
if ($redirect->header < 400 && $redirect->header >= 300)
{
$new_link = JUri::isInternal($link->new_url) ? JRoute::_($link->new_url) : $link->new_url;
$urlQuery = $uri->getQuery();

$oldUrlParts = parse_url($redirect->old_url);

$app->redirect($new_link, intval($link->header));
if (empty($oldUrlParts['query']) && $urlQuery !== '')
{
$redirect->new_url .= '?' . $urlQuery;
}

$destination = JUri::isInternal($redirect->new_url) ? JRoute::_($redirect->new_url) : $redirect->new_url;

$app->redirect($destination, (int) $redirect->header);
}

// Else rethrow the exeception with the new header and return
JErrorPage::render(new RuntimeException($error->getMessage(), $link->header, $error));
JErrorPage::render(new RuntimeException($error->getMessage(), $redirect->header, $error));
}

try
// No redirect object was found so we create an entry in the redirect table
elseif (is_null($redirect))
{
$referer = $app->input->server->getString('HTTP_REFERER', '');
$query = $db->getQuery(true)
->select($db->quoteName('id'))
->from($db->quoteName('#__redirect_links'))
->where($db->quoteName('old_url') . ' = ' . $db->quote($current));
$db->setQuery($query);
$res = $db->loadResult();

if (!$res)
{
// If not, add the new url to the database but only if option is enabled
$params = new Registry(JPluginHelper::getPlugin('system', 'redirect')->params);
$collect_urls = $params->get('collect_urls', 1);
$params = new Registry(JPluginHelper::getPlugin('system', 'redirect')->params);

if ($collect_urls == true)
if ((bool) $params->get('collect_urls', true))
{
$data = (object) array(
'id' => 0,
'old_url' => $url,
'referer' => $app->input->server->getString('HTTP_REFERER', ''),
'hits' => 1,
'published' => 0,
'created_date' => JFactory::getDate()->toSql()
);

try
{
$columns = array(
$db->quoteName('old_url'),
$db->quoteName('new_url'),
$db->quoteName('referer'),
$db->quoteName('comment'),
$db->quoteName('hits'),
$db->quoteName('published'),
$db->quoteName('created_date')
);

$values = array(
$db->quote($current),
$db->quote(''),
$db->quote($referer),
$db->quote(''),
1,
0,
$db->quote(JFactory::getDate()->toSql())
);

$query->clear()
->insert($db->quoteName('#__redirect_links'), false)
->columns($columns)
->values(implode(', ', $values));

$db->setQuery($query);
$db->execute();
$dbo->insertObject('#__redirect_links', $data, 'id');
}
catch (Exception $e)
{
JErrorPage::render(new Exception(JText::_('PLG_SYSTEM_REDIRECT_ERROR_UPDATING_DATABASE'), 500, $e));
}
}
else
{
// Existing error url, increase hit counter.
$query->clear()
->update($db->quoteName('#__redirect_links'))
->set($db->quoteName('hits') . ' = ' . $db->quoteName('hits') . ' + 1')
->where('id = ' . (int) $res);
$db->setQuery($query);
$db->execute();
}
}
catch (RuntimeException $exception)
// We have an unpublished redirect object, increment the hit counter
else
{
JErrorPage::render(new Exception(JText::_('PLG_SYSTEM_REDIRECT_ERROR_UPDATING_DATABASE'), 404, $exception));
$redirect->hits += 1;

try
{
$dbo->updateObject('#__redirect_links', $redirect, 'id');
}
catch (Exception $e)
{
JErrorPage::render(new Exception(JText::_('PLG_SYSTEM_REDIRECT_ERROR_UPDATING_DATABASE'), 500, $e));
}
}

// Render the error page.
JErrorPage::render($error);
}
}