Revisit the full slug saving flow

Tofandel · Tofandel · commit f1c987cbb0d5 · 2024-11-21T09:08:20.000+01:00
diff --git a/docs/content/2_guides/1_page-builder-with-blade/5_configuring-the-page-module.md b/docs/content/2_guides/1_page-builder-with-blade/5_configuring-the-page-module.md
@@ -168,7 +168,7 @@ class Page extends Model
         'description',
     ];
 
-    public $slugAttributes = [
+    protected $slugFields = [
         'title',
     ];
 
diff --git a/docs/content/2_guides/building_a_multilingual_site_with_twill_and_laravel_localization/Article.php b/docs/content/2_guides/building_a_multilingual_site_with_twill_and_laravel_localization/Article.php
@@ -24,7 +24,7 @@ class Article extends Model implements LocalizedUrlRoutable
         'description',
     ];
 
-    public $slugAttributes = [
+    protected $slugFields = [
         'title',
     ];
 
diff --git a/docs/content/2_guides/prefill-block-editor-from-template/Article.php b/docs/content/2_guides/prefill-block-editor-from-template/Article.php
@@ -57,7 +57,7 @@ class Article extends Model implements Sortable
 
     // #endregion fillable
 
-    public $slugAttributes = [
+    private $slugFields = [
         'title',
     ];
 
diff --git a/examples/basic-page-builder/app/Models/Page.php b/examples/basic-page-builder/app/Models/Page.php
@@ -24,7 +24,7 @@ class Page extends Model
         'description',
     ];
 
-    public $slugAttributes = [
+    protected $slugFields = [
         'title',
     ];
 }
diff --git a/examples/portfolio/app/Models/Partner.php b/examples/portfolio/app/Models/Partner.php
@@ -24,7 +24,7 @@ class Partner extends Model
         'description',
     ];
 
-    public $slugAttributes = [
+    protected $slugFields = [
         'title',
     ];
 
diff --git a/examples/portfolio/app/Models/Project.php b/examples/portfolio/app/Models/Project.php
@@ -27,7 +27,7 @@ class Project extends Model
         'description',
     ];
 
