Skip to content
This repository has been archived by the owner on Mar 11, 2022. It is now read-only.

WiP — Make field compatible with both dimsav/laravel-translatable and spatie/laravel-translatable #17

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

vv221
Copy link

@vv221 vv221 commented Sep 18, 2018

Hello World!

For some of our needs, we tweaked nova-translatable to make it work with dimsav/laravel-translatable instead of spatie/laravel-translatable. Then we took some time to improve our fork so it works with both packages.

The changes are kept as minimal as possible to allow nova-translatable to be used in the exact same way no matter the underlying laravel-translatable package available.

It isn’t ready for merging yet, as we still need to find a way to improve the composer dependencies check so nova-translatable can be installed as long as one of the supported laravel-translatable packages is available.

All suggestions to work around the lack of a logical OR in composer dependencies handling would be most welcome ;)

@gerardnll
Copy link

Some would recommend this as adding those packages in the 'suggest' field may not be really correct.

@vv221
Copy link
Author

vv221 commented Oct 23, 2018

@gerardnll
Just to be sure I understand your suggestion:
You’re saying we should have three packages: nova-translatable-common, nova-translatable-dimsav and nova-translatable-spatie, with the first one being a dependency for the others?

@gerardnll
Copy link

gerardnll commented Oct 23, 2018 via email

@vv221
Copy link
Author

vv221 commented Oct 23, 2018

@gerardnll
In think 'suggest' won’t work here, as one of these laravel-translatable packages need to be installed or this package won’t work at all.
Splitting the package is a solution I see, another one would be to have both the laravel-translatable packages providing a same virtual package. But the issue in this case is that they can not be used in the same way (even if they provide the same features), so the virtual package might induce a compatibility that doesn’t actually exist and easily trick users.

@vv221
Copy link
Author

vv221 commented May 3, 2019

We updated our fork, so it applies cleanly on current master.
A couple code clean-up and bugfixes happened too ;)

@vv221
Copy link
Author

vv221 commented May 13, 2019

The error Undefined variable: resource should now be fixed.

@vv221
Copy link
Author

vv221 commented Dec 18, 2019

As I’m going to leave my employment at Yes We Dev soon, I will no longer keep working on this compatibility fork. I don’t know if my employer is going to have someone else maintaining this.

Current version of the fork, using astrotomic/laravel-translatable instead of discontinued dimsav/laravel-translatable, is available here: https://framagit.org/yeswedev/ywd_nova-translatable

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants