Laravel-Backpack / LangFileManager

A quick interface to edit Laravel language files, for Backpack.
http://backpackforlaravel.com
MIT License
92 stars 42 forks source link

Fix for Laravel 9 lang path #86

Closed promatik closed 2 years ago

promatik commented 2 years ago

Fix for #85. This time I tested with a proper Laravel 9 installation, not our Demo that's still on Laravel 8 😅

pxpm commented 2 years ago

If we do the change like this we need to tag a new major since lang_path only exists in L9.

I would do it just to avoid future conflicts like in backup. 👍

tabacitu commented 2 years ago

If we do the change like this we need to tag a new major since lang_path only exists in L9.

Ouch! Good catch @pxpm . I agree with the spirit, but that would make us maintain two versions of this:

Can't we fix this with a conditional somehow, to make v4 work for both L8 and L9? Both if you use the old lang location and if you're using the new lang location? We can then remove that conditional when we drop L8 support next year.

pxpm commented 2 years ago

Hey @tabacitu I am sure we can, but do we want to keep that rabbit hole open and running?

V4 should support B4 and L8 .. V5 should be b5 and l9.

Supporting that array of versions in crud is already a though job, having it in secondary packages seems to me just additional wood for the fireplace.

Anyway, this is more a business perspective issue, we as developers are ready for the storm, so . Your business, your rules. 👍

tabacitu commented 2 years ago

V4 should support B4 and L8 .. V5 should be b5 and l9.

Backpack v5 also supports Laravel 8 - that's my point, we should not forget that. And to make it easier for us maintainers, we should make our addons depend on the Backpack version, not the Laravel version (whenever possible).

pxpm commented 2 years ago

Indeed, that's my point too. I still think that V5 should have followed the L9 and dropped support for L8. This is just my opinion, I agree on why we did it there and it makes sense, but do we need it for all the packages especially this "secondary" ones? Can't we just say: use V4 for Laravel 8 ( no matter the crud version) and V5 for L9, "we recommend you to update as we are now only pushing new features for the V5.

We don't let anyone behing, what worked still work. L9 is an LTS, no worries pushing support for it.

Nice rumbling to start the morning 🌅🌄🌅🌄 hehe

tabacitu commented 2 years ago

Supporting just one Laravel version sure would be easier for us.

But it wouldn't be as easy for devs to upgrade. Now they can:

We'll consider supporting just one version of Laravel next time around, I promise. Until then... the hard truth is that we should support both L8 and L9 in addons when we can, that makes them easier to maintain.

pxpm commented 2 years ago

Ok.. it's easier to make it backwards compatible here since a helper for the lang_path in helpers.php would solve it like we solved the Countable in previous versions of PHP.

tabacitu commented 2 years ago

@promatik triple-check please? Thumbs up?

promatik commented 2 years ago

Tested, and it's working 👌

I was about to make a suggestion to improve a little bit the code, but it wouldn't be perfect anyway, because of the L8 support. So, in the next version we move to the proper/clean solution.

And thanks Laravel for being always moving stuff from one place to another 😅