Remove redundant makeSlugUnique method overriding

[serdud]

\Cviebrock\EloquentSluggable\Services\SlugService::generateSuffix expects 4 arguments, but only 3 passed.

[welcome]

BOOM! Your first PR with us, thank you so much! Someone will take a look at it shortly.

Please keep in mind that:

• if this constitutes a breaking change, it might take quite a while for this to get merged; we try to emulate the Laravel release cycle as much as possible, so developers can upgrade both software once; this means a new big release every ~6 months;
• even if it's a non-breaking change, it might take a few days/weeks for the PR to get merged; unless it's a no-brainer, we like to have some community feedback on new features, before we merge them; this leads to higher-quality code, in the end; we learnt this the hard way :-)
• not all PRs get merged; sometimes we just have to hold out new features, to keep the packages lean; sometimes we don't include features that only apply to niche use cases;
• we're not perfect; if you think we're wrong, call us out on it; but in a kind way :-) we all make mistakes, best we learn from them and build better software together;

Thank you!

--
Justin Case
The Backpack Robot

[promatik]

Apparently a month ago, on v8.0.5, eloquent-sluggable introduced this BC for us 😕

Here: cviebrock/eloquent-sluggable@ca8c166#diff-0076c2a3786bba62c985d7d0220f2ab44266ad8d673df620b2789d8ffd7c0eb4R243

Merging this PR will make old code stop working (if in a rare situation devs update CRUD without updating eloquent-sluggable).

I analysed src/app/Models/Traits/SpatieTranslatable/SlugService.php, and this is the diff on the makeSlugUnique function:

I think we are safe to stop overriding this function and remove it completely.

@pxpm and @tabacitu please give your opinion on this ✌

[pxpm]

@promatik from code perspective it seems safe to don't override, my thinking here is why was it overriden to start with.

It was to use call_user_func instead of $callable() ? Or is checking if !$slug incompatible with backpack and was purposely removed ?

Did you tested it ? If you did and it works I think we are safe to remove the function. Any other reason to override the function ?

Let me know,
Pedro

[bedoz]

Same here, when i used clone feature this error appears.
Can this PR be merged asap?

[pxpm]

I'v just tested removing the function override and everything seems to work fine.

Tests in demo:

Product.php model:

use Backpack\CRUD\app\Models\Traits\SpatieTranslatable\HasTranslations;
use Backpack\CRUD\app\Models\Traits\SpatieTranslatable\Sluggable;
use Backpack\CRUD\app\Models\Traits\SpatieTranslatable\SluggableScopeHelpers;


class Product extends Model
{
    use CrudTrait;
    use HasTranslations, Sluggable, SluggableScopeHelpers;

public function sluggable(): array
    {
        return [
            'description' => [
                'source' => 'name',
                'uniqueSuffix' => function() {
                    return '-2'.time();
                }
            ],
        ];
    }

Thanks,
Pedro

[pxpm]

Sorry, I didn't meant to approve.

I meant we should remove the function completely so I can approve it. :)

if @serdud don't have time to do it now, I will do it and submit the PR on @promatik confirms we don't need to override that function.

Let me know,
Pedro

[serdud]

@pxpm
Done

[scrutinizer-notifier]

The inspection completed: No new issues

[tabacitu]

I agree, it looks to me like that method is identical in our overwrite and the "before" in cviebrock/eloquent-sluggable@ca8c166#diff-0076c2a3786bba62c985d7d0220f2ab44266ad8d673df620b2789d8ffd7c0eb4R243 that @promatik tracked down.

So removing it entirely should be fine. I have no idea why we had it in the first place... I expect we did have a reason though 😅 So let's be careful here.

@pxpm / @promatik did you also test using the lowest possible versions of laravel and cviebrock, with this change?
I'd make a new Laravel 6 project, install Backpack 4.1 on top of it with a translatable CRUD, then run composer update --prefer-lowest. Same with L7. Let's be extra-careful here please 🙏

[pxpm]

I think we are safe to remove the override. I'v created branch https://github.com/Laravel-Backpack/CRUD/tree/backpack-dependencies-tests while doing this research.

This is what I did:

• Created a new phpunit-dependencies.xml config file that instead of loading the backpack/crud vendor autoload, loads the autoload in the "main project" and runs only the tests specific to dependencies. (cviebrock sluggable is only available in the main project, it's not a crud dependency).
• Created a test that uses translatable slugs to test the functionality.
• Removed the override from backpack trait.

Tested with L8, downgraded Laravel down to 6 and the test passes in all 3 versions without the override.

To use the dependencies config while testing, run phpunit with vendor/bin/phpunit --testdox --configuration=phpunit-dependencies.xml

Let me know,
Pedro

[promatik]

Just to make sure of this, I dug both repos 😅
First time this file was commited the original file on cviebrock repo was equal to our current file.
So, even there, no changes to the original function.

The original file was then updated, here and here, our wasn't.

Those were minor changes, old laravel version support and bug fixes, so I believe this is safe to merge ✌

[tabacitu]

Ok let's do this 🤞 😅
Thanks a lot guys.