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

Add methods getTranslationChanges() and wasTranslationChanged() #231

Closed
wants to merge 14 commits into from

Conversation

Tschoatscho
Copy link

According to @Gummibeer 's suggestion added method getTranslationChanges() (no overrides or breaking changes).
For completeness in analogy to Laravel model's wasChanged() also added method wasTranslationChanged().
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.

@Gummibeer
Copy link
Member

Hey,
there was an issue in the GitHub Action doing the first commit on the main branch.
Could you click "fetch upstream" button on your fork? 🙂
That should fix it - I will ignore the markdown files for the moment.

@Tschoatscho
Copy link
Author

Tschoatscho commented Jun 11, 2021

Could you click "fetch upstream" button on your fork? 🙂

Done (again) 🙂

That should fix it - I will ignore the markdown files for the moment.

Until now it didn't ...
If we can't get the PR clean this way (after giving Github time to refresh) I could create a new PR only with the 2 (3 with typo fix) relevant commits (would also clear my error with legacy-factories).

src/Translatable/Translatable.php Outdated Show resolved Hide resolved
src/Translatable/Translatable.php Outdated Show resolved Hide resolved
src/Translatable/Translatable.php Outdated Show resolved Hide resolved
tests/TranslatableTest.php Show resolved Hide resolved
@phagna
Copy link

phagna commented Sep 23, 2021

Dear Developer!
I have some problem with this package like below:

  • I want to add more fields to the Main post table like this
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.
so what problem with it?

@Tschoatscho
Copy link
Author

TBH, I don't really understand what you want to achieve with your code.
is_active is a boolean you should not try to translate it, same with author name.
Maybe you better make your changes in the Post model and not in the PostTranslation model.
BTW, this discussion refers to a pull request on laravel-translatable and not to the laravel-translatable itself.

@Gummibeer
Copy link
Member

BTW, this discussion refers to a pull request on laravel-translatable and not to the laravel-translatable itself.

👍

Please open an issue for your problem.

Copy link
Member

@Gummibeer Gummibeer left a 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.

src/Translatable/Translatable.php Outdated Show resolved Hide resolved
if (empty($translationChanges)) {
return false;
}
if (empty($attributes)) {
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
if (empty($attributes)) {
if ($attributes === null) {

src/Translatable/Translatable.php Outdated Show resolved Hide resolved
Comment on lines +489 to +493
if (config('translatable.rule_factory.format') === RuleFactory::FORMAT_KEY) {
$attributes = array_map(function ($attribute) {
return implode('.', array_reverse(explode(':', $attribute)));
}, $attributes);
}
Copy link
Member

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();
Copy link
Member

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

tests/TranslatableTest.php Show resolved Hide resolved
tests/TranslatableTest.php Show resolved Hide resolved
tests/TranslatableTest.php Show resolved Hide resolved
add return types in unit tests
add phpdoc comments

Co-authored-by: Tom Witkowski <dev.gummibeer@gmail.com>
@dmtar
Copy link

dmtar commented Jul 20, 2022

Are you planning to merge this any time soon?

@github-actions github-actions bot added the stale label Oct 7, 2022
@github-actions github-actions bot closed this Oct 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Include changed translations with translated models' getChanges method
4 participants