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

[V4] UUID permalinks always redirect to default language page #5551

Closed
dviate opened this issue Sep 1, 2023 · 24 comments · Fixed by #6668
Closed

[V4] UUID permalinks always redirect to default language page #5551

dviate opened this issue Sep 1, 2023 · 24 comments · Fixed by #6668
Assignees
Labels
type: bug 🐛 Is a bug; fixes a bug
Milestone

Comments

@dviate
Copy link

dviate commented Sep 1, 2023

Description

When the languages are enabled, when you add a link to another page, the @/page/uuid-link is the same for every language.

Expected behavior
When I'm on a page in any non-default language, for instance site.com/es/sobre-nosotros/, where the default page link will be site.com/about-us/, I want to open the page in that language and not the default language.

To reproduce

  1. Enable languages in your config.php
/*
* https://getkirby.com/docs/reference/system/options/languages
*/
'languages' => [
	'detect' => false
],
  1. Make your own language your default language and add another non-default one, for instance Spanish
  2. Create a few page, type some stuff and add on one of those pages a link to another one

In your default language it works, when you open the Spanish version of the page and you click the link, it redirects you to the linked page in the default language. Editing the link on the Spanish version of the page doesn't help either.

Your setup

Kirby Version
4.0.0-alpha.7

Language files in site/languages/
NL (for me)

