Feat/boost tools#45
Conversation
|
To be merged after #44 |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| $this->addToNav() | ||
| ->registerPermissions() | ||
| ->registerCommands() | ||
| ->configureSSG(); |
There was a problem hiding this comment.
Bug: The installDefaultQueryStrings() call was removed from bootAddon(), breaking automatic installation of default query strings for users with the default 'file' driver configuration.
Severity: MEDIUM
Suggested Fix
Conditionally call installDefaultQueryStrings() within bootAddon() only when the configured driver is 'file'. Alternatively, update the method to be driver-aware, checking if migrations have run for the database driver before proceeding. The new manual installation command for the database driver should be clearly documented.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: src/ServiceProvider.php#L167-L170
Potential issue: The removal of the `installDefaultQueryStrings()` call from the
`bootAddon()` method prevents the automatic installation of default query strings. While
this change was likely intentional to prevent crashes when using the new 'database'
driver, which requires migrations to run first, it creates a functional regression for
users on the default 'file' driver. The code does not handle the installation process
differently based on the configured driver, leading to this incomplete implementation
and breaking expected behavior for the default setup.
Did we get this right? 👍 / 👎 to inform future reviews.
| $this->repository->saveAll('redirects', [ | ||
| ['from' => '/a', 'to' => '/b', 'redirect_type' => '301', 'sites' => ['default']], | ||
| ['from' => '/c', 'to' => '/d', 'redirect_type' => '302', 'sites' => ['default']], | ||
| ]); | ||
| ``` |
There was a problem hiding this comment.
Bug: Calling saveAll() with a regex redirect that lacks an id causes a fatal error in FileRepository due to an attempt to access the undefined id key.
Severity: HIGH
Suggested Fix
In FileRepository::save(), before attempting to use $data['id'] for a regex redirect, check if it exists. If it is not set, generate a unique ID, similar to how DatabaseRepository handles missing IDs. This will prevent the crash when users follow the saveAll() documentation.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: resources/boost/skills/crud-redirects/SKILL.md#L99-L103
Potential issue: When using the `FileRepository` driver, calling `saveAll()` with an
array of redirects that includes a regex-based redirect without an `id` key will cause a
fatal error. The `saveAll()` method iterates and calls `save()` for each item. Inside
`save()`, if `URISupport::isRegex()` identifies the `from` value as a regular
expression, the code attempts to access `$data['id']` to construct the file path. Since
the `id` is not provided, this results in an `Undefined array key "id"` error, crashing
the process. This scenario is likely as the `SKILL.md` documentation for `saveAll()`
explicitly shows an example where `id` keys are omitted.
| // Delete redirect by ID | ||
| $this->repository->delete('redirects', ['id' => '69d767de74f05']); |
There was a problem hiding this comment.
Bug: The FileRepository::delete() method silently fails to delete non-regex redirects when only an id is provided, as it only searches the directory for regex redirects.
Severity: MEDIUM
Suggested Fix
Update the FileRepository::delete() method to correctly handle the deletion of non-regex redirects by id. This may require a lookup mechanism to find the redirect's from value based on its id to construct the correct file path, or searching all possible storage locations. The documentation should also be clarified to reflect the repository's behavior.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: resources/boost/skills/crud-redirects/SKILL.md#L77-L78
Potential issue: When using the `FileRepository` driver, attempting to delete a
non-regex redirect by providing only its `id` will silently fail. The `delete()`
method's logic for handling deletions by `id` exclusively checks the directory
designated for regex redirects (`content/alt-redirect/alt-regex/`). Non-regex redirects
are stored elsewhere, with filenames based on a hash of their `from` value.
Consequently, the file is never found and deleted, leading to orphaned redirect records.
This issue is likely to occur as the `SKILL.md` documentation explicitly provides an
example of deleting a redirect using only its `id`.
1 participant
No description provided.