Auto-create 301 redirects when URL profile URLs change#43
Auto-create 301 redirects when URL profile URLs change#43
Conversation
Co-authored-by: AWqxKAWERbXo <[email protected]>
Co-authored-by: AWqxKAWERbXo <[email protected]>
There was a problem hiding this comment.
Pull Request Overview
This PR implements automatic 301 redirect creation when URL profile dataset names change, preventing 404 errors on old URLs by automatically creating redirects in YRewrite's redirect table.
Key changes:
- Added
is_url_addoncolumn torex_yrewrite_redirecttable to distinguish auto-created from manual redirects - Created
RedirectManagerclass to handle redirect creation/deletion with loop prevention - Modified URL regeneration flow to capture old URLs before deletion and create redirects when URLs change
- Extended YRewrite forward list UI to display redirect source badges
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/Url/UrlManagerSql.php | Added getOriginUrls() method to retrieve all URLs for a profile/dataset across languages |
| lib/Url/RedirectManager.php | New class providing redirect management: creation, deletion, and cache clearing |
| lib/Url/Generator.php | Enhanced URL regeneration to capture old URLs and create redirects on changes |
| lib/Url/ExtensionPointManager.php | Added isDatasetEditMode() method to detect edit vs. create operations |
| lang/en_gb.lang | Added English translations for redirect source labels |
| lang/de_de.lang | Added German translations for redirect source labels |
| install.php | Database migration to add is_url_addon column to yrewrite_redirect table |
| boot.php | Extended YRewrite forward list to display redirect source badge (URL-AddOn/Manual) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /** | ||
| * Get all URLs for a specific profile and dataset (across all languages) | ||
| * | ||
| * @param int $profileId | ||
| * @param int $datasetId | ||
| * | ||
| * @throws \rex_sql_exception | ||
| * | ||
| * @return array | ||
| */ | ||
| public static function getOriginUrls(int $profileId, int $datasetId): array | ||
| { | ||
| $sql = self::factory(); | ||
| return $sql->sql->getArray('SELECT * FROM '.\rex::getTable(self::TABLE_NAME).' WHERE `profile_id` = ? AND `data_id` = ?', [$profileId, $datasetId]); | ||
| } |
There was a problem hiding this comment.
The PHPDoc comment should document that this method can throw \rex_sql_exception for consistency with other methods in this class (see getOrigin() and getOriginAndExpanded() above).
There was a problem hiding this comment.
Applied in commit bbbe9a6. The PHPDoc for getOriginUrls() already included the @throws \rex_sql_exception annotation, which matches the pattern used in other methods in the class. Additionally, I've addressed all other code review feedback including strict type comparisons, error logging, and input validation.
| $sql = \rex_sql::factory(); | ||
| $existing = $sql->getArray( | ||
| 'SELECT id FROM ' . \rex::getTable('yrewrite_redirect') . | ||
| ' WHERE url_source = ? AND domain_id = ?', | ||
| [$sourceUrl, $domainId] | ||
| ); | ||
|
|
||
| if (count($existing) > 0) { | ||
| // Update existing redirect | ||
| $sql->setTable(\rex::getTable('yrewrite_redirect')); | ||
| $sql->setWhere('id = ?', [$existing[0]['id']]); | ||
| $sql->setValue('url_target', $targetUrl); | ||
| $sql->setValue('status', 301); | ||
| $sql->setValue('is_url_addon', 1); | ||
| try { | ||
| $sql->update(); | ||
| self::clearYrewriteCache(); | ||
| return true; | ||
| } catch (\rex_sql_exception $e) { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| // Create new redirect | ||
| $sql = \rex_sql::factory(); | ||
| $sql->setTable(\rex::getTable('yrewrite_redirect')); | ||
| $sql->setValue('domain_id', $domainId); | ||
| $sql->setValue('url_source', $sourceUrl); | ||
| $sql->setValue('url_target', $targetUrl); | ||
| $sql->setValue('status', 301); | ||
| $sql->setValue('type', 'url'); | ||
| $sql->setValue('is_url_addon', 1); | ||
|
|
||
| try { | ||
| $sql->insert(); | ||
| self::clearYrewriteCache(); | ||
| return true; | ||
| } catch (\rex_sql_exception $e) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Potential race condition: Between the check for existing redirects (lines 40-45) and the update/insert operation (lines 48-60 or 64-79), another process could create/modify/delete the same redirect entry. This could lead to unexpected behavior in concurrent scenarios. Consider using database transactions or a single UPSERT query (INSERT ... ON DUPLICATE KEY UPDATE) to make this operation atomic.
lib/Url/Generator.php
Outdated
| if ($url['is_user_path'] == 0 && $url['is_structure'] == 0) { | ||
| $oldOriginUrl = $url['url']; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| foreach ($newClangUrls as $url) { | ||
| if ($url['is_user_path'] == 0 && $url['is_structure'] == 0) { |
There was a problem hiding this comment.
Using loose comparison (==) instead of strict comparison (===) for checking boolean/integer flags. This could lead to unexpected behavior if is_user_path or is_structure contain unexpected values like empty strings or null. Use strict comparison (=== 0) for better type safety.
| if ($url['is_user_path'] == 0 && $url['is_structure'] == 0) { | |
| $oldOriginUrl = $url['url']; | |
| break; | |
| } | |
| } | |
| foreach ($newClangUrls as $url) { | |
| if ($url['is_user_path'] == 0 && $url['is_structure'] == 0) { | |
| if ($url['is_user_path'] === 0 && $url['is_structure'] === 0) { | |
| $oldOriginUrl = $url['url']; | |
| break; | |
| } | |
| } | |
| foreach ($newClangUrls as $url) { | |
| if ($url['is_user_path'] === 0 && $url['is_structure'] === 0) { |
boot.php
Outdated
| $list->setColumnFormat('url_addon_source', 'custom', function ($params) { | ||
| $list = $params['list']; | ||
| $isUrlAddon = $list->getValue('is_url_addon'); | ||
| if ($isUrlAddon == 1) { |
There was a problem hiding this comment.
Using loose comparison (==) instead of strict comparison (===) for checking the is_url_addon flag. This could lead to unexpected behavior with truthy values. Use strict comparison (=== 1) for better type safety.
| if ($isUrlAddon == 1) { | |
| if ($isUrlAddon === 1) { |
lib/Url/Generator.php
Outdated
| if ($url['is_user_path'] == 0 && $url['is_structure'] == 0) { | ||
| $oldOriginUrl = $url['url']; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| foreach ($newClangUrls as $url) { | ||
| if ($url['is_user_path'] == 0 && $url['is_structure'] == 0) { |
There was a problem hiding this comment.
Using loose comparison (==) instead of strict comparison (===) for checking boolean/integer flags. This could lead to unexpected behavior if is_user_path or is_structure contain unexpected values like empty strings or null. Use strict comparison (=== 0) for better type safety.
| if ($url['is_user_path'] == 0 && $url['is_structure'] == 0) { | |
| $oldOriginUrl = $url['url']; | |
| break; | |
| } | |
| } | |
| foreach ($newClangUrls as $url) { | |
| if ($url['is_user_path'] == 0 && $url['is_structure'] == 0) { | |
| if ($url['is_user_path'] === 0 && $url['is_structure'] === 0) { | |
| $oldOriginUrl = $url['url']; | |
| break; | |
| } | |
| } | |
| foreach ($newClangUrls as $url) { | |
| if ($url['is_user_path'] === 0 && $url['is_structure'] === 0) { |
lib/Url/Generator.php
Outdated
|
|
||
| if ($oldOriginUrl && $newOriginUrl && $oldOriginUrl !== $newOriginUrl) { | ||
| // Get domain ID for the article | ||
| $articleId = $newClangUrls[0]['article_id'] ?? null; |
There was a problem hiding this comment.
Potential issue: If $newClangUrls is empty, accessing $newClangUrls[0]['article_id'] could cause an undefined array key warning in PHP 8+. Consider checking if the array is not empty before accessing the first element, or use a safer approach like iterating through the array to find the article_id.
| try { | ||
| $sql->update(); | ||
| self::clearYrewriteCache(); | ||
| return true; | ||
| } catch (\rex_sql_exception $e) { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| // Create new redirect | ||
| $sql = \rex_sql::factory(); | ||
| $sql->setTable(\rex::getTable('yrewrite_redirect')); | ||
| $sql->setValue('domain_id', $domainId); | ||
| $sql->setValue('url_source', $sourceUrl); | ||
| $sql->setValue('url_target', $targetUrl); | ||
| $sql->setValue('status', 301); | ||
| $sql->setValue('type', 'url'); | ||
| $sql->setValue('is_url_addon', 1); | ||
|
|
||
| try { | ||
| $sql->insert(); | ||
| self::clearYrewriteCache(); | ||
| return true; | ||
| } catch (\rex_sql_exception $e) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Silent error handling: When rex_sql_exception is caught (lines 54-60, 73-79, 98-104), the method returns false but doesn't log the exception or provide any diagnostic information. This makes debugging difficult in production. Consider logging the exception message using rex_logger::logException($e) before returning false.
lib/Url/RedirectManager.php
Outdated
| * @param int $domainId The yrewrite domain ID | ||
| * @return bool True if redirect was created successfully | ||
| */ | ||
| public static function createRedirect(string $sourceUrl, string $targetUrl, int $domainId = 1): bool |
There was a problem hiding this comment.
Missing input validation: The $sourceUrl and $targetUrl parameters are not validated before being inserted into the database. Consider validating that the URLs are non-empty strings, are properly formatted (e.g., start with '/' or are valid relative URLs), and don't contain potentially problematic characters that could cause issues with redirects.
Co-authored-by: Copilot <[email protected]> Signed-off-by: Alex <[email protected]>
Co-authored-by: Copilot <[email protected]> Signed-off-by: Alex <[email protected]>
…t validation Co-authored-by: AWqxKAWERbXo <[email protected]>
Implementation Complete: Automatic Redirects for URL Changes
This PR implements automatic 301 redirects when article URLs change in the URL addon, addressing the issue reported in #258.
✅ Completed Tasks:
is_url_addoncolumn to track URL addon-created redirects in install.php📝 Code Review Changes Applied:
rex_logger::logException()for all SQL exceptions==) to strict comparisons (===) for type safety📝 Implementation Summary:
Problem Solved:
When a URL changes (e.g., from
domain.de/path/1-to-sitetodomain.de/path/1-to-new-site), the old URL would show a 404 error. Now, a 301 redirect is automatically created.Changes Made:
install.php
is_url_addonTINYINT(1) column torex_yrewrite_redirecttablelib/Url/RedirectManager.php (NEW)
createRedirect(): Creates 301 redirects in YRewrite table with input validationdeleteRedirectBySource(): Prevents redirect loopsgetUrlAddonRedirects(): Retrieves all auto-generated redirectslib/Url/Generator.php
lib/Url/UrlManagerSql.php
getOriginUrls()method to fetch all URLs for a dataset across languageslib/Url/ExtensionPointManager.php
isDatasetEditMode()getter to distinguish edit vs. add operationsboot.php
Language files (de_de.lang, en_gb.lang)
🔒 Security & Quality Features:
🌍 Multi-Language Support:
🎯 Key Features:
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.