<?php
    return [
        'code' => 'nl',
        'default' => true,
        'direction' => 'ltr',
        'locale' => 'nl_NL',
        'name' => 'Nederlands',
        'url' => '/',
        'translations' => [ *etc*

ES (test)

<?php
    return [
        'code' => 'es',
        'default' => false,
        'direction' => 'ltr',
        'locale' => 'es_ES',
        'name' => 'Español',
        'url' => '/es',
        'translations' => [ *etc*
@distantnative
Copy link
Member

Permalinks could gain that we optionally add the language code at the end @/page/dojfppdz/de and then redirect to the right language.

And then the link field would need to support that in multilingual setups.

@distantnative
Copy link
Member

@lukasbestle What do you think of this idea to add language support to permalinks? Or do you have another idea?

@distantnative distantnative added the needs: discussion 🗣 Requires further discussion to proceed label Sep 9, 2023
@lukasbestle
Copy link
Member

In theory, a UUID could contain a slash if a custom generator is used. This would then break the language code separated by a slash. But I think the UUID cache already doesn't work with slashes in UUIDs, right? If that's the case, I agree with your suggestion. It's functional but also clean.

@distantnative
Copy link
Member

Could also be ?lang=de but I'd think that could have the same problem.

@lukasbestle
Copy link
Member

lukasbestle commented Sep 11, 2023

That would already not have worked as the question mark would have ended the path parsed by PHP/our router.

But to be honest, I think we can just ignore that potential issue I brought up. I think it's very unlikely that a) anyone actually built a custom UUID generator that b) uses slashes in the UUID and c) that worked at all before even with those slashes.


The ?lang=de syntax is interesting. It would allow us to define other params in the future or users could add custom params if needed. I can't think of a use case with our current setup, but it looks more future-proof than the /@/page/some-uuid/de syntax.

However the slash syntax is certainly cleaner if we can be sure that the syntax can stay like this. Adding more params to the end later would be a pain.

@distantnative distantnative changed the title [V4] Page links redirect to default language page on localized pages [V4] UUID permalinks always redirect to default language page Oct 19, 2023
@distantnative distantnative modified the milestones: 4.0.0, 4.0, 4.x Oct 19, 2023
@anvart
Copy link

anvart commented Nov 3, 2023

Also, urls with params (@/page/uuid/param1:value1/param2:value2) do not work, params simply get cut off.

@vibellio
Copy link

will this be fixed in the next rc?!
i think in every multilanguage setup this is a critcal bug, cause nearly any client will not understand why this is not wokring.
speaking of fully released kirby 4.0 version for production of course ;)

@tobimori
Copy link
Contributor

I agree that this is an issue that should be solved before the production release.

@distantnative
Copy link
Member

@vibellio @tobimori Is this a regression of v4? My understanding was that this issue exits for 3.9 as well

@tobimori
Copy link
Contributor

tobimori commented Nov 13, 2023

The issue exists for 3.9, yes, although 3.9 doesn't promote the usage of UUID permalinks in any way (hence it never got noticed)

Now with v4, all Page Writer links are not absolute paths but the permalinks, and there is no way to select the other language with those.

@dviate
Copy link
Author

dviate commented Nov 19, 2023

It's been 79 days since this issue got posted and it seems we're still stuck on the part of how to build the URL-structure; as @tobimori mentions to me it feels like a mandatory fix before the 4.0 Release will appear.

Some suggestions, with the es locale used like in the examples in my previous post:

  • /es/@/page/UUID (This suggestion seems to me to be the most logical; in terms of implementation/rewriting a URL and maybe even SEO wise. But, having said that, I don't have too much knowledge of all the backend.)
  • /@/page/UUID/es
  • /@/page/es/UUID

@distantnative
Copy link
Member

@dviate doesn't help if we rush this and then are stuck with something dysfunctional. I get that it can be frustrating when you want this solved for your project, but pressure won't help the quality of a solution.

I think there are two parts to this:

  • Finding a URL structure to include the language. @dviate has the options and I agree with the preferred one, if this can be done with the language router. Though we also have to support the old pattern as it would be a breaking change to v3
  • Will the language be hardcoded in the writer field HTML at insertion? Probably yes. It feels not great, but I don't see how we could insert/inject it only at rendering the field.

@tobimori
Copy link
Contributor

tobimori commented Nov 20, 2023

It feels not great, but I don't see how we could insert/inject it only at rendering the field.

It'd be great if we get a Field method like $field->withResolvedLinks() that resolves all links to their non-permalink counterparts. At least that could be a potential workaround for this issue.

@distantnative
Copy link
Member

@tobimori and that one just runs a regex on /@/page/myUUID patterns and replaces it with the resolved normal URL with current language? Gotta say, I have sympathy for such a solution, esp. given v4 timeline state and not deciding on a structural solution which we might regret a few months down the road. Still offering a solution in v4 then for those using th field method.

$page->myWriterField()->toResolvedUrls()

@bastianallgeier
Copy link
Member

I agree that an additional method to resolve permalinks is great.

For the language issue I would suggest this:

  • /es/@/page/UUID will be new and redirect to the specified language (as @dviate suggested)
  • /@/page/UUID still exists and keeps on redirecting to the default language

In a multi-language installation, we could add another field to the link dialog to select the language for a link. By default, the field would be set to the currently selected language and the specific permalink is added.

@distantnative
Copy link
Member

/**
 * Converts permalinks in the field value to absolute urls
 */
'toResolvedUrls' => function (Field $field): Field {
	if ($field->isNotEmpty() === true) {
		$field->value = preg_replace_callback("$\/@\/(page|file)\/([^'\"]+)$", function ($matches) {
			if ($uuid = Uuid::for($matches[1] . '://' . $matches[2])) {
				return $uuid->model()->url();
			}

			return $matches[0];

		}, $field->value);
	}

	return $field;
},

This would though match any other /@/something/something too. Does the regex need to include that the permalink needs to be enclosed by " or '?

@bastianallgeier
Copy link
Member

@distantnative I'd keep this pretty strict tbh. Ideally we could use the dom parser for this here to be sure that we only work on attributes. But I'm not sure what's more performant.

@distantnative
Copy link
Member

@bastianallgeier you're right, went with parsing the DOM - it's not an operation that we do a thousands of times, so I think the strictness here outweighs any potential performance difference #5976

@bastianallgeier
Copy link
Member

As mentioned a couple times before, this is tricky. We will mostly solve this now in RC.2 with the new $field->permalinksToUrls() method. There are more steps that need to follow. But we can't rush this as @distantnative said. It would be fantastic if you could help us test the method as soon as the RC is out later today.

@dviate
Copy link
Author

dviate commented Jan 29, 2024

Just out of curiosity: is there any news about this? 😃

@bastianallgeier
Copy link
Member

@dviate we are still working on more UUID fixes. They are tricky. That's why we need longer than expected. Sorry for the wait.

@bastianallgeier
Copy link
Member

At least there's now a solution for the first part. Language-specifc routes: #6312 We will keep on it.

@distantnative distantnative added type: bug 🐛 Is a bug; fixes a bug and removed needs: discussion 🗣 Requires further discussion to proceed labels Jul 24, 2024
@distantnative distantnative modified the milestones: 4.x, 4.4.0 Jul 24, 2024
@distantnative
Copy link
Member

@bastianallgeier First part finally done, I think the second part would be to return from Page::permalink() etc. a language-specific permalink for the current language when in multilang. Would you think so too?

@distantnative
Copy link
Member

Well, not quite. Cause the LinkField builds the URL itself.

@distantnative distantnative removed this from the 4.4.0 milestone Aug 13, 2024
@distantnative distantnative self-assigned this Sep 12, 2024
@distantnative distantnative linked a pull request Sep 12, 2024 that will close this issue
5 tasks
@bastianallgeier bastianallgeier added this to the 4.5.0 milestone Sep 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug 🐛 Is a bug; fixes a bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants