[#9269] - Allow URLs with lengths up to 2048 chars in com_redirect, redo of PR #4781
- Closed
- 1 Mar 2016
- Medium
- Build: staging
- # 9269
- Diff
- richard67:long-urls-in-redirect-component
- Success The Travis CI build passed Details
User tests: Successful: Unsuccessful:
Redo of Pull Request #4781 for 3.5.0.
Summary of Changes
This is a redo of PR #4781 to allow longer URLs in com_redirect, thanks to @Kixo for his work.
This PR is not only an improvement, it also fixes problems with not unique URLs after conversion from utf8 to utf8mb4 and so should go into 3.5.0 RC.
All 3 URL columns for old and new URL and the referrer are enlarged to max. 2048 characters.
I had to change a few things compared to the original #4781:
- Use 2048 = sitemap protocol limit and not 2083 = old IE limit
- Use new update sql file names
- Adapt to latest changes for utf8mb4
- Not completely remove the index but change it from a unique key to just a normal index, and on mysql limit the index to the 1st 100 chars for utf8mb4 compatibility.
What this PR does not change is that if the backend user creates a redirect link manually, i.e. by creating a new one in the backend, he is not warned or alerted when entering a too long URL.
Beside the enlargement of the URL colums, this PR corrects 2 older update sql scripts, where "KEY" insted of "INDEX" was used for adding or removing indexes, which is valid syntax but not recognized by the schema manager when testing schema status. This had no visible and testable effect when I tested, so maybe has to be accepted by code review.
Testing Instructions
This PR has to be tested with both new installations of current staging + this patch, find the source here: https://github.com/richard67/joomla-cms/archive/long-urls-in-redirect-component.zip
and with updating from a 3.4.8 to a current staging + this patch, find the source for updating with the extension installer here: http://test5.richard-fath.de/Joomla_3.5.0-beta3-Beta-Update_Package_test1.zip
or for updating with Joomla! update component using this as custom URL, here: http://test5.richard-fath.de/list_test1.xml
or instead of updating, apply this patch e.g. with the patch tester component to a current staging.
After an update (or apply pacth) you have to go to "Extensions -> Manage -> Database".
There should be no database problems reported about redirect_links table.
Fix database problems (at least the utf8mb4 conversion should be shown to be done) until all done.
Then on both new installation and updated system play around with
automatically creating redirect links by calling a not existing URL with length of 2048 characters in your domain, or
manually creating redirect links with old URLs having a lenght of 2048 characters in your domain
Test all aspects, e.g. enabling the redirect to an existing URL in your domain, sorting by old URL if you have created a few ones and so on.
Result: All should work as well as before but now with the long URLs.
A long URL to redirect to will not be easy to test because we cannot make just 1 menu item with (2048 - lenght of "protocol + domain name") characters long alias to have a long URL where we could redirect too 
Note: If we wanna test every aspect we need testers for Postgresql and Microsoft SQL (sqlazure), too.
One of the links regarding URL lengths which made me chose the 2048 as limit:
http://stackoverflow.com/questions/417142/what-is-the-maximum-length-of-a-url-in-different-browsers
| Status | New | ⇒ | Pending |
| Labels |
Added:
?
|
||
@andrepereiradasilva Can you test this and have a look if with long URLs, display is still ok for your taste, also on mobile and tablet, and let me know if you have ideas for improvement?
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9269.
@810 Please test, too, if you can.
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9269.
At a skim through this looks good. Will give it a proper test later this evening when back from work :)
Well another solution would have been to zip the URLs before saving in the database ... and unzip on reading ..... (joke)
ROFL
@RCheesley @anibalsanchez @waader Anyone time to test this PR here? I ask because you tested #4781 , of which this one here is a remake.
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9269.
I have tested this item 
unsuccessfully on 993407c
So I successfully installed from scratch but when doing my favourite install 3.4.8, then checkout your branch, then run the database fixer then I get the following issue:
1071 Specified key was too long; max key length is 767 bytes SQL=ALTER TABLE `#__redirect_links` MODIFY `old_url` VARCHAR(2048) NOT NULL;
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9269.
Ok, have to check.
Thanks :)
@wilsonge Hmm, I cannot see any reason. Can you save your Joomla folder from after pulling my branch into 3.4.8 and then on clean 3.4.8 again try with my update container or with the custom url link i provided in test instructions?
And if this works, make a comparison between that what you had after pulling this patch into 3.4.8 (save before update test again)?
I see no explanation for that except you missed some of my changes.
My zip contains staging as it was when i packed it, e.g. other changes beside those in my branch, but these should not have any effect. was the thing with TEXT and TINYTEXT and so on,.
I didn't manually apply - I literally pulled your branch down with
git fetch upstream pull/9269/head:redirects
git checkout redirects
So I can't have missed your changes :/
I'll try the update through Joomla Update though :)
Yes, try please. it could help to find the differences maybe.
When using your method I can replicate the problem now, so sorry for being paranoid.
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9269.
OK Joomla Update works. The difference is (I think) that the Joomla Update doesn't run the UTF8MB4 checks - whereas the database fixer will do the 2048 character upgrade THEN do the utf8mb4 upgrade
Well after Joomla! update youn have to so the same, run the database checks and this does the update sqls and then the utf8mb4 fixes.
We can hack a bit and do stuff in the utf8mb4 scripts which we should do in update scrips, as you had done with your hack for that before, but i would like to avoid that.
Look into the new 3.5.0-2016-03-01.sql: U drop the old index, then enlarge the columns, then create the new index with a new name (more conforming even to how we normally name them, idx_columnname). The new index is a normal and not a unique one and klimited to the first 100 chars.
So all right.
When I look into my database then the index is still the old one.
So either the drop index failed, or mysql handles transactions and rolls them back when a complete sql script failed.
In the 2 conversion sqls i handle the new index then again: First I drop it in the 1st script where we not handle exceptions, then in the second script it is added again with same limit to 1st 100 chars. Is duplicate but according how we handle things and that the update script should do everything not related to utf8mb4.
Can you check and see what you can find out?
Please review my changes if I made some mistake. Otherwise if you see no reason for that, you can try to remove the last line where i create the nex index from the update script, 3.5.0.-2016-03-01.sql, but leave the 1st line in with the drop, this we need, and then try again.
@wilsonge did you pull my complete branch? Or did you use staging and integrated my PR as patch? Please try the latter. The difference between my zips and my branch is that in my zips there is already #9267 included, as it is now in current staging, but the branch/fork of this pr does not have #9267 included yet.
Sorry been pulled in a meeting - will be back on this in about 30 mins!
It seems that the problem is that the old index is not dropped, which should happen with 3.5.0-2016-03-01.sql
Ahhhhh I have the mistake: There is something wrong with the changeitem for mysql. It does not handle "KEY" instead of "INXEX" anymore, so all "ADD KEY" or "DROP KEY" does not work for the update scripts running by the schema manager. When updaing with my zip or url, the scrips are run my sql directly and so no problem. I can use "INDEX" instead of "KEY" but I think there was some old update sql also using "KEY", and also the utf8mb4 scripts use that (those are run by sql and not filtered by the schema mangger before and so no pronlem, but when we have scripts where we use "KEY", like massively in the joomla.sql, then we have to allow this, too, for the update script, in my opinion.
Beside my new 3.5.0-2016-03-01.sql, the 2.5.4-2012-03-19.sql uses "ADD KEY", and 3.0.0.sql uses "DROP KEY".
Weither we change these old scripts, too, or we change the schema manager like i did with my old abandoned PR.
1st I could do with this PR, but 2nd would be better as I wrote above.
I will do 1st now, wait my next commit and test again with your method while I then update my zip.
This PR has received new commits.
CC: @wilsonge
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9269.
This PR has received new commits.
CC: @wilsonge
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9269.
Apologies! Just got back. Testing now :)
Yes, is fixed now.
OK I think we're good. When I hit fix I get the profile fields table (but that's just because this PR hasn't been rebased on master since I merged your other PR fixing that). Just going to test installing again!
OK we're good
| Status | Pending | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2016-03-01 23:52:20 |
| Closed_By | ⇒ | wilsonge |
| Milestone |
Added: |
||
Merged
Oh, that was fast
Shall I do as PR for the schema manager (mysqli changeitem) to accept also "KEY" in future? I am sure this will happen again and again that epople use "KEY" in update sqls.
Sure :) I wouldn't mind seeing that
I will do so but am not sure if it will come in time for the RC.
It's ok. I think there's probably going to need to be another beta first - we've fixed several critical issues and I want to see things a bit calmer on the front before I release an RC
Anyway I fixed the old sql files here now so it is not urgent with the new PR. Am just testing the install from web things.
thanks!
@andrepereiradasilva In case if the long URLs cause display issues, may I ping you later to ask you for help if necessary?
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9269.