New `language:variables` root #6212

afbora · 2024-01-26T14:07:46Z

This PR …

This was a more complex problem than I expected (always 🙈). Because the system should be able to read, update and delete language variables both from the language file and from a custom root. It involves a breakage change, but I wanted to remove the code complexity by creating a separate LanguageVariables object.

Fixes

External language variables setup no longer works since K4's language views External language variables setup no longer works since K4's language views #6176

Features

New language:variables root

$kirby = new Kirby([
    'roots' => [
        'language:variables' => __DIR__ . '/site/variables'
    ]
]);

Breaking changes

Language::translations() deprecated, use Language::variables() instead
$language->translations() returns LanguageVariables object instead array. Use $language->translations()->toArray() or $language->variables()->toArray().

Ready?

Unit tests for fixed bug/feature
In-code documentation (wherever needed)
Tests and checks all pass

For review team

Add changes to release notes draft in Notion
Add to website docs release checklist (if needed)

lukasbestle

Great solution to move the logic to a separate class. 👍
A few things I noticed while skimming through the changes:

src/Cms/Language.php

src/Cms/LanguageTranslations.php

tobimori · 2024-02-06T17:31:29Z

Excited for this, thanks! Will this include the possibility to add translations (e.g. for the panel) even if the language does not have a content language assigned to it? Some of my pages are German only, but should still have English translations for the panel.

afbora · 2024-02-10T12:31:00Z

@tobimori In your case; multilingual is not enabled for the frontend, but you want to add translations (for example for blueprint labels) for users in different languages using the panel. Do I understand correctly?

tobimori · 2024-02-10T12:32:25Z

@tobimori In your case; multilingual is not enabled for the frontend, but you want to add translations (for example for blueprint labels) for users in different languages using the panel. Do I understand correctly?

Multilingual is enabled, but the languages in the panel might differ from the languages in the frontend.
Let's say I have de and en in the frontend, but I want to add a french translation for my field labels etc. as well.

Currently, I have a translations directory and load yaml files from a plugin. (https://github.com/tobimori/kirby-baukasten/blob/main/site/plugins/project-extended/index.php#L20)

afbora · 2024-02-10T12:37:50Z

If there is no FR language (so fr.php language file), unfortunately it will not load custom FR translations. It will only load the translation files for existing languages from a custom root.

tobimori · 2024-02-10T13:02:30Z

😦

distantnative

Some first comments. Let me know if you think I misunderstood parts of the code :)

src/Cms/Language.php

distantnative · 2024-07-22T15:39:44Z

src/Cms/Language.php

+		if ($this->translations()->root() !== null) {
+			$this->translations()->save($data['translations'] ?? []);
+			$data['translations'] = [];
+		}


How about instead of unsetting it here, we do an if-else clause: The if part writes it to the dedicated root, the else part adds it to the $data array?

If custom root is already set (root !== null), it empties the data.translations array and writes to the custom root. If custom root is not set, it saves the data array normally as before.

What I meant:

$data = [ ...$existingData, 'code' => $this->code(), 'default' => $this->isDefault(), 'direction' => $this->direction(), 'locale' => Locale::export($this->locale()), 'name' => $this->name(), 'url' => $this->url, ]; ksort($data); // save translations to the custom root and remove translations // to prevent duplication write into the language file if ($this->translations()->root() !== null) { $this->translations()->save(); } else { $data['translations'] = $this->translations()->toArray(); }

(not passing anything to $this->translations()->save() here based on the other suggestion I make below)

We need to empty to $data['translations'] index if custom root defined. Because after few lines later will run Data::write() and same translation strings to be written to language translations index. $data['translations'] = [] prevents it.

But if we don't add it to $data in the first place, we wouldn't need to unset it, right? Or could $existingData also have a translations key?

Yes, $existingData mean /site/languages/*.php data and currently languages have translations key.

distantnative · 2024-07-22T15:40:50Z

src/Cms/Language.php

@@ -571,7 +580,7 @@ public function update(array|null $props = null): static
 		$updated = $this->clone($props);

 		if (isset($props['translations']) === true) {
-			$updated->translations = $props['translations'];
+			$updated->translations = new LanguageTranslations($updated, $props['translations']);


Does this work? I thought translations property is protected.

It was already protected: protected array $translations?

src/Cms/LanguageTranslations.php

afbora · 2024-09-13T20:43:25Z

@distantnative I did some changes based on your reviews and done some improvements. Would be nice if you take a look when you're available before writing unit tests and last touch 🙌

distantnative · 2024-09-14T10:38:53Z

src/Cms/Core.php

+			'sessions'     => fn (array $roots) => $roots['site'] . '/sessions',
+			'snippets'     => fn (array $roots) => $roots['site'] . '/snippets',
+			'templates'    => fn (array $roots) => $roots['site'] . '/templates',
+			'translations' => null,


Question: I don't have a good idea yet, but wondering if it's too confusing to have i18n:translations and translations? Maybe languages:translations?

@bastianallgeier What do you think?

What was our last decision for the language variable permissions? I wonder if we maybe should go the same route here with naming as for permissions?

For now to be languageVariables.create|update|delete.

We could go for language:variables maybe

I know that it totally clashes with the translations naming everywhere. But I think it makes sense to fully commit to one naming convention here and language variables are at least not giving us any more headaches with the entire content translations vs. ui translations monster.

Maybe even i18n:variables then?

Not sure for this PR, @bastianallgeier ?

I would say that it belongs to the language and thus the language namespace feels better to me.

My point there is that so far we don't have a language namespace anyways, we are creating a new one. And what if language is the i18n namespace?

This is still about the root and I think we are mixing them up here. The i18n roots are all in our kirby folder to store interface translations and slug rules. The language:variables root would be a custom project root inside of site.

distantnative · 2024-09-14T10:42:18Z

src/Cms/Language.php

+		if ($this->translations()->root() !== null) {
+			$this->translations()->save($data['translations'] ?? []);
+			$data['translations'] = [];
+		}


What I meant:

$data = [ ...$existingData, 'code' => $this->code(), 'default' => $this->isDefault(), 'direction' => $this->direction(), 'locale' => Locale::export($this->locale()), 'name' => $this->name(), 'url' => $this->url, ]; ksort($data); // save translations to the custom root and remove translations // to prevent duplication write into the language file if ($this->translations()->root() !== null) { $this->translations()->save(); } else { $data['translations'] = $this->translations()->toArray(); }

(not passing anything to $this->translations()->save() here based on the other suggestion I make below)

distantnative · 2024-09-14T10:44:14Z

src/Cms/LanguageTranslations.php

+	public function save(array $translations = []): static
+	{
+		$this->data = $translations;


Suggested change

public function save(array $translations = []): static

{

$this->data = $translations;

public function save(array|null $translations = null): static

{

if ($translations !== null) {

$this->update($translations);

}

src/Cms/Language.php

Co-authored-by: Bastian Allgeier <[email protected]>

src/Cms/Language.php

afbora added needs: discussion 🗣 Requires further discussion to proceed type: enhancement ✨ Suggests an enhancement; improves Kirby type: feature 🎉 Adds a feature (requests → feedback.getkirby.com) needs: tests 🧪 Requires missing tests labels Jan 26, 2024

afbora requested a review from a team January 26, 2024 14:07

afbora self-assigned this Jan 26, 2024

lukasbestle reviewed Jan 27, 2024

View reviewed changes

src/Cms/Language.php Outdated Show resolved Hide resolved

src/Cms/LanguageTranslations.php Outdated Show resolved Hide resolved

src/Cms/LanguageTranslations.php Outdated Show resolved Hide resolved

src/Cms/LanguageTranslations.php Outdated Show resolved Hide resolved

lukasbestle removed the needs: discussion 🗣 Requires further discussion to proceed label Jan 27, 2024

afbora mentioned this pull request Jan 27, 2024

[v4] New translations root #6216

Closed

5 tasks

distantnative added the needs: changes 🔁 Implement any requested changes to proceed label Feb 3, 2024

afbora added this to the 5.0.0 milestone Mar 29, 2024

distantnative force-pushed the lab/afbora/new-translations-root branch from 7f4acb1 to 178fa70 Compare July 22, 2024 15:33

distantnative reviewed Jul 22, 2024

View reviewed changes

distantnative linked an issue Jul 22, 2024 that may be closed by this pull request

External language variables setup no longer works since K4's language views #6176

Open

afbora changed the title ~~[v5] New translations root~~ New translations root Sep 1, 2024

afbora force-pushed the lab/afbora/new-translations-root branch 7 times, most recently from a18e0ef to b1b372f Compare September 13, 2024 20:35

distantnative reviewed Sep 14, 2024

View reviewed changes

afbora changed the title ~~New translations root~~ New language:variables root Sep 16, 2024

distantnative removed the needs: changes 🔁 Implement any requested changes to proceed label Sep 19, 2024

bastianallgeier reviewed Oct 1, 2024

View reviewed changes

src/Cms/Language.php Outdated Show resolved Hide resolved

src/Cms/Language.php Outdated Show resolved Hide resolved

src/Cms/Language.php Outdated Show resolved Hide resolved

afbora and others added 6 commits December 9, 2024 15:09

New translations root

659fd41

Apply code reviews

dd175e4

Improvements

4ff20d1

Rename root with language:variables

ea2e7ad

Swtich from ::translations() to ::variables()

14195e3

Apply suggestions from code review

e4d97b1

Co-authored-by: Bastian Allgeier <[email protected]>

afbora force-pushed the lab/afbora/new-translations-root branch from b56dad6 to e4d97b1 Compare December 9, 2024 12:10

github-advanced-security bot found potential problems Dec 9, 2024

View reviewed changes

src/Cms/Language.php Fixed Show fixed Hide fixed

Update Language.php

854ddb5

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New `language:variables` root #6212

New `language:variables` root #6212

afbora commented Jan 26, 2024 •

edited

Loading

lukasbestle left a comment

tobimori commented Feb 6, 2024

afbora commented Feb 10, 2024

tobimori commented Feb 10, 2024 •

edited

Loading

afbora commented Feb 10, 2024

tobimori commented Feb 10, 2024

distantnative left a comment

distantnative Jul 22, 2024

afbora Sep 13, 2024

distantnative Sep 14, 2024

afbora Sep 16, 2024

distantnative Sep 16, 2024

afbora Sep 27, 2024

distantnative Jul 22, 2024

afbora Sep 13, 2024

afbora commented Sep 13, 2024 •

edited

Loading

distantnative Sep 14, 2024

afbora Sep 16, 2024

bastianallgeier Sep 16, 2024

afbora Sep 16, 2024

bastianallgeier Sep 16, 2024

distantnative Sep 16, 2024

afbora Sep 27, 2024

bastianallgeier Sep 27, 2024

distantnative Sep 27, 2024

bastianallgeier Oct 1, 2024

distantnative Sep 14, 2024

distantnative Sep 14, 2024

-	public function save(array $translations = []): static
-	{
-		$this->data = $translations;
+	public function save(array|null $translations = null): static
+	{
+		if ($translations !== null) {
+			$this->update($translations);
+		}

New language:variables root #6212

Are you sure you want to change the base?

New language:variables root #6212

Conversation

afbora commented Jan 26, 2024 • edited Loading

This PR …

Fixes

Features

Breaking changes

Ready?

For review team

lukasbestle left a comment

Choose a reason for hiding this comment

tobimori commented Feb 6, 2024

afbora commented Feb 10, 2024

tobimori commented Feb 10, 2024 • edited Loading

afbora commented Feb 10, 2024

tobimori commented Feb 10, 2024

distantnative left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afbora commented Sep 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

New `language:variables` root #6212

New `language:variables` root #6212

afbora commented Jan 26, 2024 •

edited

Loading

tobimori commented Feb 10, 2024 •

edited

Loading

afbora commented Sep 13, 2024 •

edited

Loading