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

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

Open
daxsis opened this issue Apr 28, 2020 · 10 comments
Assignees
Labels

Comments

@daxsis
Copy link

daxsis commented Apr 28, 2020

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
Copy link
Author

daxsis commented Apr 28, 2020

I tested with 4.1 of backpack. The same issue

@daxsis daxsis changed the title Declaration conflict with spatie/laravel-tags version 2.6.1 [4.1][BUG] Declaration conflict with spatie/laravel-tags version 2.6.1 Apr 28, 2020
@tabacitu tabacitu added the Bug label Apr 30, 2020
@tabacitu
Copy link
Member

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.

@tabacitu tabacitu removed the Bug label Jul 25, 2020
@tabacitu tabacitu added Possible Bug A bug that was reported but not confirmed yet. Priority: COULD Priority: SHOULD labels Oct 16, 2020
@tabacitu tabacitu changed the title [4.1][BUG] Declaration conflict with spatie/laravel-tags version 2.6.1 [BUG] Declaration conflict with spatie/laravel-tags version 2.6.1 Oct 16, 2020
@stephenr85
Copy link

stephenr85 commented Jun 30, 2023

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
Copy link
Member

tabacitu commented Jul 2, 2023

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
Copy link

stephenr85 commented Jul 3, 2023 via email

@pxpm
Copy link
Contributor

pxpm commented Jul 3, 2023

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
Copy link
Member

tabacitu commented Jul 5, 2023

@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
Copy link

stephenr85 commented Jul 5, 2023 via email

@alancwoo
Copy link

alancwoo commented Mar 4, 2024

@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
Copy link
Contributor

pxpm commented Jul 14, 2024

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

@pxpm pxpm changed the title [BUG] Declaration conflict with spatie/laravel-tags version 2.6.1 [v7] Remove spatie HasTranslations trait overwrite to allow compatibility with packages like spatie/laravel-tags Jul 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Development

No branches or pull requests

5 participants