-
-
Notifications
You must be signed in to change notification settings - Fork 157
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
Add methods getTranslationChanges() and wasTranslationChanged() #231
Conversation
…patibility" This reverts commit 8d5326d.
Hey, |
Done (again) 🙂
Until now it didn't ... |
… array temporarily remove Translatable::wasTranslationChanged() adjust unit tests
Dear Developer!
Schema::create('posts', function(Blueprint $table) {
$table->increments('id');
$table->string('author');
$table ->boolean('is_active');
$table->timestamps();
}); //Post Model namespace App\Models;
use Illuminate\Support\Facades\Schema;
use Illuminate\Database\Eloquent\Model;
use Astrotomic\Translatable\Translatable;
use Illuminate\Database\Eloquent\Factories\HasFactory;
// use Astrotomic\Translatable\Contracts\Translatable as TranslatableContract;
class Post extends Model implements TranslatableContract
{
use HasFactory, Translatable;
public function __construct(array $attributes = [])
{
$this->fillable = Schema::getColumnListing($this->getTable());
parent::__construct($attributes);
}
// protected $guarded = ['id'];
public $translatedAttributes = ['title', 'full_text'];
} //PostTranslation Model namespace App\Models;
use Illuminate\Database\Eloquent\Factories\HasFactory;
use Illuminate\Database\Eloquent\Model;
class PostTranslation extends Model
{
use HasFactory;
public $timestamps = false;
protected $fillable = ['author','title', 'full_text'];
} //Controller public function store(Request $request)
{
$request->request->add(['author'=> auth()->user()->name]);
$request->request->add(['is_active'=> 1]);
Post::create($request->validated());
return redirect()->route('dashboard');
} when submit the form it save only ('title','content') to table post_translations only. |
TBH, I don't really understand what you want to achieve with your code. |
👍 Please open an issue for your problem. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the long wait time.
Thanks for your work here - looking good so far, only a bit too powerful/complex. 😉
Have some code-style and two logic requests.
if (empty($translationChanges)) { | ||
return false; | ||
} | ||
if (empty($attributes)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (empty($attributes)) { | |
if ($attributes === null) { |
if (config('translatable.rule_factory.format') === RuleFactory::FORMAT_KEY) { | ||
$attributes = array_map(function ($attribute) { | ||
return implode('.', array_reverse(explode(':', $attribute))); | ||
}, $attributes); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As the user can provide attribute and locale I don't see a reason why we should do this "complex" string handling here. wasTranslationChanged('title', 'de')
should check if the german title was changed. So it's easier to use/read and more clear than wasTranslationChanged('de.name')
or wasTranslationChanged('name:de')
.
By removing this string exploding the following logic also gets a lot easier.
|
||
public function wasTranslationChanged($attributes = null, $locale = null): bool | ||
{ | ||
$translationChanges = $this->getTranslationChanges(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there are a few cases missing and therefore the check isn't complete:
if empty($translationChanges)
return false; // absolutely nothing changed
if ($attributes === null && $locale === null)
return true; // something changed and the user doesn't filter anything
if (is_string($attributes) && $locale !== null)
return isset($translationChanges[$locale][$attributes]); // specific translation changed
if ($attributes === null && $locale !== null)
return empty($translationChanges[$locale]); // check if anything for locale changed
if (is_string($attributes) && $locale === null)
return empty(array_column($translationChanges, $attributes)); // any translation of single attribute changed
// loop over matrix and do checks
add return types in unit tests add phpdoc comments Co-authored-by: Tom Witkowski <[email protected]>
Are you planning to merge this any time soon? |
According to @Gummibeer 's suggestion added method
getTranslationChanges()
(no overrides or breaking changes).For completeness in analogy to Laravel model's
wasChanged()
also added methodwasTranslationChanged()
.Both methods are covered by additional unit tests.
Could not resist to fix a typo in an existing unit test.
Dependencies allow Illuminate/Laravel 8 where Factories have been removed. Therefore needed to add
laravel/legacy-factories
.The first commit 'Prettified Code!' was created by a Github Action which is defined within the original repository. I just changed the default branch's name to 'main' which triggered the action. As the action seems to be defined by the maintainer I assume its results are intended (although they blow up this PR).
I'm not sure whether and where I should update the docs. Would appreciate any suggestions.