User tests: Successful: Unsuccessful:
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.
Status | New | ⇒ | Pending |
Category | ⇒ | Administration Language & Strings Front End Plugins |
Labels |
Added:
?
?
|
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.
It is needed @Allon, some sites have multiple domains and https/http. Regardless if it’s right or wrong J3 does this
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.
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.
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.
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.
@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?
The check is not done when creating a new Redirect.
Let me make a gif.
@infograf768 thanks! testing / checking now.
This was broken in 3.8.6 it worked in 3.8.5. Looking into it now.
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
@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?
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.
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.
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.
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.
@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
Unless you have some hidden tracking or a crystal ball you cannot say this.
99% of needed redirects
@tonypartridge
siesta now. will post an animated gif of the original issue later on.
This is the issue before my PR. To test:
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.
After my PR, we can save the bôgusurl
expired url with a New URL.
now replicated thank you! Looking into it.
@infograf768 see #19950 for your UTF8 support.
Can we please get some tests on this PR in the mean time :-)
I have tested this item
Tested OK here.
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.
I have tested this item
Status | Pending | ⇒ | Ready to Commit |
RTC
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:
?
|
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: