Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Slugging after save problems #543

Closed
adamthehutt opened this issue Jan 15, 2021 · 16 comments
Closed

Slugging after save problems #543

adamthehutt opened this issue Jan 15, 2021 · 16 comments

Comments

@adamthehutt
Copy link

This recent change caused a serious BC breakage for me related to pre-save model validation. I also question whether it's a good idea from a performance standpoint, since it adds another roundtrip to the database every time a model is saved.

Can this be configurable instead? I would think the default behavior should be to generate the slug prior to saving, but a config setting could allow that to be overridden if necessary.

I can produce a PR for this if you'd like @cviebrock.

@cviebrock
Copy link
Owner

Hi @adamthehutt ... I realized it's a BC change, and I've deleted the tag that implemented it for now.

I also realized it adds another DB call per save, but that didn't bother me much from a performance standpoint, when weighed against the benefit of being able to use the primary key in your source fields.

Yes, it could be configurable ... but that would make the SluggableObserver class a bit more convoluted. I'd be more curious as to what your pre-save validation looks like. If it's a common pattern, than I'll revert these changes, or mark them BC, or make it configurable.

@adamthehutt
Copy link
Author

@cviebrock I don't know how common it is. It's a bit of a departure from the standard Laravel approach to validation. But essentially my models are all self-validating, which ensures that the same data integrity logic is applied regardless of where the model is created (e.g., a form, a webhook, an artisan command, a job, etc.)

In this context, the validation logic says that a model has to have a slug, so when none is present during validation, the save is aborted. This isn't a huge deal and I could work around it in a few different ways, but I'd prefer not to.

Anyway, let me know what you think and if I can help. Thanks!

@cviebrock
Copy link
Owner

@adamthehutt Any chance you can try the master branch now? Take a look at this:

https://github.com/cviebrock/eloquent-sluggable#when-is-a-model-slugged

Specifically, you'd need to add this to your model:

    public function sluggableEvent(): string
    {
        return SluggableObserver::SAVING;
    }

Maybe I'll change it so the default is the old method, and make the breaking change for the next major release. But if you can let me know if that fixes your issue, I'll move forward.

Thanks!

@adamthehutt
Copy link
Author

@cviebrock Thank you! This fixes my specific problem, yes.

However, fixing that issue revealed another, more serious one. The observer's saving() method now returns false when no slug is generated (e.g., because one already exists). When a Laravel observer method returns false it entirely aborts the related event. That means any time that the slugging doesn't happen (including, as far as I can tell, on a regular update) the save is aborted entirely.

I restored the void return in the observer method and everything is back to working perfectly.

@andrey-helldar
Copy link

andrey-helldar commented Jan 16, 2021

I have two cases:

Option 1: Using the default observer

Trait
<?php

namespace App\Concerns\Models;

use Cviebrock\EloquentSluggable\Sluggable;

trait HasSluggable
{
    use Sluggable;

    public function sluggable(): array
    {
        return ['slug' => ['source' => 'title']];
    }
}
Result:

SQLSTATE[HY000]: General error: 1364 Field 'slug' doesn't have a default value (SQL: insert into package_spaces (user_service_id, title, updated_at, created_at) values (1, foo, 2021-01-16 22:49:39, 2021-01-16 22:49:39))

Option 2: Using a custom observer

Trait
<?php

namespace App\Concerns\Models;

use App\Observers\Sluggable as SluggableObserver;
use Cviebrock\EloquentSluggable\Sluggable;

trait HasSluggable
{
    use Sluggable;

    public static function bootSluggable()
    {
        static::observe(SluggableObserver::class);
    }

    public function sluggable(): array
    {
        return ['slug' => ['source' => 'title']];
    }
}
Observer
<?php

namespace App\Observers;

use Cviebrock\EloquentSluggable\SluggableObserver;
use Illuminate\Database\Eloquent\Model;

class Sluggable extends SluggableObserver
{
    public function saving(Model $model): bool
    {
        return $this->generateSlug($model, 'saving');
    }

    public function saved(Model $model): bool
    {
        return true;
    }
}
Result

image

image

Called code

<?php

use App\Models\PackageSpace;
use Illuminate\Support\Facades\Artisan;

Artisan::command('foo', function () {
    PackageSpace::create([
        'user_service_id' => 1,

        'title' => 'foo',
    ]);
});

Schema

CREATE TABLE `package_spaces` (
  `id` bigint unsigned NOT NULL AUTO_INCREMENT,
  `user_service_id` bigint unsigned NOT NULL,
  `slug` varchar(255) COLLATE utf8mb4_unicode_ci NOT NULL,
  `title` varchar(255) COLLATE utf8mb4_unicode_ci NOT NULL,
  `description` varchar(512) COLLATE utf8mb4_unicode_ci DEFAULT NULL,
  `is_user` tinyint(1) NOT NULL DEFAULT '1',
  `is_private` tinyint(1) NOT NULL DEFAULT '0',
  `is_active` tinyint(1) NOT NULL DEFAULT '1',
  `created_at` timestamp NULL DEFAULT NULL,
  `updated_at` timestamp NULL DEFAULT NULL,
  `deleted_at` timestamp NULL DEFAULT NULL,
  PRIMARY KEY (`id`),
  UNIQUE KEY `package_spaces_user_service_id_slug_unique` (`user_service_id`,`slug`)

cviebrock added a commit that referenced this issue Jan 17, 2021

Verified

This commit was signed with the committer’s verified signature.
driesvints Dries Vints
@cviebrock
Copy link
Owner

@adamthehutt I've pushed some more changes to master

  • revert to using saving as the default event (but keep it configurable)
  • fix the return values for the events to prevent event aborting

Mind giving it a test again in your case? Thanks, and sorry for the hassle!

@cviebrock
Copy link
Owner

@andrey-helldar If you choose to configure the package to do the slugging on the saved event (which was going to be the new default, but I've reverted that), then your slug column needs to be nullable.

@andrey-helldar
Copy link

@cviebrock, according to the logic of the application, the slug field cannot be null.
This raises a package issue.
I changed the calls from saved to saving method, thus solving the problem.

@cviebrock
Copy link
Owner

@andrey-helldar I'm still not sure I follow you.

As it stands, the new version I plan on tagging (based on master right now) works identically to older versions: the models are slugged on saving, so your DB column can be set to NOT NULL because the package will set a value for that field before the model is persisted to the DB.

If you change it to use the saved method (which I repeat is not the default), then you should make your slug column nullable. This is because the model will be saved first without the slug, then the slug is calculated and saved in a second DB query. If the column is NOT NULL, then that first save will fail.

If you've got a test case that shows that it still isn't working after my most recent changes, I'd be happy to take a look. Obviously, I don't want to break things for anyone!

@andrey-helldar
Copy link

@cviebrock, language barrier at its best 😀 (no)

I will try to write so that Google Translate will correctly translate my idea.

So, in the current release, models are slugged after writing to the database. This means I have to make the slug field as NULLABLE. But I cannot do this.

My solution is override the save methods. My working code is shown above.

In the next release, you will fix this and I will be able to remove my code from the project. And yours will remain :)

PS: Hopefully Google translated it correctly. If not, ignore it. I'll just wait for the next release 🍻

@adamthehutt
Copy link
Author

Thanks, @cviebrock! Seems to be good now.

@andrey-helldar
Copy link

Thanks, @cviebrock! Seems to be good now.

Power of the downgrade 😀

@cviebrock
Copy link
Owner

Thanks for the help everyone. I've released 8.0.3 which still uses the saving event, so nothing should be broken for anyone. But please: open up a new ticket if you find an issue!

@dluague
Copy link

dluague commented Jan 20, 2021

Saving still break when updating a model because of this because it will return true or false, if it return false Laravel will terminate the update.

cviebrock added a commit that referenced this issue Jan 20, 2021
@cviebrock
Copy link
Owner

@dluague Try 8.0.4 now.

@cviebrock cviebrock reopened this Jan 20, 2021
@dluague
Copy link

dluague commented Jan 21, 2021

Thanks, @cviebrock it works fine now in my end using 8.0.4.

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

No branches or pull requests

4 participants