Skip to content

File download URLs have changed #1467

Description

@kuopassa

Expected behaviour

File download tag <txp:file_download_link /> should always create a download URL that keeps file_download in URL structure. Currently before-mentioned part is localized. Current arrangement causes link rot.

Actual behaviour

A localized download URL is created with <txp:file_download_link />. For example in English download URL could look like:

https://release-demo.textpattern.co/File+download/1/devrandom_100kb.txt

When language is switched to Finnish, the download URL becomes:

https://release-demo.textpattern.co/Tiedostonlataus/1/devrandom_100kb.txt

When language is switched to German, the download URL becomes:

https://release-demo.textpattern.co/Datei-Download/1/devrandom_100kb.txt

Steps to reproduce

Change Textpattern language.

Additional information

Textpattern version: 4.7.3

Activity

Bloke commented on Feb 5, 2020

@Bloke
Member

This was supposed to be a feature but you're right, it creates hassle. It affects /category and /author URLs too. I vote that we reverse this behaviour as follows:

  1. Allow either localised or fixed English URLs for the time being to resolve to the same resource.
  2. If a localised URL is given, redirect to the fixed, English URL.
  3. Fix tags that generate links so they create links to the English URLs.
  4. Deprecate the localised URL formats and broadcast this far and wide so that links can be updated at source if anybody is constructing them manually.

Although that should retain backwards compatibility, if it's too much work to squeeze into 4.8.0 at this late stage, we'll aim for 4.8.1.

@bloatware @petecooper @philwareham @phiw13 @cara-tm Anything I've overlooked with the above? Is that achievable or does it need a better approach?

petecooper commented on Feb 5, 2020

@petecooper
Member

When was this behaviour introduced, approximately? On the grounds this is a valid but not commonly-reported issue, personally I'm inclined to +1 the process outlined in #1467 (comment) and scope out the work involved for a fix in 4.8.0, given that as a significant release it'll have more eyeballs on it.

If this means the 4.8 release date is moved, so be it - my gut feeling says this is sufficiently important to resolve. Deprecation with sufficient time and warning is preferred, and we can use the post-4.8.0 release time to spread the word.

(Usual 'My 2c' tag goes here.)

philwareham commented on Feb 5, 2020

@philwareham
Member

Inclined to agree with Pete on this. It's valid enough to be needing a 4.8.0 (major release) fix.

Bloke commented on Feb 5, 2020

@Bloke
Member

Okay, I'm down with that plan. The localised URLs have been around ages. 4.2.0. maybe even earlier, with various local-aware URL schemes (e.g. images) added since that use the same underlying concept.

If we're clever about it, we can make this backwards compatible and then phase out the localised ones over time. Alternatively, if we're going to redirect anyway, we could just make the cutoff now. Any old, localised URLs get rewritten so manually-crafted links and SEO are auto-corrected. All tags generate the English-style link hooks. Might be less work.

added this to the v4.8 milestone on Feb 5, 2020

petecooper commented on Feb 5, 2020

@petecooper
Member

FWIW, I think this issue alone would be sufficient to release beta.3 ahead of RC, especially given the global nature of forum beta testers.

Bloke commented on Feb 5, 2020

@Bloke
Member

Handling both schemes is pretty easy in preText(). Anywhere there's a gTxt() construct, do this instead:

case 'section':
case urldecode(strtolower(urlencode(gTxt('section')))):
...
case 'category':
case urldecode(strtolower(urlencode(gTxt('category')))):
...

That'll allow either. No redirects, no nothing, it'll just permit both schemes. With just that change and making the tags output fixed English URL hooks, we're done. We can then think about phasing them out later.

Redirecting is more tricky as it'd take extra processing and (I think, unless anyone has any neat ideas on a teensy bit of refactoring in preText()) a little duplication. Do we need to do that?

bloatware commented on Feb 5, 2020

@bloatware
Member

IIRC, pretext() already works this way (edit: actually for file download links only). We only need to fix links generation in various tags.

Bloke commented on Feb 5, 2020

@Bloke
Member

Cool. I've got a mod that I'm testing now.

self-assigned this
on Feb 5, 2020

Bloke commented on Feb 5, 2020

@Bloke
Member

Seems that link generation is now only done in pagelinkurl() which is pretty sweet. All the tags that care about creating category/author links use it. I'm going to check images, though, as they're a special case.

bloatware commented on Feb 5, 2020

@bloatware
Member

I'm not sure image and other non-article context links are working at all.

Bloke commented on Feb 5, 2020

@Bloke
Member

Oh.

I'm hitting some weird stuff now too where the front end isn't rendering in the given language, but it might just be my site. Investigating...

bloatware commented on Feb 5, 2020

@bloatware
Member

Why wouldn't we universally use ?author=Parkling links, like when section is set? This would allow author etc to be valid section name.

Bloke commented on Feb 5, 2020

@Bloke
Member

We could. Uhh, should? I don't see any downside to allowing this as well. example.com/section?author=bob seems like a valid use case to me.

4 remaining items

petecooper commented on Feb 6, 2020

@petecooper
Member

I don’t deny that there is some kind of problem, but is it really urgent?

It's important to fix, in my opinion, and a non-patch release would be a great time to fix it. 4.8 is soon, 4.9 is a few years away, so I would err toward a resolution sooner with timely advice about any chances involved (i.e. "the way you did this has changed, or will change from next non-patch version"). It's not necessarily urgent, but the current moment is an appropriate time for at least an investigatory resolution, be that a 'fix' or plot out some advisory docs that can be included in 4.8 release.

