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.

Sign up for GitHub

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

Jump to bottom

New language:variables root #6212

Draft
wants to merge 6 commits into
base: v5/develop
Choose a base branch
from
Draft

New language:variables root #6212

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
5ce59f3
New `translations` root
afbora Jan 26, 2024
437745f
Apply code reviews
afbora Jan 27, 2024
b1b372f
Improvements
afbora Sep 13, 2024
b171903
Rename root with `language:variables`
afbora Sep 16, 2024
1628be7
Swtich from `::translations()` to `::variables()`
afbora Sep 16, 2024
b56dad6
Apply suggestions from code review
afbora Nov 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions config/areas/languages/views.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@
$language = Find::language($code);
$link = '/languages/' . $language->code();
$strings = [];
$foundation = $kirby->defaultLanguage()->translations();
$translations = $language->translations();
$foundation = $kirby->defaultLanguage()->translations()->toArray();
$translations = $language->translations()->toArray();

// TODO: update following line and adapt for update and
// delete options when `languageVariables.*` permissions available
Expand Down
12 changes: 9 additions & 3 deletions src/Cms/AppTranslations.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,10 @@ protected function i18n(): void
$this->multilang() === true &&
$language = $this->languages()->find($locale)
) {
$data = [...$data, ...$language->translations()];
$data = [
...$data,
...$language->translations()->toArray()
];
}


Expand Down Expand Up @@ -135,7 +138,10 @@ public function translation(string|null $locale = null): Translation

// inject current language translations
if ($language = $this->language($locale)) {
$inject = [...$inject, ...$language->translations()];
$inject = [
...$inject,
...$language->translations()->toArray()
];
}

// load from disk instead
Expand All @@ -161,7 +167,7 @@ public function translations(): Translations
if ($languages = $this->languages()) {
foreach ($languages as $language) {
$languageCode = $language->code();
$languageTranslations = $language->translations();
$languageTranslations = $language->translations()->toArray();

// merges language translations with extensions translations
if (empty($languageTranslations) === false) {
Expand Down
53 changes: 27 additions & 26 deletions src/Cms/Core.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -331,33 +331,34 @@ public function load(): Loader
public function roots(): array
{
return $this->cache['roots'] ??= [
'kirby' => fn (array $roots) => dirname(__DIR__, 2),
'i18n' => fn (array $roots) => $roots['kirby'] . '/i18n',
'kirby' => fn (array $roots) => dirname(__DIR__, 2),
'i18n' => fn (array $roots) => $roots['kirby'] . '/i18n',
'i18n:translations' => fn (array $roots) => $roots['i18n'] . '/translations',
'i18n:rules' => fn (array $roots) => $roots['i18n'] . '/rules',

'index' => fn (array $roots) => static::$indexRoot ?? dirname(__DIR__, 3),
'assets' => fn (array $roots) => $roots['index'] . '/assets',
'content' => fn (array $roots) => $roots['index'] . '/content',
'media' => fn (array $roots) => $roots['index'] . '/media',
'panel' => fn (array $roots) => $roots['kirby'] . '/panel',
'site' => fn (array $roots) => $roots['index'] . '/site',
'accounts' => fn (array $roots) => $roots['site'] . '/accounts',
'blueprints' => fn (array $roots) => $roots['site'] . '/blueprints',
'cache' => fn (array $roots) => $roots['site'] . '/cache',
'collections' => fn (array $roots) => $roots['site'] . '/collections',
'commands' => fn (array $roots) => $roots['site'] . '/commands',
'config' => fn (array $roots) => $roots['site'] . '/config',
'controllers' => fn (array $roots) => $roots['site'] . '/controllers',
'languages' => fn (array $roots) => $roots['site'] . '/languages',
'license' => fn (array $roots) => $roots['config'] . '/.license',
'logs' => fn (array $roots) => $roots['site'] . '/logs',
'models' => fn (array $roots) => $roots['site'] . '/models',
'plugins' => fn (array $roots) => $roots['site'] . '/plugins',
'sessions' => fn (array $roots) => $roots['site'] . '/sessions',
'snippets' => fn (array $roots) => $roots['site'] . '/snippets',
'templates' => fn (array $roots) => $roots['site'] . '/templates',
'roles' => fn (array $roots) => $roots['blueprints'] . '/users',
'i18n:rules' => fn (array $roots) => $roots['i18n'] . '/rules',

'index' => fn (array $roots) => static::$indexRoot ?? dirname(__DIR__, 3),
'assets' => fn (array $roots) => $roots['index'] . '/assets',
'content' => fn (array $roots) => $roots['index'] . '/content',
'media' => fn (array $roots) => $roots['index'] . '/media',
'panel' => fn (array $roots) => $roots['kirby'] . '/panel',
'site' => fn (array $roots) => $roots['index'] . '/site',
'accounts' => fn (array $roots) => $roots['site'] . '/accounts',
'blueprints' => fn (array $roots) => $roots['site'] . '/blueprints',
'cache' => fn (array $roots) => $roots['site'] . '/cache',
'collections' => fn (array $roots) => $roots['site'] . '/collections',
'commands' => fn (array $roots) => $roots['site'] . '/commands',
'config' => fn (array $roots) => $roots['site'] . '/config',
'controllers' => fn (array $roots) => $roots['site'] . '/controllers',
'languages' => fn (array $roots) => $roots['site'] . '/languages',
'license' => fn (array $roots) => $roots['config'] . '/.license',
'logs' => fn (array $roots) => $roots['site'] . '/logs',
'models' => fn (array $roots) => $roots['site'] . '/models',
'plugins' => fn (array $roots) => $roots['site'] . '/plugins',
'roles' => fn (array $roots) => $roots['blueprints'] . '/users',
'sessions' => fn (array $roots) => $roots['site'] . '/sessions',
'snippets' => fn (array $roots) => $roots['site'] . '/snippets',
'templates' => fn (array $roots) => $roots['site'] . '/templates',
'translations' => null,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bastianallgeier What do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe even i18n:variables then?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure for this PR, @bastianallgeier ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

];
}

Expand Down
25 changes: 18 additions & 7 deletions src/Cms/Language.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ class Language implements Stringable
protected bool $single;
protected array $slugs;
protected array $smartypants;
protected array $translations;
protected LanguageTranslations $translations;
protected string|null $url;

/**
Expand All @@ -68,14 +68,15 @@ public function __construct(array $props)
$this->single = $props['single'] ?? false;
$this->slugs = $props['slugs'] ?? [];
$this->smartypants = $props['smartypants'] ?? [];
$this->translations = $props['translations'] ?? [];
$this->url = $props['url'] ?? null;

if ($locale = $props['locale'] ?? null) {
$this->locale = Locale::normalize($locale);
} else {
$this->locale = [LC_ALL => $this->code];
}

$this->translations = new LanguageTranslations($this, $props['translations'] ?? []);
}

/**
Expand Down Expand Up @@ -130,7 +131,7 @@ public function clone(array $props = []): static
'name' => $this->name,
'slugs' => $this->slugs,
'smartypants' => $this->smartypants,
'translations' => $this->translations,
'translations' => $this->translations->toArray(),
'url' => $this->url,
], $props));
}
Expand Down Expand Up @@ -235,6 +236,9 @@ public function delete(): bool
throw new Exception('The language could not be deleted');
}

// delete custom translations file if defined
$language->translations()->delete();

// if needed, convert content storage to single lang
foreach ($kirby->models() as $model) {
if ($language->isLast() === true) {
Expand Down Expand Up @@ -482,12 +486,19 @@ public function save(): static
'direction' => $this->direction(),
'locale' => Locale::export($this->locale()),
'name' => $this->name(),
'translations' => $this->translations(),
'translations' => $this->translations()->toArray(),
'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($data['translations'] ?? []);
$data['translations'] = [];
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


Data::write($this->root(), $data);

return $this;
Expand Down Expand Up @@ -548,9 +559,9 @@ public function toArray(): array
}

/**
* Returns the translation strings for this language
* Returns the language translations object for this language
*/
public function translations(): array
afbora marked this conversation as resolved.
Show resolved Hide resolved
public function translations(): LanguageTranslations
{
return $this->translations;
}
Expand Down Expand Up @@ -594,7 +605,7 @@ public function update(array|null $props = null): static
$language = $language->clone($props);

if (isset($props['translations']) === true) {
$language->translations = $props['translations'];
$language->translations = $language->translations->update($props['translations'] ?? null);
}

// validate the language rules after before hook was applied
Expand Down
133 changes: 133 additions & 0 deletions src/Cms/LanguageTranslations.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,133 @@
<?php

namespace Kirby\Cms;

use Kirby\Data\Data;
use Kirby\Exception\Exception;
use Kirby\Filesystem\F;
use Throwable;

/**
* Manages the translations string for a language,
* either from the language file or `translations` root
* @since 5.0.0
*
* @package Kirby Cms
* @author Ahmet Bora <[email protected]>
* @link https://getkirby.com
* @copyright Bastian Allgeier
* @license https://getkirby.com/license
*/
class LanguageTranslations
{
public function __construct(
protected Language $language,
protected array $data = []
) {
$this->data = [...$this->load(), ...$this->data];
}

/**
* Deletes the current language translations file
* if custom root defined
*/
public function delete(): void
{
if ($file = $this->root()) {
if (F::remove($file) !== true) {
throw new Exception('The language translations could not be deleted');
}
}
}

/**
* Returns a single translation string by key
*/
public function get(string $key, string|null $default = null): string|null
{
return $this->data[$key] ?? $default;
}

/**
* Loads the language translations based on custom roots
*/
public function load(): array
{
if ($file = static::root()) {
afbora marked this conversation as resolved.
Show resolved Hide resolved
try {
return Data::read($file);
} catch (Throwable) {
// skip when an exception thrown
}
}

return [];
}

/**
* Saves the language translations in the custom root
* @return $this
* @internal
*/
public function save(array $translations = []): static
distantnative marked this conversation as resolved.
Show resolved Hide resolved
{
$this->data = $translations;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
}


if ($root = $this->root()) {
Data::write($root, $this->data);
}

return $this;
}

/**
* Returns custom translations root path if defined
*/
public function root(): string|null
{
if ($root = App::instance()->root('translations')) {
return $root . '/' . $this->language->code() . '.php';
}

return null;
}

/**
* Removes a translation key
*
* @return $this
*/
public function remove(string $key): static
{
unset($this->data[$key]);
return $this;
}

/**
* Sets the translation key
*
* @return $this
*/
public function set(string $key, string|null $value = null): static
{
$this->data[$key] = $value;
return $this;
}

/**
* Returns translations
*/
public function toArray(): array
{
return $this->data;
}

/**
* Updates the translations data
*/
public function update(array $data = []): static
{
$this->data = $data;
return $this;
}
}
20 changes: 9 additions & 11 deletions src/Cms/LanguageVariable.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ public static function create(

$kirby = App::instance();
$language = $kirby->defaultLanguage();
$translations = $language->translations();
$translations = $language->translations()->toArray();

if ($kirby->translation()->get($key) !== null) {
if (isset($translations[$key]) === true) {
Expand All @@ -72,11 +72,9 @@ public function delete(): bool
{
// go through all languages and remove the variable
foreach ($this->kirby->languages() as $language) {
$variables = $language->translations();

unset($variables[$this->key]);

$language->update(['translations' => $variables]);
$translations = $language->translations();
$translations->remove($this->key);
$language->update(['translations' => $translations->toArray()]);
}

return true;
Expand All @@ -88,7 +86,7 @@ public function delete(): bool
public function exists(): bool
{
$language = $this->kirby->defaultLanguage();
return isset($language->translations()[$this->key]) === true;
return $language->translations()->get($this->key) !== null;
}

/**
Expand All @@ -104,10 +102,10 @@ public function key(): string
*/
public function update(string|null $value = null): static
{
$translations = $this->language->translations();
$translations[$this->key] = $value ?? '';
$translations = $this->language->translations();
$translations->set($this->key, $value);

$language = $this->language->update(['translations' => $translations]);
$language = $this->language->update(['translations' => $translations->toArray()]);

return $language->variable($this->key);
}
Expand All @@ -117,6 +115,6 @@ public function update(string|null $value = null): static
*/
public function value(): string|null
{
return $this->language->translations()[$this->key] ?? null;
return $this->language->translations()->get($this->key);
}
}
Loading