? ? ? Pending

User tests: Successful: Unsuccessful:

avatar tonypartridge
tonypartridge
19 Mar 2018

The redirects plugin always stores the absolute URL. This change adds a config option into the plugin to include the domain or not.

This means redirects are no longer domain dependant since we added relative redirect support a few versions back.

Config option set to include url by default for BC.

avatar tonypartridge tonypartridge - open - 19 Mar 2018
avatar tonypartridge tonypartridge - change - 19 Mar 2018
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 19 Mar 2018
Category Administration Language & Strings Front End Plugins
avatar tonypartridge tonypartridge - change - 19 Mar 2018
Labels Added: ? ?
avatar tonypartridge
tonypartridge - comment - 19 Mar 2018

should we be keeping it short when providing a description? But I’m easy either way. Thoughts @brianteeman?

On 19 Mar 2018, 15:16 +0000, Quy notifications@github.com, wrote:

@Quy commented on this pull request.
In administrator/language/en-GB/en-GB.plg_system_redirect.ini:

@@ -13,4 +13,6 @@ PLG_SYSTEM_REDIRECT_FIELD_EXCLUDE_URLS_REGEXP_DESC="Should the term be handled a
PLG_SYSTEM_REDIRECT_FIELD_EXCLUDE_URLS_REGEXP_LABEL="Regular Expression"
PLG_SYSTEM_REDIRECT_FIELD_EXCLUDE_URLS_TERM_DESC="A regular expression or a term which should be excluded."
PLG_SYSTEM_REDIRECT_FIELD_EXCLUDE_URLS_TERM_LABEL="Term"
+PLG_SYSTEM_REDIRECT_FIELD_STORE_FULL_URL_DESC="When we save the expired url should we include the domain name? If not we will store it relatively i.e. https://www.joomla.org/article becomes /article."
How about this to keep it short and simple without the references to we?
Save the expired URL as absolute (include domain) or relative (exclude domain).

You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.

avatar laoneo
laoneo - comment - 19 Mar 2018

I was wondering why the full url is stored at all? If it is not needed, then I would remove it and not make it configurable.

avatar tonypartridge
tonypartridge - comment - 19 Mar 2018

It is needed @Allon, some sites have multiple domains and https/http. Regardless if it’s right or wrong J3 does this 😬 and must continue so. There was a bit of backlash because my lag change forced lower case url redirects but some people were using upper and lowercase characters in random places....

J4 could go completely relative though?

On 19 Mar 2018, 18:40 +0000, Allon Moritz notifications@github.com, wrote:

I was wondering why the full url is stored at all? If it is not needed, then I would remove it and not make it configurable.

You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.

avatar infograf768
infograf768 - comment - 20 Mar 2018

Note:
When doing #19734 I remarked that it worked perfectly when the oldurls were created by the plugin, but NOT when creating them in the component manager.
Whatever the decision to include or not the absolute url, would be good to have a look at the fact that manual entries are not checked the same way as automatic.

avatar tonypartridge
tonypartridge - comment - 20 Mar 2018

How do you mean? For this pr we wouldn’t want to prevent the url in Manual entries given we still support urls it’s just this config sets the plugin to auto store in relative or absolute.

Or do you mean when adding manually they are not stored utf8? If so that’s an easy enough fix.

On 20 Mar 2018, 08:45 +0000, infograf768 notifications@github.com, wrote:

Note:
When doing #19734 I remarked that it worked perfectly when the oldurls were created by the plugin, but NOT when creating them in the component manager.
Whatever the decision to include or not the absolute url, would be good to have a look at the fact that manual entries are not checked the same way as automatic.

You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.

avatar infograf768
infograf768 - comment - 20 Mar 2018

Or do you mean when adding manually they are not stored utf8? If so that’s an easy enough fix.

They are stored as utf8, but we do not have the check for duplicates that we get for the urls entered automatically by the plugin.

avatar tonypartridge
tonypartridge - comment - 20 Mar 2018

@infograf768 at which point is this check needed? When we add a url it's checked if it exists before adding it? If it doesn't exist it isn't added.. if it exists it's updated..

Am I completely missing your point here?

avatar infograf768
infograf768 - comment - 20 Mar 2018

The check is not done when creating a new Redirect.
Let me make a gif.

avatar infograf768
infograf768 - comment - 20 Mar 2018

redirect

avatar tonypartridge
tonypartridge - comment - 20 Mar 2018

@infograf768 thanks! testing / checking now.

avatar tonypartridge
tonypartridge - comment - 20 Mar 2018

This was broken in 3.8.6 it worked in 3.8.5. Looking into it now.

avatar infograf768
infograf768 - comment - 20 Mar 2018

As long as it does not break #19734 😃

avatar tonypartridge
tonypartridge - comment - 20 Mar 2018

The problem is @infograf768 your pr breaks checks to validate against duplicate urls since it encodes them... on the stored URL. But the non-stored URL is not stored as rawurlencode.

Looking at a correct fix now

avatar infograf768
infograf768 - comment - 20 Mar 2018

Any working solution which also solves the original issue "solved" by #19734 is welcome

avatar tonypartridge
tonypartridge - comment - 20 Mar 2018

@infograf768 I've just tested without your PR and both:

http://mydomain.com/bogusurl
http://mydomain.com/bøgusurl

