Summary: | 856$u URL checker ( check-url.pl ) complains about ftp based URLs | ||
---|---|---|---|
Product: | Koha | Reporter: | Kyle M Hall <kyle> |
Component: | Architecture, internals, and plumbing | Assignee: | Kyle M Hall <kyle> |
Status: | In Discussion --- | QA Contact: | Testopia <testopia> |
Severity: | normal | ||
Priority: | P5 - low | CC: | jesse, kyle.m.hall, mtompset, tmisilo |
Version: | unspecified | ||
Hardware: | All | ||
OS: | All | ||
Change sponsored?: | --- | Patch complexity: | --- |
Documentation contact: | Documentation submission: | ||
Text to go in the release notes: | Version(s) released in: | ||
Attachments: | Bug 14530 - 856$u URL checker ( check-url.pl ) complains about ftp based URLs |
Description
Kyle M Hall
2015-07-14 13:22:00 UTC
Created attachment 41026 [details] [review] Bug 14530 - 856$u URL checker ( check-url.pl ) complains about ftp based URLs The script check-url.pl assumes that any URL not prefixed with http must be a relative URL. This is problematic as the URL could be prefixed with ftp as well. Any 856$u using an ftp url is given the error "400 URL Must be absolute". Since HTTP::Request cannot handle ftp urls, we should just skip them. I also noticed that check-url-quick.pl checks for prefixes with case insensitivity. That seems like a good idea so I've added that to check-url.pl Test Plan: 1) Apply this patch 2) Find or create a record with an ftp url in 865$u 3) Run check-url.pl 4) Note the ftp url is skipped 5) Run check-url-quick.pl 6) Note the ftp url is skipped Actually, couldn't you put an http URL for an ftp proxy into host? ./misc/cronjobs/check-url.pl --host=http://this.site.proxies.ftp.requests then it would attempt to go to http://this.site.proxies.ftp.requests/ftp://whatever/whereever/whatever.foo Personally, I'd prefer to see a re-write to allow for other protocols (bring back gopher! :) ). Because skipping the ftp URLs kind of defeats the purpose of the script which is to validate that the URLs exist. This could be a significant problem if a large portion of URLs are not http. Also, quick says it replaces the non-quick version. Perhaps we should delete the non-quick version? (In reply to M. Tompsett from comment #2) > Actually, couldn't you put an http URL for an ftp proxy into host? > > ./misc/cronjobs/check-url.pl --host=http://this.site.proxies.ftp.requests > > then it would attempt to go to > http://this.site.proxies.ftp.requests/ftp://whatever/whereever/whatever.foo > > Personally, I'd prefer to see a re-write to allow for other protocols (bring > back gopher! :) ). > > Because skipping the ftp URLs kind of defeats the purpose of the script > which is to validate that the URLs exist. This could be a significant > problem if a large portion of URLs are not http. That sounds reasonable, but not everyone will want to go to the trouble of setting that up. As a middle ground, I think adding a --skip-ftp parameter should suffice. (In reply to M. Tompsett from comment #3) > Also, quick says it replaces the non-quick version. Perhaps we should delete > the non-quick version? That sounds good to me. Would you want to file a separate bug report for that? If that bug/patch makes it into master before this one, I'll just have to update it. Thanks! BTW, I was bored a while back, and actually wrote a forking LWP version. I'll attach it later, once I actually test it in place. :) This is mostly a question about URL checking in general. This script checks for good links in 856 $ u? Cab it be run anywhere while logged into the server, or does it need to be run from a specific part in the file system? Just want to make sure. Thank you, Jesse |