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

Keep the number of translations even with null values #427

Merged
merged 1 commit into from
Dec 1, 2023
Merged

Keep the number of translations even with null values #427

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
26 changes: 14 additions & 12 deletions src/HasTranslations.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ public function getTranslation(string $key, string $locale, bool $useFallbackLoc

$translations = $this->getTranslations($key);

$translation = $translations[$normalizedLocale] ?? '';
$translation = $translations[$normalizedLocale] ?? null;
Copy link
Contributor

@mabdullahsari mabdullahsari Dec 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This is a big issue for str functions that no longer accept null values and consequently blow up.

Copy link
Member

Choose a reason for hiding this comment

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

Feel free to change it back to an empty string. Do make sure all tests pass.

Copy link
Contributor Author

@sdebacker sdebacker Dec 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Every nullable varchar string from the database have to be checked before using the str functions. Also, with the previous version, null values were saved in the json field for the last language, so this issue was already there, am I wrong ?

Copy link
Contributor

Choose a reason for hiding this comment

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

The previous behavior was converting those default null values to empty strings so this check was handled by the package, which was then changed to be null moving this burden onto the shoulders of the package users. 👍


$translatableConfig = app(Translatable::class);

Expand Down Expand Up @@ -101,20 +101,20 @@ public function getTranslationWithoutFallback(string $key, string $locale): mixe
return $this->getTranslation($key, $locale, false);
}

public function getTranslations(string $key = null, array $allowedLocales = null): array
public function getTranslations(string $key = null, array $allowedLocales = null, bool $keepNullValues = true): array
{
if ($key !== null) {
$this->guardAgainstNonTranslatableAttribute($key);

return array_filter(
json_decode($this->getAttributes()[$key] ?? '' ?: '{}', true) ?: [],
fn ($value, $locale) => $this->filterTranslations($value, $locale, $allowedLocales),
fn ($value, $locale) => $this->filterTranslations($value, $locale, $allowedLocales, $keepNullValues),
ARRAY_FILTER_USE_BOTH,
);
}

return array_reduce($this->getTranslatableAttributes(), function ($result, $item) use ($allowedLocales) {
$result[$item] = $this->getTranslations($item, $allowedLocales);
return array_reduce($this->getTranslatableAttributes(), function ($result, $item) use ($allowedLocales, $keepNullValues) {
$result[$item] = $this->getTranslations($item, $allowedLocales, $keepNullValues);

return $result;
});
Expand Down Expand Up @@ -204,7 +204,7 @@ public function forgetAllTranslations(string $locale): self

public function getTranslatedLocales(string $key): array
{
return array_keys($this->getTranslations($key));
return array_keys($this->getTranslations($key, null, false));
}

public function isTranslatableAttribute(string $key): bool
Expand Down Expand Up @@ -268,14 +268,16 @@ protected function normalizeLocale(string $key, string $locale, bool $useFallbac
return $locale;
}

protected function filterTranslations(mixed $value = null, string $locale = null, array $allowedLocales = null): bool
protected function filterTranslations(mixed $value = null, string $locale = null, array $allowedLocales = null, bool $keepNullValues = true): bool
{
if ($value === null) {
return false;
}
if (! $keepNullValues) {
if ($value === null) {
return false;
}

if ($value === '') {
return false;
if ($value === '') {
return false;
}
}

if ($allowedLocales === null) {
Expand Down
27 changes: 22 additions & 5 deletions tests/TranslatableTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@
$this->testModel->setTranslation('name', 'en', 'testValue_en');
$this->testModel->save();

expect($this->testModel->getTranslation('name', 'fr', false))->toBe('');
expect($this->testModel->getTranslation('name', 'fr', false))->toBe(null);
});

it('will return fallback locale translation when getting an unknown locale and fallback is true', function () {
Expand Down Expand Up @@ -126,7 +126,7 @@
$this->testModel->setTranslation('name', 'en', 'testValue_en');
$this->testModel->save();

expect($this->testModel->getTranslationWithoutFallback('name', 'fr'))->toBe('');
expect($this->testModel->getTranslationWithoutFallback('name', 'fr'))->toBe(null);
});

it('will return an empty string when getting an unknown locale and fallback is empty', function () {
Expand All @@ -139,7 +139,7 @@
$this->testModel->setTranslation('name', 'en', 'testValue_en');
$this->testModel->save();

expect($this->testModel->getTranslation('name', 'fr'))->toBe('');
expect($this->testModel->getTranslation('name', 'fr'))->toBe(null);
});

it('can save a translated attribute', function () {
Expand All @@ -149,6 +149,13 @@
expect($this->testModel->name)->toBe('testValue_en');
});

it('can save null value in a translated attribute', function () {
$this->testModel->setTranslation('name', 'en', null);
$this->testModel->save();

expect($this->testModel->name)->toBe(null);
});

it('can set translated values when creating a model', function () {
$model = TestModel::create([
'name' => ['en' => 'testValue_en'],
Expand Down Expand Up @@ -448,13 +455,23 @@ public function setNameAttribute($value)
it('can check if an attribute has translation', function () {
$this->testModel->setTranslation('name', 'en', 'testValue_en');
$this->testModel->setTranslation('name', 'nl', null);
$this->testModel->setTranslation('name', 'de', null);
$this->testModel->save();

expect($this->testModel->hasTranslation('name', 'en'))->toBeTrue();

expect($this->testModel->hasTranslation('name', 'pt'))->toBeFalse();
});

it('will return the same number of translations with the same values as saved', function () {
$this->testModel->setTranslation('name', 'en', 'testValue_en');
$this->testModel->setTranslation('name', 'nl', null);
$this->testModel->setTranslation('name', 'de', '');
$this->testModel->save();

expect($this->testModel->getTranslations('name'))->toEqual(['en' => 'testValue_en', 'nl' => null, 'de' => '']);
});

it('can correctly set a field when a mutator is defined', function () {
$testModel = (new class () extends TestModel {
public function setNameAttribute($value)
Expand Down Expand Up @@ -699,7 +716,7 @@ public function setAttributesExternally(array $attributes)
$this->testModel->save();

$this->testModel->setLocale('it');
expect($this->testModel->getTranslation('name', 'it', false))->toBe('');
expect($this->testModel->getTranslation('name', 'it', false))->toBe(null);
});

it('will return default fallback locale translation when getting an unknown locale with fallback any', function () {
Expand Down Expand Up @@ -760,7 +777,7 @@ public function setAttributesExternally(array $attributes)

$model->setLocale('fr');

expect($model->name)->toBe('');
expect($model->name)->toBe(null);
});

it('can set fallback locale on model', function () {
Expand Down
Loading