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.

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

Implement a fallback for redirect urls #10670

Merged
merged 7 commits into from May 30, 2016
Merged

Implement a fallback for redirect urls #10670

merged 7 commits into from May 30, 2016

Conversation

matrikular
Copy link
Contributor

@matrikular matrikular commented May 29, 2016

Introduction

The current implementation of the redirect application (component and plugin) expects a direct match to act uppon. So, if one wants to manually add a "rewrite rule" for e.g. http(s)://www.domain.tld/path/some-alias?some=param the rule would have to be exactly that url.

There are a couple of scenarios where query params could be added to any url, like when using a newsletter tool or if you've implemented certain affiliate mechanisms, where, especially in case of a relaunch of a website, you would have to create a whole bunch of redirect rules for each and every newsletter and / or affliiate link. Seldom this amount of different redirects can or should be handled via rewrite rules in a .htacces file.

This PR will add to the default behavior in a way that, if no specific (and published) redirect rule was found, it keeps looking for a "fallback".

Here is an example (behavior without the patch).:
If no (published) redirect rule for http(s)://www.domain.tld/path/some-alias?some=param exists, it will lead to a 404 status code.

After the patch, the plugin would also find http(s)://www.domain.tld/path/some-alias and would append the missing params automatically to the redirect url.

Summary of Changes

  • Removed unnecessary comment blocks e.g.$app = JFactory::getApplication(); or $uri = JUri::getInstance();are common practice and should not need to be explained.
  • "Reduced" the database access, refactored the insert / update methods and added separate try / catch blocks to support different error messages for both operations. I've also changed the error code in the catch block from 404 to 500 since in case of an error there, we're not dealing with a 404 anymore.

Testing Instructions


// 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

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...

@andrepereiradasilva
Copy link
Contributor

I have tested this item ✅ successfully on 53b4277

followed test instructions and worked fine.


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10670.

@chmst
Copy link
Contributor

chmst commented May 29, 2016

I have tested this item ✅ successfully on 53b4277

I've tested this successfully. Works as described, very useful!


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10670.

@zero-24
Copy link
Contributor

zero-24 commented May 30, 2016

RTC based on testing thanks 👍


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10670.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label May 30, 2016
@brianteeman brianteeman added this to the Joomla 3.6.0 milestone May 30, 2016
@zero-24
Copy link
Contributor

zero-24 commented May 30, 2016

Sorry the ???? was not expected 😄 I have just commented from my phone and i maybe hit the wrong button ;)


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10670.

@wilsonge wilsonge merged commit fc02236 into joomla:staging May 30, 2016
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label May 30, 2016
@wilsonge
Copy link
Contributor

Merged - thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants