merge DecodeShortURLs and Redirectors plugins - #36
Conversation
|
The merge looks good but you're still keeping track of shorteners separately from redirectors via the I suggest dropping the The same for the eval rules: short_url() can just be an alias of redir_url() etc. The former to be removed in some future version. Most of these flags are being set identically anyway: Might as well set the flag once and have both rules read it. Then we just need one cache and one set of budget caps (max_redir_urls, etc.) and no more confusion over "which list does X belong in". Having one cache is a big win because currently we have two database handles, 8 statement handles, two autoclean blocks, and two SQL tables with nearly identical schemas. The only other snag is that DecodeShortURLs historically has been stripping the query string from URLs before fetching. Redirectors does not. The purpose of that is unclear to me. However, it's only safe to strip query strings if we're sure the query string doesn't carry any information about the target URL. That requires knowing the internal workings of every shortening service. The result is that we're fetching URLs that don't exist in the email. So we can't be sure we're getting the same response that the user would get. I suggest NOT stripping query parameters from any URLs. It's safe (and appropriate) to strip fragments (i.e. the part after a #) because fragments are meant to be processed client-side per RFC 3986. |
|
We can simplify code by using sub aliases but changing SQL schemas will break existing setups. |
Atm SHORT_URL_404 test fails but it's failing on trunk as well, this is probably related to a different http response code send by the shortener service.