Bloke commented on Feb 6, 2020

@Bloke
Member

While switching front-end languages is the major bugbear of this system, it's not the only problem. Internationalised URLs were meant to make things prettier and more welcoming for non-English users. In reality, weird character encoding issues, browser foibles and ugly strings of urlencode() characters in some languages were commonplace and made the URLs longer than necessary.

Plus, switching to English hooks is a) faster to process - when we eventually drop support for the international variants, and b) more determinant - anyone hooking into them can just test a single string instead of one of any number of possible strings depending on language.

It's been on my 'low-level annoyance' list for ages and I always thought it'd be too much hassle to implement due to major backwards-compatibility upheaval. But since it appears we can run the schemes side-by-side with little effect for a few versions, well, why not do it now as Pete says while we have a major revision dropping.

Yes, any links created via <txp:> tags will change after upgrade. But any and all existing manually-crafted links will continue to work, as will search engine inbound links and people's bookmarks. The only question is whether we should issue a redirect at core level now so links are gradually updated or if we leave that for a future (point or major) release.

If the changes I posted last night cause a problem in this or any other regard then we can choose to iron them out now or pull the commit and defer it. I'm happy either way but I would like to find a sane way to move forward and leave internationalised URL identifiers behind.

phiw13 commented on Feb 6, 2020

@phiw13

@Bloke

It's been on my 'low-level annoyance' list for ages and I always thought it'd be too much hassle to implement due to major backwards-compatibility upheaval.

[…] I'm happy either way but I would like to find a sane way to move forward and leave internationalised URL identifiers behind. (emphasis mine)

Yes we (you me) have discussed this in the past in some probably unrelated forum thread. Yes it does cause problems for some people, they may actually like their localised URL (catégorie / …). That is why I am slightly annoyed by the sudden hurry at the end of a testing period. I hope your commit doesn't cause too many problems, I won’t have time to test for another week or so.

BTW - what gonna happen with this test.local/catégorie/カタカナ/ ? (yes, カタカナ is the category name)

Bloke commented on Feb 6, 2020

@Bloke
Member

Hmmm, maybe you're right. Maybe this should be opt-in. If someone wants to use localised URLs, we could permit it via a Site pref.

Even though we may offer the ability to choose international vs english for link generation from tags, perhaps we leave the ability to process either in preText()? That saves us having to detect which to look for and (potentially) keeps the logic simple. Not sure yet. It might be better to detect which system is in use in preText() too, which avoids the possibility of duplicate content. We can discuss this.

Taking the above into consideration, this is something that can wait for a point release. It's more than just a simple tweak, requires a new lang string and will be backwards-compatible because the pref will default to 'yes, permit international URLs' for upgraders (and perhaps 'no' for new installs).

If I read the code correctly, file_download is already hard-coded in 4.8.0 so I think the OP is solved in the strict sense anyway. I'll check when I revert the commit and we'll see how it goes.

what gonna happen with this test.local/catégorie/カタカナ/ ?

Nothing happens there with the patch. It works fine and shows articles in that category. However, this construct does cause the new permlink schemes to choke. I'll raise a separate issue for that.

removed this from the v4.8 milestone on Feb 6, 2020

Bloke commented on Jul 5, 2020

@Bloke
Member

Revisiting this, it seems we have a number of options for /section, /category, /author, /file_download (and potentially future hard-coded triggers like /tag) :

a) enforce all such hooks in English. Phase out the l10n links.
b) permit both English and localized hooks, and either:
--> permlinks are generated in one of the two schemes depending on a pref (which requires a new translation string and a new pophelp: kinda late in the day to be doing this for 4.8.2).
--> defer the decision on which scheme to use to the tag level, e.g. introduce a lang attribute on <txp:category_list> and <txp:author_list> and all such tags that create hard-coded links via pagelinkurl(). Passing this $lang forward to pagelinkurl() would enable us to override the URL component to a given language. Default: front-end site language, same as today.
c) ditch URL hooks for messy syntax throughout. Not as pretty. And a backwards compatibility headache?
d) permit custom hooks to be rewritten. Configuration, not convention, though there is a convention at the outset.

As of right now, English AND internationalized hooks are valid, so anyone who wants a more consistent experience regardless of front-end language can opt to manually create such links in English, instead of relying on tags to construct them in local lingo.

Do we go further than this in 4.8.2 or do we rethink this whole thing in 4.9?

bloatware commented on Jul 5, 2020

@bloatware
Member

4.9 for me, it does not look urgent.

added this to the v4.9 milestone on Jul 5, 2020

phiw13 commented on Aug 4, 2020

@phiw13

See also some relevant comments in this forum thread here: https://forum.textpattern.com/viewtopic.php?pid=324671#p324671 and follow-up posts

added a commit that references this issue on Dec 22, 2023

bloatware commented on Dec 22, 2023

@bloatware
Member

Awaiting a pref, I have added lang attribute to <txp:page_url /> tag. It works like this:

<txp:page_url lang="">
<!-- txp URLs are not localized here -->
    <txp:page_url lang>
    <!-- here they are again -->
    </txp:page_url>
<!-- back to English -->
</txp:page_url>

Currently lang is binary (LANG|English), but could be extended when we go multilingual.

changed the title [-]File download URL's have changed[/-] [+]File download URLs have changed[/+] on Dec 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

Type

No type

Projects

No projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions