- Sponsor
-
Notifications
You must be signed in to change notification settings - Fork 461
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
Comments
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 |
@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! |
@adamthehutt Any chance you can try the 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! |
@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. |
I have two cases: Option 1: Using the default observerTrait<?php
namespace App\Concerns\Models;
use Cviebrock\EloquentSluggable\Sluggable;
trait HasSluggable
{
use Sluggable;
public function sluggable(): array
{
return ['slug' => ['source' => 'title']];
}
} Result:
Option 2: Using a custom observerTrait<?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;
}
} ResultCalled code<?php
use App\Models\PackageSpace;
use Illuminate\Support\Facades\Artisan;
Artisan::command('foo', function () {
PackageSpace::create([
'user_service_id' => 1,
'title' => 'foo',
]);
}); SchemaCREATE 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`) |
@adamthehutt I've pushed some more changes to master
Mind giving it a test again in your case? Thanks, and sorry for the hassle! |
@andrey-helldar If you choose to configure the package to do the slugging on the |
@cviebrock, according to the logic of the application, the |
@andrey-helldar I'm still not sure I follow you. As it stands, the new version I plan on tagging (based on If you change it to use the 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! |
@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 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 🍻 |
Thanks, @cviebrock! Seems to be good now. |
Power of the downgrade 😀 |
Thanks for the help everyone. I've released 8.0.3 which still uses the |
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. |
@dluague Try 8.0.4 now. |
Thanks, @cviebrock it works fine now in my end using 8.0.4. |
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.
The text was updated successfully, but these errors were encountered: