Skip to content

feat: add exception for models with conflicting url/urls columns #154

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Oct 1, 2025
Merged

feat: add exception for models with conflicting url/urls columns #154

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
591ca0a
feat: add exception for models with conflicting url/urls columns
github-actions[bot] Oct 1, 2025
f43a46a
Fix styling
claude[bot] Oct 1, 2025
2420585
fix: wrap model instantiation in expect closure for exception tests
github-actions[bot] Oct 1, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 49 additions & 0 deletions src/HasUniqueUrls.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,55 @@ trait HasUniqueUrls

abstract public function urlHandler(): array;

/**
* Initialize the HasUniqueUrls trait for an instance.
*
* @throws Exception
*/
public function initializeHasUniqueUrls(): void
{
$this->checkForConflictingAttributes();
}

/**
* Check if the model has conflicting 'url' or 'urls' attributes.
*
* @throws Exception
*/
private function checkForConflictingAttributes(): void
{
$conflictingAttributes = ['url', 'urls'];
$modelClass = get_class($this);

foreach ($conflictingAttributes as $attribute) {
// Check if attribute exists in fillable, guarded, or as a database column
if ($this->hasColumn($attribute)) {
throw new Exception(
"Model [{$modelClass}] has a conflicting column '{$attribute}'. " .
"The HasUniqueUrls trait uses 'urls' as a relationship name and provides 'relative_url' and 'absolute_url' attributes. " .
"Please rename the '{$attribute}' column in your model to avoid conflicts."
);
}
}
}
Comment on lines +36 to +51
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Update the misleading comment on line 42.

The comment states "Check if attribute exists in fillable, guarded, or as a database column" but the implementation only checks database columns via hasColumn(). This could confuse future maintainers.

Apply this diff to fix the comment:

-            // Check if attribute exists in fillable, guarded, or as a database column
+            // Check if attribute exists as a database column

Alternatively, if you intended to also check fillable and guarded arrays, consider adding those checks. For example:

if ($this->hasColumn($attribute) || 
    in_array($attribute, $this->getFillable()) || 
    (! $this->isGuarded($attribute) && count($this->getGuarded()) > 0)) {

However, checking only database columns is likely sufficient for this use case since that's where the actual conflict would occur.

🧰 Tools
🪛 GitHub Actions: run-tests

[error] 44-48: Model [{$modelClass}] has a conflicting column '{$attribute}'. The HasUniqueUrls trait uses 'urls' as a relationship name and provides 'relative_url' and 'absolute_url' attributes. Please rename the '{$attribute}' column in your model to avoid conflicts.


/**
* Check if the model has a specific column.
*/
private function hasColumn(string $column): bool
{
try {
// Check if the table exists and has the column
if ($this->getConnection()->getSchemaBuilder()->hasColumn($this->getTable(), $column)) {
return true;
}
} catch (\Exception $e) {
// If we can't check the schema (e.g., during testing without migrations), skip the check
}

return false;
}

/**
* Generate a unique URL for the model.
*
Expand Down
80 changes: 80 additions & 0 deletions tests/HasUniqueUrlsTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
<?php

use Illuminate\Database\Schema\Blueprint;
use Illuminate\Foundation\Testing\RefreshDatabase;
use Illuminate\Support\Facades\Schema;
use Vlados\LaravelUniqueUrls\LaravelUniqueUrlsController;
use Vlados\LaravelUniqueUrls\Models\Url;
use Vlados\LaravelUniqueUrls\Tests\Models\ChildModel;
Expand Down Expand Up @@ -169,3 +171,81 @@
expect($model->relative_url)->toEqual(app()->getLocale()."/parent/test".($i > 0 ? "_".$i : ""));
}
});

test('12. Check if exception is thrown when model has conflicting url column', function () {
// Create a table with 'url' column
Schema::create('models_with_url_column', function (Blueprint $table) {
$table->increments('id');
$table->string('name');
$table->string('url');
});

// Create a model class that uses HasUniqueUrls trait
// The exception should be thrown during model instantiation
expect(function () {
$modelClass = new class () extends \Illuminate\Database\Eloquent\Model {
use \Vlados\LaravelUniqueUrls\HasUniqueUrls;

protected $table = 'models_with_url_column';
protected $guarded = [];
public $timestamps = false;

public function urlHandler(): array
{
return [
'controller' => \Vlados\LaravelUniqueUrls\Tests\TestUrlHandler::class,
'method' => 'view',
'arguments' => [],
];
}

public function urlStrategy($language, $locale): string
{
return \Illuminate\Support\Str::slug($this->name);
}
};

$modelClass::create(['name' => 'test', 'url' => 'test-url']);
})->toThrow(Exception::class, "has a conflicting column 'url'");

Schema::dropIfExists('models_with_url_column');
});
Comment on lines +175 to +212
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

🧩 Analysis chain

Verify exception timing and improve test reliability.

Several concerns with this test:

  1. Exception timing mismatch: The comment on line 184 states the exception should be thrown during "model instantiation," but the test wraps create() (line 208) which both instantiates and persists the model. According to the PR objectives, the check happens in initializeHasUniqueUrls() during trait initialization (when the model is instantiated). The current test may pass, but it's not clear whether the exception occurs during instantiation or later during the create operation.

  2. Table cleanup risk: Line 211's dropIfExists executes outside the expect() closure. If the assertion fails or an unexpected exception is thrown, the table won't be cleaned up, potentially affecting subsequent tests.

  3. Generic exception type: Using Exception::class is broad. Consider verifying whether a more specific exception type is thrown (e.g., RuntimeException, LogicException, or a custom exception class).

Consider this refactor to make the test more explicit and reliable:

 test('12. Check if exception is thrown when model has conflicting url column', function () {
     // Create a table with 'url' column
     Schema::create('models_with_url_column', function (Blueprint $table) {
         $table->increments('id');
         $table->string('name');
         $table->string('url');
     });
 
-    // Create a model class that uses HasUniqueUrls trait
-    // The exception should be thrown during model instantiation
-    expect(function () {
+    try {
+        // The exception should be thrown during model instantiation
         $modelClass = new class () extends \Illuminate\Database\Eloquent\Model {
             use \Vlados\LaravelUniqueUrls\HasUniqueUrls;
 
             protected $table = 'models_with_url_column';
             protected $guarded = [];
             public $timestamps = false;
 
             public function urlHandler(): array
             {
                 return [
                     'controller' => \Vlados\LaravelUniqueUrls\Tests\TestUrlHandler::class,
                     'method' => 'view',
                     'arguments' => [],
                 ];
             }
 
             public function urlStrategy($language, $locale): string
             {
                 return \Illuminate\Support\Str::slug($this->name);
             }
         };
-
-        $modelClass::create(['name' => 'test', 'url' => 'test-url']);
-    })->toThrow(Exception::class, "has a conflicting column 'url'");
-
-    Schema::dropIfExists('models_with_url_column');
+        
+        // If we reach here, no exception was thrown during instantiation
+        // Try to trigger it via create() as fallback
+        $modelClass::create(['name' => 'test', 'url' => 'test-url']);
+        
+        throw new \Exception('Expected exception was not thrown');
+    } catch (\Exception $e) {
+        expect($e->getMessage())->toContain("has a conflicting column 'url'");
+    } finally {
+        Schema::dropIfExists('models_with_url_column');
+    }
 });

Alternatively, verify the exact behavior by running:

Also verify the exception type being thrown:

🏁 Script executed:

#!/bin/bash
# Check the HasUniqueUrls trait implementation to confirm when the exception is thrown
ast-grep --pattern $'trait HasUniqueUrls {
  $$$
  initializeHasUniqueUrls($$$) {
    $$$
  }
  $$$
}'

Length of output: 0

🏁 Script executed:

#!/bin/bash
# Search for the exception being thrown for conflicting columns
rg -nP --type=php -A3 -B3 "throw.*conflicting column"

Length of output: 0

🏁 Script executed:

#!/bin/bash
# Locate the HasUniqueUrls trait file
rg -lP '\btrait\s+HasUniqueUrls' -g '*.php'
# Inspect the first match if found
FILE=$(rg -lP '\btrait\s+HasUniqueUrls' -g '*.php' | head -n1)
echo "Trait file: $FILE"
if [ -n "$FILE" ]; then
  echo "=== File Header ==="
  sed -n '1,200p' "$FILE"
  echo "=== initializeHasUniqueUrls ==="
  rg -n "function initializeHasUniqueUrls" -C3 "$FILE"
  echo "=== throw statements ==="
  rg -n "throw new" -C3 "$FILE"
fi

Length of output: 5092

Clarify exception timing and enforce cleanup

  • Reflect that the conflicting‐column exception is thrown in initializeHasUniqueUrls() during model construction; you can trigger it by instantiating the anonymous model (new class …) without calling create().
  • Move Schema::dropIfExists('models_with_url_column') into a finally block (or use Pest’s afterEach) to guarantee teardown even if the assertion fails.
  • (Optional) Replace the generic \Exception with a dedicated exception class for conflicting columns.
🤖 Prompt for AI Agents 
In tests/HasUniqueUrlsTest.php around lines 175 to 212, the test currently
expects the conflicting-column exception to occur on create(), but the exception
is actually thrown during model construction in initializeHasUniqueUrls();
update the test to instantiate the anonymous model (new class ...) to trigger
the exception immediately instead of calling create(), and wrap the
Schema::dropIfExists('models_with_url_column') call in a finally block (or use
Pest’s afterEach) so the table is always dropped regardless of test outcome;
optionally replace the generic \Exception assertion with the specific
conflicting-column exception class if one exists.


test('13. Check if exception is thrown when model has conflicting urls column', function () {
// Create a table with 'urls' column
Schema::create('models_with_urls_column', function (Blueprint $table) {
$table->increments('id');
$table->string('name');
$table->string('urls');
});

// Create a model class that uses HasUniqueUrls trait
// The exception should be thrown during model instantiation
expect(function () {
$modelClass = new class () extends \Illuminate\Database\Eloquent\Model {
use \Vlados\LaravelUniqueUrls\HasUniqueUrls;

protected $table = 'models_with_urls_column';
protected $guarded = [];
public $timestamps = false;

public function urlHandler(): array
{
return [
'controller' => \Vlados\LaravelUniqueUrls\Tests\TestUrlHandler::class,
'method' => 'view',
'arguments' => [],
];
}

public function urlStrategy($language, $locale): string
{
return \Illuminate\Support\Str::slug($this->name);
}
};

$modelClass::create(['name' => 'test', 'urls' => 'test-urls']);
})->toThrow(Exception::class, "has a conflicting column 'urls'");

Schema::dropIfExists('models_with_urls_column');
});
Comment on lines +214 to +251
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Same concerns as test 12, plus code duplication.

This test has the same structural issues as test 12:

  1. Exception timing mismatch: Comment says instantiation, but test wraps create().
  2. Table cleanup risk: dropIfExists outside the exception-handling closure.
  3. Generic exception type: Consider a more specific exception class.
  4. Code duplication: Tests 12 and 13 are nearly identical except for the column name ('url' vs 'urls') and table name.

Consider extracting a helper function or using Pest's dataset feature to reduce duplication:

test('12. Check if exception is thrown when model has conflicting url column', function () {
    testConflictingColumn('url');
});

test('13. Check if exception is thrown when model has conflicting urls column', function () {
    testConflictingColumn('urls');
});

function testConflictingColumn(string $columnName): void
{
    $tableName = "models_with_{$columnName}_column";
    
    Schema::create($tableName, function (Blueprint $table) use ($columnName) {
        $table->increments('id');
        $table->string('name');
        $table->string($columnName);
    });
    
    try {
        $modelClass = new class ($tableName) extends \Illuminate\Database\Eloquent\Model {
            use \Vlados\LaravelUniqueUrls\HasUniqueUrls;
            
            protected $guarded = [];
            public $timestamps = false;
            
            public function __construct(string $table = null, array $attributes = [])
            {
                if ($table) {
                    $this->table = $table;
                }
                parent::__construct($attributes);
            }
            
            public function urlHandler(): array
            {
                return [
                    'controller' => \Vlados\LaravelUniqueUrls\Tests\TestUrlHandler::class,
                    'method' => 'view',
                    'arguments' => [],
                ];
            }
            
            public function urlStrategy($language, $locale): string
            {
                return \Illuminate\Support\Str::slug($this->name);
            }
        };
        
        $modelClass::create(['name' => 'test', $columnName => "test-{$columnName}"]);
        
        throw new \Exception('Expected exception was not thrown');
    } catch (\Exception $e) {
        expect($e->getMessage())->toContain("has a conflicting column '{$columnName}'");
    } finally {
        Schema::dropIfExists($tableName);
    }
}

Or use Pest's dataset feature:

test('check if exception is thrown when model has conflicting column', function (string $column) {
    // test implementation using $column
})->with(['url', 'urls']);
🤖 Prompt for AI Agents 
tests/HasUniqueUrlsTest.php around lines 214 to 251: the test duplicates test 12
and has three issues — it asserts an exception on create() while the comment
says instantiation, it leaves Schema::dropIfExists outside exception handling
risking leftover state, and it uses a generic Exception and duplicates code; fix
by asserting the exception at the actual failing point (either instantiate the
anonymous model to trigger the trait check or wrap the create() call inside the
expect(...)->toThrow), move table cleanup into a finally block (or use
try/catch/finally) so Schema::dropIfExists always runs, replace Exception::class
with the more specific exception class thrown by the trait, and remove
duplication by extracting a helper function or using Pest datasets to run the
same test for 'url' and 'urls'.