search

Laravel-Backpack / CRUD

Build custom admin panels. Fast!
https://backpackforlaravel.com
MIT License
3.19k stars 898 forks source link

[v7] Remove spatie HasTranslations trait overwrite to allow compatibility with packages like spatie/laravel-tags #2725

Open daxsis opened 4 years ago

daxsis commented 4 years ago

Bug report

Declaration of Backpack\CRUD\app\Models\Traits\SpatieTranslatable\HasTranslations::getLocale() must be compatible with Spatie\Tags\Tag::getLocale(): string

What I did

Install package from spatie/laravel-tags and use custom Tag model, which changes in the core functionality of the spatie package nothing.

What I expected to happen

No conflicts of the resolution between spatie/laravel-tags and backpack version of the HasTranlsation trait

What happened

??

What I've already tried to fix it

add return type ): string but then there is another issue which is 

Declaration of Backpack\CRUD\app\Models\Traits\SpatieTranslatable\HasTranslations::setTranslation(string $key, string $locale, $value): Backpack\CRUD\app\Models\Traits\SpatieTranslatable\HasTranslations must be compatible with Spatie\Tags\Tag::setTranslation(string $key, string $locale, $value): Spatie\Tags\Tag

Backpack, Laravel, PHP, DB version

When I run php artisan backpack:version the output is:

PHP VERSION:

PHP 7.4.3 (cli) (built: Feb 18 2020 17:29:46) ( ZTS Visual C++ 2017 x64 ) Copyright (c) The PHP Group Zend Engine v3.4.0, Copyright (c) Zend Technologies with Xdebug v2.9.2, Copyright (c) 2002-2020, by Derick Rethans

LARAVEL VERSION:

v7.9.2@757b155658ae6da429065ba8f22242fe599824f7

BACKPACK VERSION:

4.0.60@3c6116bce3c90ef9850d3d07de5c7b8f41a7b6cd

daxsis commented 4 years ago

I tested with 4.1 of backpack. The same issue

tabacitu commented 4 years ago

Thanks for raising the issue @daxsis . Hmm... Then you should probably roll your own HasTranslation trait, instead of using the one provided by Backpack, that has the changes this needs. We won't be able to fix this for a while - I want Backpack to be compatible with Spatie/Laravel-Tags, but right now we're working on launching 4.1 and I'm afraid this issue won't be a priority for us for some time - so I recommend you go for a solution yourself.

Providing a solution in Backpack won't be as easy as creating a solution inside your project - I bet spatie requires PHP 7.4 whereas Backpack support PHP down to 7.2.5 - so our solutions have to take that into consideration.

Sorry to be the bearer of bad news! If you end up with a solution you like, please consider sharing it here, or as a PR - it would be much appreciated and it would go a long way towards having a fix inside Backpack.

stephenr85 commented 1 year ago

This is still an issue. The last compatible version of Translatable package is https://github.com/spatie/laravel-translatable/releases/tag/4.6.0 (from 2020).

After creating my own trait with compatible methods and following the docs, including having the appropriate fields in $translatable, the fields still showed as arrays in the CRUD form. With the field types set to "text" or "slug", the resulting values were empty. This might be due to even more of the HasTranslation trait needing to be updated, but this indicates to me that it's not a safe feature to use at the moment, unfortunately.

tabacitu commented 1 year ago

Sorry @stephenr85 we haven't been able to prioritize this compatibliity thing. We've been hard at work launching Backpack v6 - due TO.DAY! 😱🎉 I've reassigned this so we can take a look at this after the v6 dust settles.

Thank you for your patience. Cheers!

stephenr85 commented 1 year ago

Thanks for the prompt response! Very much looking forward to v6!

I think this will be a quick fix for v6 considering the laravel V10 requirement.

I started a test upgrade on my project, but saw that the pro and devtools packages are still in dev. Is there an ETA on official releases of those?

On Sun, Jul 2, 2023, 07:01 Cristian Tăbăcitu @.***> wrote:

Sorry @stephenr85 https://github.com/stephenr85 we haven't been able to prioritize this compatibliity thing. We've been hard at work launching Backpack v6 - due TO.DAY! 😱🎉 I've reassigned this so we can take a look at this after the v6 dust settles.

Thank you for your patience. Cheers!

— Reply to this email directly, view it on GitHub https://github.com/Laravel-Backpack/CRUD/issues/2725#issuecomment-1616620441, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAHOKM56DXMAKSXIX3BZ6J3XOFPILANCNFSM4MTGOWCA . You are receiving this because you were mentioned.Message ID: @.***>

pxpm commented 1 year ago

Hey @stephenr85 thanks for the questions.

All packages have been tagged to support v6. (if we didn't messed up somewhere 🙃 ), but I think we didn't 🙏

Either PRO and Devtools were tagged 2.X that support CRUD v6.

Let me know if a composer update didn't pick those for you.

PS: if you have tested backpack v6, you may need to change your composer.json back to tagged versions like: "backpack/pro: ^2.0"

Cheers

tabacitu commented 1 year ago

@stephenr85 you can find a list of all v6-compatible versions of our packages in the v6 upgrade guide - https://backpackforlaravel.com/docs/6.x/upgrade-guide#step-2

stephenr85 commented 1 year ago

Perfect, thank you! I've successfully updated my app...

On Wed, Jul 5, 2023 at 6:57 AM Cristian Tăbăcitu @.***> wrote:

@stephenr85 https://github.com/stephenr85 you can find a list of all v6-compatible versions of our packages in the v6 upgrade guide - https://backpackforlaravel.com/docs/6.x/upgrade-guide#step-2

— Reply to this email directly, view it on GitHub https://github.com/Laravel-Backpack/CRUD/issues/2725#issuecomment-1621613207, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAHOKM3QNC3U4LTEJZA5UELXOVJEFANCNFSM4MTGOWCA . You are receiving this because you were mentioned.Message ID: @.***>

alancwoo commented 8 months ago

@tabacitu I was wondering if there is any status on this issue, we are using and having issues with spatie/laravel-tags and unable to work with the tags in the CRUD controller. The 6.x docs specifically mention using Backpacks HasTranslations trait but this seems to have a lot of incompatibilities as per the original report from the OP 3 years ago.

I was wondering if this documentation is just incorrect or if I am missing something? I've tried adjusting the backpack trait but ultimately cannot get it to work and was curious if any guidance could be provided as I'm not entirely sure why backpack requires a custom trait to begin with.

I've just adjusted all the methods to match where they can but eventually run into an issue in setLocale which fails with Using $this when not in object context

pxpm commented 4 months ago

Hey guys.

I am doing some issue cleanup, and I've investigated this issue a bit more.

I think the only solution here is to remove the Backpack overwrite of spatie laravel translatable trait. This trait have been following us along the years and it was indeed needed at the time it was created, but not anymore I guess.

I think most of the stuff we do there can now be done using the Translatable spatie singleton to configure the translation behavior.

There is only the "fake" translatables that would probably require us to do some more tinkering, but I think it's doable.

This can only be done in next version, as it is a breaking change. Trying to make this non-bc would force us to have two implementations for the same behavior and a bunch of conditionals.

Tagging for next version, and renaming the issue as I am 100% confident that if we remove the trait overwrite spatie/laravel-tags will work out of the box.

Cheers