-    public $slugAttributes = [
+    protected $slugFields = [
         'title',
     ];
 
diff --git a/examples/tests-capsules/app/Twill/Capsules/Posts/Models/Post.php b/examples/tests-capsules/app/Twill/Capsules/Posts/Models/Post.php
@@ -26,7 +26,7 @@ class Post extends Model implements Sortable
 
     public $translatedAttributes = ['title', 'description'];
 
-    public $slugAttributes = ['title'];
+    public $slugFields = ['title'];
 
     public $mediasParams = [
         'cover' => [
diff --git a/examples/tests-modules/app/Models/Author.php b/examples/tests-modules/app/Models/Author.php
@@ -56,7 +56,7 @@ class Author extends Model implements Sortable
     public $translatedAttributes = ['name', 'description', 'bio'];
 
     // uncomment and modify this as needed if you use the HasSlug trait
-    public $slugAttributes = ['name'];
+    protected $slugFields = ['name'];
 
     // add checkbox fields names here (published toggle is itself a checkbox)
     public $checkboxes = ['published'];
diff --git a/examples/tests-modules/app/Models/Category.php b/examples/tests-modules/app/Models/Category.php
@@ -25,7 +25,7 @@ class Category extends Model
     public $translatedAttributes = ['title'];
 
     // uncomment and modify this as needed if you use the HasSlug trait
-    public $slugAttributes = ['title'];
+    protected $slugFields = ['title'];
 
     // add checkbox fields names here (published toggle is itself a checkbox)
     public $checkboxes = ['published'];
diff --git a/examples/tests-singleton/app/Models/ContactPage.php b/examples/tests-singleton/app/Models/ContactPage.php
@@ -25,7 +25,7 @@ class ContactPage extends Model
         'description',
     ];
 
-    public $slugAttributes = [
+    protected $slugFields = [
         'title',
     ];
 
diff --git a/examples/tests-subdomain-routing/app/Models/Page.php b/examples/tests-subdomain-routing/app/Models/Page.php
@@ -28,7 +28,7 @@ class Page extends Model implements Sortable
         'description',
     ];
 
-    public $slugAttributes = [
+    public $slugFields = [
         'title',
     ];
 
diff --git a/src/Commands/stubs/model.stub b/src/Commands/stubs/model.stub
@@ -21,8 +21,9 @@ class {{modelClassName}} extends Model {{modelImplements}}
         'description',
     ];
     {{/hasTranslation}}{{hasSlug}}
-    public $slugAttributes = [
+    protected $slugFields = [
         'title',
     ];
+    protected $slugDeps = [];
     {{/hasSlug}}
 }
diff --git a/src/Models/Behaviors/HasSlug.php b/src/Models/Behaviors/HasSlug.php
@@ -144,7 +144,7 @@ private function handleSlugsSaving(): void
             $slugParams = $this->twillSlugData;
 
             foreach ($slugParams as $locale => $params) {
-                $slugParams[$locale] = array_merge($this->getSlugParams($locale) ?? [], $params);
+                $slugParams[$locale] = array_merge($this->getSlugParams($locale, empty($params['slug'])) ?? [], $params);
             }
             $slugParams = array_filter($slugParams, fn($p) => !empty($p['slug']));
         }
@@ -176,7 +176,13 @@ public function updateOrNewSlug(array $slugParams): void
                 if ($this->relationLoaded('slugs')) {
                     // Report update to slugs so that getSlug() returns the correct value
                     $slug = $this->slugs->whereKey($oldMatchingSlug->getKey());
-                    $slug->active = $isNowActive;
+                    if ($slug) {
+                        $slug->active = $isNowActive;
+                        $slug->syncOriginalAttribute('active');
+                    } else {
+                        // The relation was loaded before the old slug even existed, unload it and let it lazy reload as needed
+                        $this->unsetRelation('slugs');
+                    }
                 }
                 if ($isNowActive) {
                     $this->disableLocaleSlugs($oldMatchingSlug->locale, $oldMatchingSlug->getKey());
@@ -220,9 +226,7 @@ protected function addOneSlug(array $slugParams, $alreadySuffixed = false): void
             $slugParams['slug'] = $this->suffixSlugIfExisting($slugParams);
         }
 
-        $slugParams[$this->getForeignKey()] = $this->getKey();
-
-        $slugModel = \Illuminate\Database\Eloquent\Model::unguarded(fn() => $this->getSlugModelClass()::create($slugParams));
+        $slugModel = \Illuminate\Database\Eloquent\Model::unguarded(fn() => $this->slugs()->create($slugParams));
 
         if ($this->relationLoaded('slugs')) {
             // Report update to slugs so that getSlug() returns the correct value
@@ -248,7 +252,10 @@ public function disableLocaleSlugs(string|array $locale = null, int|array $excep
             $this->slugs->where('active', true)
                 ->whereNotIn('id', Arr::wrap($except_slug_id))
                 ->whereIn('locale', Arr::wrap($locale))
-                ->each(fn($slug) => $slug->active = false);
+                ->each(function (Model $slug) {
+                    $slug->active = false;
+                    $slug->syncOriginalAttribute('active');
+                });
         }
     }
 
@@ -282,11 +289,13 @@ private function suffixSlugIfExisting(array $slugParams): string
     }
 
     /**
-     * @deprecated
+     * @deprecated use suffixSlugIfExisting instead
      * Checks if a slug needs a suffix due to a conflict with another model.
      */
     private function slugNeedsSuffix(array $slugParams): bool
     {
+        trigger_deprecation('area17/twill', '3.5', 'The slugNeedsSuffix method is deprecated and will be removed in 4.x use suffixSlugIfExisting instead');
+
         unset($slugParams['active']);
 
         $hasExisting = false;
@@ -349,16 +358,28 @@ public function getSlugAttribute(): string
         return $this->getSlug();
     }
 
+    public function getSlugDeps(): array
+    {
+        return $this->slugDeps ?? array_slice($this->slugAttributes ?? [], 1);
+    }
+    public function getSlugFields(): array
+    {
+        if (!isset($this->slugFields) && isset($this->slugAttributes)) {
+            trigger_deprecation('area17/twill', '3.5', 'The slugAttributes property has been deprecated instead define slug fields with the slugFields property and additional columns with the slugDeps property');
+        }
+        return $this->slugFields ?? array_slice($this->slugAttributes ?? [], 0, 1);
+    }
+
     public function getSlugParams(?string $locale = null, bool $skipUnchanged = false): ?array
     {
         $translatedAttributes = $this->getTranslatedAttributes();
         $slugParams = [];
-        $attributes = $this->slugAttributes ?? [];
-        $deps = $this->slugDependencies ?? [];
-        if (empty($attributes)) {
-            return null;
+        $deps = $this->getSlugDeps();
+        $fields = $this->getSlugFields();
+        if (empty($fields)) {
+            return $locale === null ? [] : null;
         }
-        foreach (getLocales() as $appLocale) {
+        foreach ($locale ? [$locale] : getLocales() as $appLocale) {
             if ($appLocale === $locale || $locale === null) {
                 $wasChanged = $this->wasRecentlyCreated;
 
@@ -384,9 +405,9 @@ public function getSlugParams(?string $locale = null, bool $skipUnchanged = fals
                     return $this->$attribute;
                 };
 
-                $slugAttributes = array_filter(array_map($getAttributeValue, $attributes));
+                $slugFields = array_filter(array_map($getAttributeValue, $fields));
 
-                if (empty($slugAttributes)) {
+                if (empty($slugFields)) {
                     // Skip empty slugs
                     continue;
                 }
@@ -397,7 +418,7 @@ public function getSlugParams(?string $locale = null, bool $skipUnchanged = fals
                 }
                 $slugParam = [
                         'active' => $translation?->active ?? true,
-                        'slug' => Arr::join($slugAttributes, '-'),
+                        'slug' => Arr::join($slugFields, '-'),
                         'locale' => $appLocale,
                     ] + $slugDependencies;
 
diff --git a/src/Repositories/Behaviors/HandleSlugs.php b/src/Repositories/Behaviors/HandleSlugs.php
@@ -11,7 +11,7 @@ trait HandleSlugs
 {
     public function beforeSaveHandleSlugs(TwillModelContract $object, array $fields): void
     {
-        if (property_exists($this->model, 'slugAttributes')) {
+        if (method_exists($this->model, 'getSlugFields')) {
             $object->twillSlugData = [];
             $submittedLanguages = Collection::make($fields['languages'] ?? []);
 
@@ -63,7 +63,7 @@ public function getSlugParameters(TwillModelContract $object, array $fields, arr
 
         $slugParams = $object->getSlugParams($slug['locale']);
 
-        foreach ($object->slugDependencies ?? [] as $param) {
+        foreach ($object->slugAttributes as $param) {
             if (isset($slugParams[$param]) && isset($fields[$param])) {
                 $slug[$param] = $fields[$param];
             } elseif (isset($slugParams[$param])) {
diff --git a/tests/integration/Anonymous/AnonymousModule.php b/tests/integration/Anonymous/AnonymousModule.php
@@ -94,6 +94,12 @@ class AnonymousModule
     /** @var string[] */
     private array $slugAttributes = [];
 
+    /** @var string[] */
+    private array $slugFields = [];
+
+    /** @var string[] */
+    private array $slugDeps = [];
+
     protected function __construct(public string $namePlural, public Application $app)
     {
         $this->classPrinter = new PsrPrinter();
@@ -203,6 +209,22 @@ public function withSlugAttributes(array $fields): self
         return $this;
     }
 
+
+    public function withSlugFields(array $fields): self
+    {
+        $this->slugFields = $fields;
+
+        return $this;
+    }
+
+    public function withSlugDeps(array $fields): self
+    {
+        $this->slugDeps = $fields;
+
+        return $this;
+    }
+
+
     /**
      * Boots the anonymous module and returns the model class.
      */
@@ -626,6 +648,18 @@ private function getModelClass(string $classNameWithNamespace): PhpNamespace
                 $this->slugAttributes
             );
         }
+        if ($this->slugDeps !== []) {
+            $class->addProperty(
+                'slugDeps',
+                $this->slugDeps
+            );
+        }
+        if ($this->slugFields !== []) {
+            $class->addProperty(
+                'slugFields',
+                $this->slugFields
+            );
+        }
 
         foreach ($this->belongsToMany as $name => $target) {
             $method = $class->addMethod($name);
diff --git a/tests/integration/Models/StandaloneSlugTest.php b/tests/integration/Models/StandaloneSlugTest.php
@@ -44,8 +44,8 @@ public function testSavingStandaloneSlug(): void
         $this->assertEquals(10, count($log));
         $this->assertEquals('new-test-author', $author->getSlug('en'));
         $this->assertEquals('nouveau-test-auteur', $author->getSlug('fr'));
-        // All queries combined should take less than 15ms (they usually take a total of 5ms)
-        $this->assertLessThan(15, array_sum(Arr::pluck($log, 'time')));
+        // All queries combined should take less than 40ms (they usually take a total of 5ms, allow big range to avoid flaky test)
+        $this->assertLessThan(40, array_sum(Arr::pluck($log, 'time')));
 
         $author2 = $createAuthor();
         $this->assertEquals('test-author-2', $author2->slug);
diff --git a/tests/integration/SlugTest.php b/tests/integration/SlugTest.php
@@ -2,11 +2,9 @@
 
 namespace A17\Twill\Tests\Integration;
 
+use A17\Twill\Models\Behaviors\HasSlug;
+use A17\Twill\Models\Model;
 use A17\Twill\Tests\Integration\Anonymous\AnonymousModule;
-use App\Models\Author;
-use App\Repositories\AuthorRepository;
-use Illuminate\Support\Arr;
-use Illuminate\Support\Facades\DB;
 
 class SlugTest extends TestCase
 {
@@ -34,7 +32,7 @@ public function testMultipleSlugAttributes()
                 'first_name' => [],
                 'last_name' => [],
             ])
-            ->withSlugAttributes([
+            ->withSlugFields([
                 'last_name', 'first_name'
             ])
             ->boot();
@@ -179,4 +177,24 @@ public function testCanReuseSoftDeletedSlugWithHistory(): void
         $this->assertCount(2, $model->slugs()->get());
         $this->assertEquals('slug-update-2', $model->getSlug());
     }
+
+    public function testCustomSlugDoesntChangeOnUpdate(): void
+    {
+        /** @var Model|HasSlug $model */
+        $model = $this->module->getRepository()->create([
+            'title' => 'My title',
+            'slug' => ['en' => 'my-custom-slug'],
+        ]);
+
+        $this->assertEquals('my-custom-slug', $model->getSlug());
+        $this->assertEquals(1, $model->slugs()->count());
+
+        $model = $this->module->getRepository()->update($model->id, ['position' => 1]);
+
+        $this->assertEquals(1, $model->slugs()->count());
+        $this->assertEquals('my-custom-slug', $model->getSlug());
+
+        $model = $this->module->getRepository()->update($model->id, ['title' => 'My new title']);
+        $this->assertEquals('my-new-title', $model->getSlug());
+    }
 }

Original file line number	Diff line number	Diff line change
`@@ -24,7 +24,7 @@ class Page extends Model`
`24`	`24`	`'description',`
`25`	`25`	`];`
`26`	`26`
`27`		`- public $slugAttributes = [`
	`27`	`+ protected $slugFields = [`
`28`	`28`	`'title',`
`29`	`29`	`];`
`30`	`30`	`}`