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

Targets with invalid character encoding (was: Maximum length of URLs?) #76

Closed
fredl99 opened this issue Dec 28, 2014 · 47 comments
Closed

Comments

@fredl99
Copy link

fredl99 commented Dec 28, 2014

Re: Shorty 0.4.4, ownCloud 7.0.4 (stable)

Hi,
Trying to manually shorten a URL like this one:
http://www.willhaben.at/iad/kaufen-und-verkaufen/kfz-zubehoer-motorradteile/audi-a6-c4-seitenspiegel-beide-107995093?adId=107995093
Shorty complains: "Sorry, that target is invalid!"
When using the Bookmarklet it results in an "Internal Server Error (500)" :(

When I manually cut away the portion from the righmost slash up to end of the URL then Shorty will accept it. But that's no useful target then.

Second: The selected states don't seem to work properly anymore. Any of "closed", "private" or "shared" results in a "forbidden" message when calling the short URL, regardless if or which user is logged in. No login-page appears either. The only working status at the moment is "public".

@arkascha
Copy link
Contributor

Thanks for the feedback!
I take the liberty to separate the second aspect into a separate issue there:
#77

@arkascha
Copy link
Contributor

I can reproduce an issue with creating a new Shorty using the URL you provided as target.

Targets can have a length up to 4096 bytes according to the implementation. The resulting length is actually much lower due to the usage of unicode to store idn domains and the like. But still this limit should not even closely be hit here.

I currently have no idea what the problem is. I will have to investigate.

@arkascha
Copy link
Contributor

Sorry, this is an issue with the IDN handling I implemented. That is quite buggy, my fault.
I am currently working on the upcoming version 0.5, that one will have the issue fixed.
Sorry!

@fredl99
Copy link
Author

fredl99 commented Dec 28, 2014

Thanks for your quick reply!
And especially thanks for your effort in making and maintaining Shorty!

I didn't really suspect the length itself causing the issue. The title was just remaining from the first glance, because unusual long URLs seemed to be problematic. Of course it would be kind of funny if a URL-shortener would be overwhelmed by being fed with long URLs 😀

I'm sorry I can't give any better hints because js is not by business. But maybe some of the chars within the URLs are the cause (#, %, ?, ...) Here's another one:
http://www.ubuntu-forum.de/artikel/64408/ben%C3%B6tige-hilfe-zum-sichern-meiner-umfangreichen-bildersammlung.html
Well, this one includes German Umlauts, but the former did not...

As a conclusion: Would you suggest to switch back to an earlier version of Shorty?

Thanks,
Fredl.

@arkascha
Copy link
Contributor

Hi!

On Sunday 28 December 2014 07:52:33 fredl99 wrote:

As a conclusion:
Would you suggest to switch back to an earlier version of Shorty?

Sure, you can downgrade, you can pick it from github.
That's probably the best option if you cannot wait for two days. I hope to
have the upcoming version out by Tuesday.

arkascha

@arkascha
Copy link
Contributor

OK, this turned out to be two issues combined, which made debugging somewhat annoying:

  • faulty handling of idn domain names
  • faulty handling of target page titles containing non-ascii characters

@arkascha
Copy link
Contributor

@fredl99 Could I ask you to help testing the upcoming versions?
you'd have to install Shorty-0.5.0 & Shorty_Tracking-0-3-0 from the owncloud/shorty repository at github using branch "stable7".
That would be a GREAT help!

@fredl99
Copy link
Author

fredl99 commented Dec 28, 2014

Sure I'm willing to help. I'm going to install it and will report.

(Didn't know when you plan to release 0.5, so of course I can wait 👍 )

@fredl99
Copy link
Author

fredl99 commented Dec 28, 2014

Ok, did it and 0.5.0 seems to work fine.
At least both of the URLs mentioned before are processed correctly now. Good job! 👍
Only two minor flaws:

  • Umlauts in the "Title" field are garbled.
  • I cannot expand the rightmost columns with the new slider

Ok, I recognized the "slider" is actually a "click/double-click" element. Sorry...

I have not evaluated shorty_tracking 0.3.0 by now.

Only one question: shorty and shorty_tracking are in a common directory now, which also contains another subfolder "l10n". Is that on purpose with a special meaning?

@fredl99
Copy link
Author

fredl99 commented Dec 28, 2014

One more thing:
The column "Last" says "1970-01-01 01:00:00" until the first access takes place.

Feature request: Wouldn't it be nice to have the table sorted by clicking on a header field? (Instead of shrinking/expanding the according column)

Please feel free to cut and move this testing thread once again.

@arkascha
Copy link
Contributor

OK, I understand:

  • the "collapsible columns" are working for you. Fine.
  • shorty and shorty_tracking have always been maintained in a common repository. No change here. Also the 'l10n' ("internationalization") folder has always been there, that is part of the owncloud enironment (their translation strategy). So that should be fine.
  • I will check the "umlauts" in the title field

About clicking a column title to sort it:
I can only have one action on that event, I thought collapsing would make most sense here.
If you want to sort a column: you ARE aware of the "toolbar" shorty tables offer? In earlier versions that was opened when clicking a column title, that is not possible any more, indeed. Instead you now have to click the triangle in the left most column title cell to show/hide the toolbar. That toolbar gives you options like column sorting and filtering. I think in an app like Shorty sorting is not such a common action. Filtering probably is more frequent.
Do you have a suggestion how to emphasize how to open the toolbar? I am not really happy with that feature being kind of hidden now...

@arkascha
Copy link
Contributor

@fredl99 Another thing: about the "1970-01-01 01:00:00" for the "last access" date:
what database engine do you use? Seems to depend on that...

@fredl99
Copy link
Author

fredl99 commented Dec 28, 2014

I must admit I'm not very familiar with Shorty. In general, my owncloud-server is not as heavily used as it should be. But I'm working on it as you can see. Once I started to play with it, the problems came along. (Not really, only joking!)

So, thank you for pointing me to the triangle. Meanwhile, by switching a bit back and forth between 0.4.4 and 0.5.0 I discovered the sort option in 0.4.4 and I think I like it more. At least I discovered it by myself...
The shrink/expand feature does work, but the first impression was irritation because the right three columns were way too thin initially. Took me a little while until I understood the meaning of the "double arrow". (I was dragging it left and right at first)

Filtering is cool indeed, but I like sorting, too. One can see the popularity of Shortys at one glance. Or which ones are old and outdated. That's useful imo.

As for the database, I'm using MySQL. The access date isn't such an issue, as it's corrected after the first access.
But more important: Sorting the table doesn't work anymore. I carefully double-checked against 0.4.0 to eliminate a JS/Browser problem. 0.4.4 sorts instantly, 0.5.0 does nothing at all. 🙅

One more thing: When I edit a Shorty, it's entry is showing "undefined" in title and target fields, and "never" in expiration, creation and last date fields. But it still works and is correct again after reloading the page. 0.4.4 doesn't do that.

Didn't know about the common repo / folder hierarchy. I always had a standard install of OC with Shorty enabled. At least in the latest versions there where only those two folders within the "apps" directory. ATM I'm just switching symlinks between "dist" and "test" folders outside of "apps".

@arkascha
Copy link
Contributor

I fix the visualization of modified Shorty directly in the lists, that was indeed broken. Thanks!
I currently see these issues unsolved:

  • initial access date which should be "-never-" but is "1970-01-01...."
  • sorting by column (just saw this now, thanks again)
  • there is need to enhance the UI: indication of collapsing and toolbar needs improvement

Your feedback is really helpful, thank you so much!
I will address the issues tomorrow, too tired tonight ;.)

@fredl99
Copy link
Author

fredl99 commented Dec 28, 2014

I have to thank for the quick solutions.
I agree with the first two issues in your list. But I don't really recognize the need for collapsing the columns.
I'd rather concentrate on the Umlaut (charset) issue in the title and description fields. (Looks like this: [gelöst] Benötige Hilfe ...)

BTW: I have cloned the github branch "stable7", so I can quickly fetch new commits for testing. Just let me know.
gn8

@arkascha
Copy link
Contributor

I merged a few fixes. Amongst it a fix for the "umlauts issue" in the title aspect.

Turned out the original strategy was quite ok, so I reverted some details again. Actually the title tag inside your example page at http://www.willhaben.at/.... is invalid! It contains an invalid utf sequence which made my title extraction routine crash. I made my routine more robust, so that it ignores such invalid titles now. Seems to work. Impressive quality that "willhaben.at" service offers. Been a while since I last saw a professional service unable to handle character encoding :-)

Could you give this a try and test if umlauts work for you now?

@fredl99
Copy link
Author

fredl99 commented Dec 29, 2014

Looks much better now!

  • The title field is set correctly, even with Umlauts. But a '&' is still converted to &.
    I suppose angle brackets will be similar. (Though I haven't seen any of these in a title recently)
    I wasn't aware of the wrong character encoding at willhaben.at. But this is something that can happen everywhere. Better be prepared :) Excuse my lack of knowledge, but what exactly is the problem with copying the content of a <title> tag into the text field? Doing it manually works fine.
  • The "last access" date/time is set correctly or "never" now. No more Unix birthday :)
  • Entries are still OK after manually editing
  • Open:
    • still no sorting of the table
    • for some reason the bookmarklet in Google-Chrome quit working. The shorty page will open, but no new entry is created. In Iceweasel it still works. Maybe some cache problem in Chrome.
    • There is no calendar popup when editing an entry. It's only there at the initial creation. The date can then only be typed into the field, but won't updated in the list view. But the new value is still in the date field when opening the entry for reviewing or editing... (?)
    • some favicons will show up or sometimes not, even from the same target site. Especially bad: favicons with a wrong address will show a "missing graphics" icon. Example: http://www.webidsupport.com/forumsfavicon.ico which is obviously a typo in the target site (missing slash). I would suggest to drop favicons, because they also cause "unsecure element" warnings in https resources. Alternatively they could be fetched and delivered from the OC server via the current connection. (Which could be a legal issue...). Still there would be a bunch of validity checks necessary to avoid missing graphics.

That's all I saw by now :)
Fredl.

@fredl99
Copy link
Author

fredl99 commented Dec 29, 2014

Oh, I forgot:
When I enter a title manually (like for willhaben), the entry field allows exactly 80 chars, whereas an automagically filled-in title fits entirely.

@arkascha
Copy link
Contributor

Great, what a wealth of information! Priceless! Are you for hire? :-)

  • I will check the encoding of the "&", got an idea where it comes from.
    The "copying of a <title> tag is actually a little more complicated: the meta data is retieved by the server, not the client. For this client and server communicate via http/json (an ajax request). So the content if the title tag is handed over from server to client inside a json structure. That structure breaks if there is invalid utf content in there (somewhat understandable, though not convenient). This is an issue that is not easy to work around, since the string received is illegal. This situation is as if you find a sound wave spit out by a loudspeaker inside your letter box: you won't be able to open that "message". but still you most likely do NOT want to implement a workaround conversion in your letter box which would cost several million bugs for some incident that clearly never will occur :-)))))
  • didn't really expect my minor change here to fix the "last access" date/time issue, but ok!
  • so manually updating the list after a Shorty has been manipulated works. Great.

  • Sorting by a table column has to be fixed. Will look into it tonight.
  • I'll check again the Shortlet, but it appears to work for me. No idea currently.
  • The calendar popup in the "edit" dialog has always been missing. There are reasons for this, I will have to reimplement a few details for this issue. until now I was too lazy to do this myself. I wish I could find some contributors!
  • I am working on loading the favicons via the server instead of directly. This means a caching strategy which would make sense. But that feature is not ready right now.
  • no idea why the title is cut of after 80 chars when typing it in manually. I will look into that too :-)

