Adds support for implicit route model binding with translated slugs

[marvin-wtt]

Adds implicit route model binding support for translated slugs using spatie/laravel-translatable if the route key matches the slug field

[freekmurze]

Seems interesting, but currently tests and updated readme seem to be missing. Could you take care of that?

[marvin-wtt]

@freekmurze Thanks for the reply. I updated the readme and added a brief explanation. Unfortunately I was unable to add test cases because sqlite doesn't support json without the JSON1 extention. I added a try-catch block around the added operation to avoid conflicts with databases that don't support json.
Do you want me to squash the commits?

[freekmurze]

In that case, we'll need to switch to using MySQL for the tests. Currently I don't have the time to take care of this, but I accept a separate PR that does this.

[marvin-wtt]

Turns out I didn't read the laravel docs about JSON Where Clauses properly. Fortunately SQLite does support json but not Laravels whereJsonContains method. I updated it to use the the JSON_EXTRACT() function instead and it works for testing.

[freekmurze]

Let me know when this is ready for review.

[marvin-wtt]

I added some questions to your change request comments where I was unsure what to do. Except that, it's ready for review.

[freekmurze]

It's going in the right direction 👍

[marvin-wtt]

I just realized that my comments mentioned above where still pending 🙈. I'll take care of the change request this evening

[marvin-wtt]

@freekmurze As you mentioned in the review comment, child model resolution was not working correctly as it is only handled by the parent model. I created a PR which fixes this problem: laravel/framework#39929 . This however means that implicit route model binding only works from the next release (probably v8.76.0+).
I could also keep the current changes to at least support implicit route model binding for the parent model for lower version - this would be unnecessary for all future versions though.
How would you prefer to proceed here?

[freekmurze]

As you mentioned in the review comment, child model resolution was not working correctly as it is only handled by the parent model. I created a PR which fixes this problem: laravel/framework#39929 . This however means that implicit route model binding only works from the next release (probably v8.76.0+).

Instead of checking for the specific Laravel version number (and adding a note), feel free to update the required Laravel version in composer.json 👍

Seems like the tests are failing, could you take a look at that?

[marvin-wtt]

Instead of checking for the specific Laravel version number (and adding a note), feel free to update the required Laravel version in composer.json 👍

I can do that for laravel framework v8 but not for v7.

Seems like the tests are failing, could you take a look at that?

The test fails because the version I check on should be replaced with the next release. I tested with 8.x-dev and all tests passed. I'll fix that as soon as the new framework version is released

[freekmurze]

I can do that for laravel framework v8 but not for v7.

Let's just drop support for Laravel 7.

I'll fix that as soon as the new framework version is released

Great, thanks! Feel free to poke me again then.

[marvin-wtt]

I updated the framework version to v8.76.1

@freekmurze Do you think it would be useful to not only query for the current locale but for all enabled locales to better support url sharing? Currently url sharing is only possible within the same locale.
To accomplish that it would be better to store the translated slugs in a separate table to make the query more effizient imo.

[freekmurze]

Do you think it would be useful to not only query for the current locale but for all enabled locales to better support url sharing?

I think just enable this for the current locale is fine 👍

Thanks for your work on this PR.

[marvin-wtt]

@freekmurze First of all thanks for merging

I think just enable this for the current locale is fine 👍

I need this for a project anyway. The question is if you'd like to have this as a feature in this project and what your opinion on a separate table to improve querying is.

My idea would be like this:

id -> bigint
model -> string
model_id -> bigint
locale -> string
slug -> string

This could be a PR for laravel-translatable as well but I think it is intended to keep it as json for simplicity.

Alternatively I could also image to query the model like this:

foreach ($locales as $locale) {
    $query->orWhere("{$field}->{$locale}", $value);
}

But afaIk, it is bad practice to query with json like this

[keizah7]

sail@f34348e1ecf9:/var/www/html$ composer require spatie/laravel-sluggable
Using version ^3.2 for spatie/laravel-sluggable
./composer.json has been updated
Running composer update spatie/laravel-sluggable
Loading composer repositories with package information
Updating dependencies
Your requirements could not be resolved to an installable set of packages.

  Problem 1
    - Root composer.json requires spatie/laravel-sluggable ^3.2 -> satisfiable by spatie/laravel-sluggable[3.2.0].
    - spatie/laravel-sluggable 3.2.0 requires illuminate/database ^8.76.1 -> found illuminate/database[v8.76.1, ..., 8.x-dev] but these were not loaded, likely because it conflicts with another require.


Installation failed, reverting ./composer.json and ./composer.lock to their original content.

[marvin-wtt]

What other packages do you use? And which version do you use for your project?

[keizah7]

I was able to install only after updating all packages