are treated separately. The problem with your PR is the check against the DB is never valid since we do not store as rawurlencode. It actually just breaks the check completely.

Are you saying both these links should be treated separately or equally?

avatar infograf768
infograf768 - comment - 20 Mar 2018

Are you saying both these links should be treated separately or equally?

They should be treated separately. That was the whole goal of my PR.
See the original issue
#19713

specially that part

On save it will give the error "Save failed with the following error: The source URL must be unique."
Test the URL and it will give you a 404 page. It will not use the redirect set in step No. 4.

avatar infograf768
infograf768 - comment - 20 Mar 2018

Basically, the check DOES work fine when the db is already populated by the plugin, but not when the redirect is fully created in the manager.

avatar tonypartridge
tonypartridge - comment - 20 Mar 2018

I’ve done a pr to revert your pr and will test throughly the difference between plugin and component created urls.

On 20 Mar 2018, 10:23 +0000, infograf768 notifications@github.com, wrote:

Basically, the check DOES work fine when the db is already populated by the plugin, but not when the redirect is fully created in the manager.

You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.

avatar infograf768
infograf768 - comment - 20 Mar 2018

If you don't mind, a PR to revert my PR is not the solution as 99% of needed redirects are done by the plugin and my PR works fine for these.

avatar tonypartridge
tonypartridge - comment - 20 Mar 2018

@infograf768, so I reverted your PR and ran some tests.

I enabled the plugin and accessed:
https://mywebsite.com/bøgusurl-auto

404 adding to com_redirects as:
https://mywebsite.com/bøgusurl-auto

I then did:
https://mywebsite.com/bogusurl-auto

404 adding to com_redirects as:
https://mywebsite.com/bogusurl-auto

Both redirect independently.

I also then tested creating manual entries, again both validations are unique, I cannot add https://mywebsite.com/bogusurl-auto if it exists and can add https://mywebsite.com/bogusurl-auto if https://mywebsite.com/bøgusurl-auto exists and https://mywebsite.com/bogusurl-auto doesn't.

Can you please test on your site with the PR reverted as pr my other pr. And provide a gif of the issue you are facing?

Many thanks

avatar brianteeman
brianteeman - comment - 20 Mar 2018

Unless you have some hidden tracking or a crystal ball you cannot say this.

99% of needed redirects

avatar infograf768
infograf768 - comment - 20 Mar 2018

@tonypartridge
siesta now. will post an animated gif of the original issue later on.

avatar infograf768
infograf768 - comment - 20 Mar 2018

@tonypartridge

This is the issue before my PR. To test:

  1. Revert #19734
  2. In frontend, enter a bogusurl, get 404, edit it in back-end, then try a bôgusurl, get a 404, edit it in backend, try to save.

This is what we get before my PR.
redirect_2

After my PR, we can save the bôgusurl expired url with a New URL.

avatar tonypartridge
tonypartridge - comment - 20 Mar 2018

now replicated thank you! Looking into it.

avatar tonypartridge
tonypartridge - comment - 20 Mar 2018

@infograf768 see #19950 for your UTF8 support.

Can we please get some tests on this PR in the mean time :-)

avatar infograf768
infograf768 - comment - 20 Mar 2018

#19950 works great! Needs another tester.

avatar infograf768
infograf768 - comment - 20 Mar 2018

I have tested this item successfully on 2499d3e

Tested OK here.


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

avatar infograf768 infograf768 - test_item - 20 Mar 2018 - Tested successfully
avatar tonypartridge
tonypartridge - comment - 20 Mar 2018

Done

On 20 Mar 2018, 23:03 +0000, Quy notifications@github.com, wrote:

@Quy commented on this pull request.
In administrator/language/en-GB/en-GB.plg_system_redirect.ini:

@@ -13,4 +13,6 @@ PLG_SYSTEM_REDIRECT_FIELD_EXCLUDE_URLS_REGEXP_DESC="Should the term be handled a
PLG_SYSTEM_REDIRECT_FIELD_EXCLUDE_URLS_REGEXP_LABEL="Regular Expression"
PLG_SYSTEM_REDIRECT_FIELD_EXCLUDE_URLS_TERM_DESC="A regular expression or a term which should be excluded."
PLG_SYSTEM_REDIRECT_FIELD_EXCLUDE_URLS_TERM_LABEL="Term"
+PLG_SYSTEM_REDIRECT_FIELD_STORE_FULL_URL_DESC="Save the expired URL as absolute (include domain) or relative (exclude domain)."
+PLG_SYSTEM_REDIRECT_FIELD_STORE_FULL_URL_LABEL="Include domain name in Expired URL"
Change domain name to Domain Name since labels are capitalized.

You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

avatar Quy
Quy - comment - 20 Mar 2018

I have tested this item successfully on de72198


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

avatar Quy Quy - test_item - 20 Mar 2018 - Tested successfully
avatar Quy Quy - change - 20 Mar 2018
Status Pending Ready to Commit
avatar Quy
Quy - comment - 20 Mar 2018

RTC


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

avatar mbabker mbabker - change - 25 Mar 2018
Status Ready to Commit Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2018-03-25 14:58:36
Closed_By mbabker
Labels Added: ?
avatar mbabker mbabker - close - 25 Mar 2018
avatar mbabker mbabker - merge - 25 Mar 2018

Add a Comment

Login with GitHub to post a comment