Thanks again!

@fredl99
Copy link
Author

fredl99 commented Dec 29, 2014

Thank you for the explanation of the title problem. I see, it's more complicated than somebody whose coding skills end at HTML3/4 and CSS2 would expect 😄

Regarding the bookmarklet you shouldn't waste any time. As I stated it still works with Iceweasel/Firefox and I also checked with Qupzilla now.
The manually entered expiration date would have a higher priority in my opinion. I don't know if it's only not updated in the list view or ignored altogether.

The 80 chars boundary is funny: When a correct title is already filled in, chars can be deleted but not appended again. But it's completely there at the beginning. When I try copy&pasting, it is truncated to 80 chars.

@fredl99
Copy link
Author

fredl99 commented Dec 29, 2014

N.B.:

I don't know if it's only not updated in the list view or ignored altogether.

It's only in the list. 0.4.4 shows correct dates, even when they where altered by 0.5.0 before.
0.4.4 also does it right when changing it again, even a date like 2015-02-31 is re-calculated correctly 😄

@arkascha
Copy link
Contributor

@fredl99 Which repository did you clone to update your test environment:

  • the owncloud/shorty repo from the main owncloud project or
  • the owncloud-shorty repo from my personal account?

Sorry, cannot see that in github. I ask because I just wonder where to publish my changes for you to test.

@arkascha
Copy link
Contributor

Fixed:

  • max length of title in dialogs
  • appearance of toolbar elements in tracking table
  • filtering the tracking table by columns
  • updating of expiration date in list of Shortys
  • prevent closing of the toolbar when filters/selections apply
  • table filtering
  • table sorting

Unbelievable how many regressions were introduced by the DOM changes in 0.5.

@fredl99
Copy link
Author

fredl99 commented Dec 29, 2014

I cloned from https://github.com/owncloud/shorty. I thought this is what you were referring to earlier

If you have some spare time: Could you check if there could be some occasional error in parsing URLs of favicons? I have collected two examples which obviously fail due to a missing slash:
http://www.webidsupport.com/forumsfavicon.ico
http://www.no-access.de/defavicon.ico
Or could this be coincidence?

Going to pull your fixes...

@fredl99
Copy link
Author

fredl99 commented Dec 29, 2014

  • max length of title in dialogs ✅
  • table sorting ✅
  • table filtering ❓
    ...did work before, but if a filter is set and an entry is edited, so that it wouldn't match anymore, it's still visible. (i.e. filter is set to "private", one of the entries is changed to "shared")
    Shouldn't it disappear then? (This was the same before)
  • updating of expiration date in list of Shortys:
    Now changes to Unix timestamp format until the page is reloaded. (Not that I couldn't convert by myself, but... 😃

Still have to look at shorty_tracking.
All in all: Great work! 👍

@arkascha
Copy link
Contributor

About the table filtering: not sure of the entry should vanish in cases you describe. This is an ages old discussion. The problem: The entry you edit vanishes and the user has no feedback at all and cannot revert the change. That constantly leads to confusion and frustration.
Indeed formally this is a break in the logic. But the same thing is done when you add a new Shorty: it is always added at the top of the list of Shortys, regardless of how that is sorted. Same reasons: you should be able to see a feedback and use the created entry right away. Difficult if the new entr is buried deep inside a long list which often does not even show the right chunk (page) of the list.

@arkascha
Copy link
Contributor

I just merged a reimplementation of the "collapsible column feature".
I changed the control since I understand your hesitation to accept the changed reaction when clicking a columns title.
So the old behavior is back now: clicking a column title always toggles the toolbar below the title (easiert to "find" the toolbar like this). Collapsing/Expanding a column is now controlled via an additional icon in the toolbar, it's always the first icon, left of the sorting icons which got restyled too to make room for the additional icon. The click behavior is more logically, I think you will like it.
I am not yet happy with the changed visualization of the current sorting order, though. You got a better idea here than the bigger and darker icon? I had to drop the box around the icon due to the limited room I have in the toolbar. Always remember that many people use small screens. This also is the main motivation for the collapsible columns feature.

@arkascha
Copy link
Contributor

About the favicons and the example sites you gave:
The reasons why Shorty does not offer a favicon in both cases is a simple one: the pages do not use/define a favicon. Consequently Shorty shows no favicon in both cases. This is not an error, but something between a feature and a sane fallback...
I rechecked, and everything looks fine to me.
But I actually wonder where you got the favicon-URLs you posted from? They are not in the database, the list of shortys does not specify any img src url in these cases and the dialogs show an empty icon (built in to shorty). So where did you get these URLs from?!?

OK, pushed a small fix dealing with this, maybe things are better now for you.
Though I still do not understand where you got those URLs from :-)

@arkascha
Copy link
Contributor

Changing the expiration date of an existing Shorty results in a correct display as date in the reloaded list afterwards. I never get a timestamp here, actually that aspect is a date, no time associated. No idea how that can come out as a timestamp.
Or did I miss understand your comment "updating of expiration date in list of Shortys:" from two days ago?

Ah! I just realize you probably confuse the columns!
Indeed the column for the last access does get displayed as a timestamp, whyever!
I will look into that. Thanks!

OK, seems to be fixed too now :-)

@fredl99
Copy link
Author

fredl99 commented Jan 1, 2015

I think this thread is becoming a bit confusing now. Can you divide it into separate topics again, please?
This answer is about the favicons:
(examples: http://www.webidsupport.com/forumsfavicon.ico http://www.no-access.de/defavicon.ico)

the pages do not use/define a favicon.

They do, but the URL in Shorty's table is missing the last slash:
http://www.webidsupport.com/forums/favicon.ico -> works
http://www.no-access.de/de/favicon.ico -> works

But I actually wonder where you got the favicon-URLs you posted from?

There's a visual difference between the "empty icon" provided by Shorty and a "missing graphics" icon provided by the browser:
image(top: correct, middle: replaced by shorty, bottom: missing with wrong URL)

Right click on the icon and "open picture in new tab" or "copy picture URL" reveals the truth 😄

I think I know the reason: Both of these sites use relative links:
<link rel="Shortcut Icon" href="favicon.ico" type="image/x-icon" />
<link rel="SHORTCUT ICON" href="favicon.ico">
Whereas complete URLs work as they should:
<link rel="shortcut icon" href="https://ssl.gstatic.com/docs/doclist/images/icon_13_image_favicon.ico">

So if the base URL would end in a slash in the two examples, the favicon-URL would work, too.
And, I'm sorry to say, your latest fix doesn't help for that... 😉

@fredl99
Copy link
Author

fredl99 commented Jan 1, 2015

Filtering is not my main interest, so I'm fine as it is anyway.

About the collapsible columns, I will have a look at it, but just as a thought:

Always remember that many people use small screens. This also is the main motivation for the collapsible columns feature.

Of course I understood the intention behind this feature. The purpose is to give more room for a particular column. So why not make it a single-click action: i.e. clicking the "target" header collapses all other columns to their minimum width at once and expands the "target" column to the remaining space? A second click reverts all columns to their defaults.

Imho that would be nicer than repeatedly collapsing columns until the desired one gets enough room (and still won't be showing the entire content sometimes). Is this technically possible?

@fredl99
Copy link
Author

fredl99 commented Jan 1, 2015

The click behavior is more logically, I think you will like it.

Yes, I think it's more intuitive this way. Thanks!

I am not yet happy with the changed visualization of the current sorting order, though. You got a better idea here than the bigger and darker icon?

A darker triangle is the usual way to emphasize the sorting order, if any. It wouldn't have to be bigger too.

Regarding the single-click expansion mentioned before: The "double-arrow" mouse-pointer would also describe this action better than it does now.

@fredl99
Copy link
Author

fredl99 commented Jan 1, 2015

Re: Last access date

Or did I miss understand your comment "updating of expiration date in list of Shortys:" from two days ago?

You're right, my explanation was misleading. Regardless of the change, the time of the last access was shown in seconds of the Unix epoch afterwards. As I've tried it with the expiration date, I reported it wrong.

OK, seems to be fixed too now :-)

It is. ✅

@arkascha
Copy link
Contributor

arkascha commented Jan 1, 2015

  • favicon extraction issue is fixed
    sorry, no idea why I did not spot those links last time I looked. You clearly have the better detective genes of us two :-) Turned out that this was a really primitive bug: the 3 different options to code a reference (url, absolute and relative path) were already coded as different cases, but indeed for the relative path case a slash was missing. So relative favicon references simply never worked. Thanks for spotting this correctly.

Since this is fixed I won't separate it into a separate issue here.

@arkascha
Copy link
Contributor

arkascha commented Jan 1, 2015

  • Emphasis of the active sorting order: the zooming of the icon is important for barrier reduction. Think of visually impaired users having difficulties with contrasts. I always try to use two separate ways to emphasize something.

@arkascha
Copy link
Contributor

arkascha commented Jan 1, 2015

  • About the cursor for expanding a column:
    For me the current cursor is a cursor with two horizontal pointers, one left one right. But this might indeed be a question of the cursor theme loaded. None of the standard browser cursors appears more suited. Or do you suggest to change the icon, not the cursor when hovering?

I will experiment with different systems and browsers. I hesitate to generate a custom cursor for this...

@arkascha
Copy link
Contributor

arkascha commented Jan 1, 2015

  • About the logic to collapse a single column or to instead collapse all other columns if a title is clicked:
    The original idea was to allow to define how one does to have the table looking "normally". Note that the app stores which columns are collapsed and which are expanded. It will render the table the same when you use the app the next time. The feature was meant to allow temporarily expand single columns for a better overview.

Let's see if there is a way to get both ideas into a single solution :-)

@fredl99
Copy link
Author

fredl99 commented Jan 1, 2015

favicons:

Since this is fixed I won't separate it into a separate issue here.

Would have been a cheap opportunity to add to the "fixed issues" 😆

Emphasis of the active sorting order:
Think of visually impaired users

In no way I want to sound ignorant. But most browsers/OSs offer high contrast settings these users will
use anyway. But I won't talk against your decisions with this, since I'm not affected. Maybe a visually impaired user should respond on this.

For me the current cursor is a cursor with two horizontal pointers, one left one right.

Yes, same here. And for me it suggests "expand". That's why I meant it would fit to "expand this column to the max" by collapsing all others. Naturally another pointer would be nice for reverting all back to the last state (replace "default widths" as I wrote before, because I didn't consider that these settings are stored somewhere.)

@arkascha
Copy link
Contributor

arkascha commented Jan 1, 2015

Ah, I see what you mean with the cursor: currently the hovering cursor is the same for both actions: collapse and expand. Indeed misleading.
Unfortunately there is no opposite standard cursor. I hesitate to use a custom cursor because this cannot fit into more than a single cursor theme...

@arkascha
Copy link
Contributor

arkascha commented Jan 1, 2015

About separating issues:
Easiest is to create separate issues for separate things. It is easy to close separately defined issues, it is hard to split separate issues reported in a single entry.

Though certainly getting feedback, questions and hints on issues at all is most important :-)

@arkascha
Copy link
Contributor

arkascha commented Jan 1, 2015

Hm, doesn't expanding a column to max and collapsing all other columns only make sense for the target and title column, if at all?

@fredl99
Copy link
Author

fredl99 commented Jan 1, 2015

Sure. That's were I use this feature anyway. :)
So, "expand to the longest field length (of that column) or to the available space (with all other columns collapsed)" would indeed be a better common formula.

@fredl99
Copy link
Author

fredl99 commented Jan 2, 2015

A bit on the initial topic. I just saw this:

Error PHP iconv(): Detected an illegal character in input string at shorty/shorty/lib/meta.php#111

Do you need any additional input for this?

@arkascha
Copy link
Contributor

arkascha commented Jan 2, 2015

I'd say that is the error message thrown by php when trying to process the title with the invalid encoding used by willhaben.at I mentioned 4 days ago in this thread. I implemented a workaround for such issue and for me it works. But that does not mean that the sequence is not illegal any more. It is just ignored now :-)

@fredl99
Copy link
Author

fredl99 commented Jan 2, 2015

I'm sorry, it's from "2015-01-02T13:18:26+00:00" But I'm ready to ignore it too ;)

This one is around for a long time and it's still here:
{"app":"PHP","message":"Call to undefined function p() at \/shorty\/shorty\/templates\/tmpl_wdg_verify.php#39","level":3,"time":"2015-01-02T13:39:28+00:00"}

@arkascha
Copy link
Contributor

arkascha commented Jan 2, 2015

Yes, i know that message is still thrown. As said: it is ignored now (the issue) which was not the case before. I cannot change the way willhaben.at encodes their title tag. So this error will always be thrown, also in future, when such an illegal string is processed.

I separated the issue with the "undefined function p()" as #90.

@fredl99 fredl99 changed the title Maximum length of URLs? Targets with invalid character encoding (was: Maximum length of URLs?) Jan 7, 2015
@fredl99
Copy link
Author

fredl99 commented Jan 7, 2015

I renamed this issue reflecting the real problem behind it.
Since the original issue is now solved, I'll close here and open a new one regarding the discussion about the tables.

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

No branches or pull requests

2 participants