Slugging behavior when creating a new model varies depending on onUpdate setting

[seitenumbruch]

Expected behavior

Slugging behavior when creating a new model should be independent of setting onUpdate.

Actual behavior

$model = new Example();
$model->title = "The Title";
$model->slug = "title";
$model->save();

// 'onUpdate' => false
echo $model->slug;    "title"

// 'onUpdate' => true
echo $model->slug;    "the-title"  <-- unexpected

I believe, this stems from the needsSlugging() method:

protected function needsSlugging(string $attribute, array $config): bool
    {
        $value = $this->model->getAttributeValue($attribute);

        if (
            $config['onUpdate'] === true ||
            $value === null ||
            trim($value) === ''
        ) {
            return true;
        }

        if ($this->model->isDirty($attribute)) {
            return false;
        }

        return (!$this->model->exists);
    }

Is this intentional?

You could make the argument that manually setting the slug doesn't make much sense when the model is configured to (re)generate the slug onUpdate. And I would agree. Nevertheless, I believe that this change in behavior is not immediately obvious and can easily be missed: You would not expect the onUpdate setting to affect the behavior on model creation.

Use Case

Why would I even want to manually set the slug? Example:

The model Product is set up to automatically generate a slug from the field 'title'. However, the web shop owner may in some rare cases prefer to manually define a slug if they want the slug to deviate from the title (like in the example above).

I believe, this is what #272 was trying to achieve too.

No resolution required from my end. It does not affect me. I just stumbled upon this while refactoring some code and wanted to let you know, in case you weren't aware of this.

Thank You for a great package

[cviebrock]

Hmmm ... I see what you're saying. If you create a new model and manually set the slug, then it shouldn't generate one for you. That makes logical sense to me, and could probably be solved by just reversing the order of the two if statements in the needsSlugging method.

It's probably a breaking change, though. I'd need to test it out against all the unit tests to be sure (and even then, it could still be breaking the behaviour that others have come to rely on).

An alternative would be to add a new check before the other two: if the model is new and the attribute has a value, then just leave it alone. Does that make sense to you?

[seitenumbruch]

I agree with all your thoughts. I believe that "fixing" this – no matter how you go about it – would always result in a breaking change, since changing existing behaviour is what this is all about.

As to your two first suggestions/thoughts:

1. Reordering the if statements

could probably be solved by just reversing the order of the two if statements in the needsSlugging method

This would affect the behaviour on updates as well. In other words: When updating a model and manually setting a slug, Sluggable won't generate a slug. While this makes logical sense to me, it does undermine your suggestion in the docs:

If you want to regenerate one or more of your model's slug fields, you can set those fields to null or an empty string before the update.

This would no longer work.

2. Don't generate slug if model is new and attribute has value

add a new check before the other two: if the model is new and the attribute has a value, then just leave it alone.

While this approach would avoid the issue of no longer being able to force slug generation on update, I am not a fan of it. In the name of fixing one inconsistency it would introduce another: Manually setting a slug would result in different behaviour when updating a model than when creating one.

My thoughts

I, personally, would go with your first suggestion (reordering if-statements). Yes, you'd lose the ability to force generating a new slug on update by setting the field to null, but I am not a fan of this "hack" anyways. I'd much rather make use of the SlugService::createSlug() method and explicitly generate a new slug.

[seitenumbruch]

I thought about this a little more. Maybe this is of help?

Short:

protected function needsSlugging(string $attribute, array $config): bool
{
    if ($value === null && $trim($value) === '') {
        return true;
    }

    if ($this->model->isDirty($attribute)) {
        return false;
    }

    if ($this->model->exists && $config['onUpdate'] === true) {
        return true;
    }

    return false;
}

The same again, but with explanatory comments:

protected function needsSlugging(string $attribute, array $config): bool
{
    // IF 1: Always generate a new slug if the current value is invalid

    if ($value === null && $trim($value) === '') {
        return true;
    }

    // IF 2: Never generate a new slug if a valid slug (see IF 1) has been set manually

    if ($this->model->isDirty($attribute)) {
        return false;
    }

    /*
     * I believe that at this point only updates remain, since all new model creations
     * either have a manually set slug value (triggered IF 2)
     * or don't have any slug value at all (triggered IF 1).
     * 
     * I believe that what remains are existing models with valid slug values in the database.
     * However, I am not sure about that, which is why I still check for the models existence in the next if-condition.
     */

    // IF 3: Generate a new slug if the current value is valid (see IF 1)
    // and has not been set manually (see IF 2)
    // and this is an update and a new slug should be generated on updates

    if ($this->model->exists && $config['onUpdate'] === true) {
        return true;
    }

    /*
     * Now all that remains should be existing models with valid slug values in the database
     * for which onUpdate is set to false.
     */ 

    return false;
}

Please take all my input with a huge grain of salt. I am not an experienced developer at all and my understanding of PHP is very limited.

Edit: With this approach it would still be possible to force generating a new slug by setting the slug to null.

[seitenumbruch]

I am very sorry, I must have accidentally closed this issue :/ You can tell this is my first ever issue...