Conversation
There was a problem hiding this comment.
Pull request overview
Implements issue #147 by attempting to add a fixed /about route and introducing a new CMS page type intended to back that route.
Changes:
- Added an
aboutrule toapp/_config/routes.yml. - Added a new
AboutPagepage type (and a page controller class) inapp/src/AboutPage.php.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| app/src/AboutPage.php | Introduces AboutPage and a page controller class for the About section. |
| app/_config/routes.yml | Adds a Director rule intended to route /about to the new About implementation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| use SilverStripe\CMS\Controllers\ContentController; | ||
|
|
There was a problem hiding this comment.
use SilverStripe\CMS\Controllers\ContentController; is unused in this file (the controller extends PageController). Remove the unused import to avoid dead code / lint warnings.
| use SilverStripe\CMS\Controllers\ContentController; |
| private static $has_one = []; | ||
| } | ||
|
|
||
| class AboutPage_Controller extends PageController |
There was a problem hiding this comment.
The page-type controller class is named AboutPage_Controller, but this project uses the SilverStripe 6 convention *Controller (e.g. PageController, App\Controllers\SitemapController). With CMS 6.x the _Controller suffix is not the expected naming and may not be discovered/used. Rename the class to AboutPageController (and adjust the filename if your autoloading conventions require it).
| class AboutPage_Controller extends PageController | |
| class AboutPageController extends PageController |
| rules: | ||
| 'sitemap.xml': 'App\Controllers\SitemapController' | ||
| 'robots.txt': 'App\Controllers\RobotsController' | ||
| 'about': AboutPage |
There was a problem hiding this comment.
This adds a new Director rule for 'about', but the repo already defines an about route in app/_config/about_routes.yml ('about//$Action/$ID/$OtherID': 'AboutController'), which will also match /about. The duplicate/conflicting rules make routing order-dependent and may prevent this rule from ever being hit. Consider updating/removing the existing about rule (or changing this rule to a different URL pattern) so there is a single source of truth for the /about route.
| 'about': AboutPage |
| rules: | ||
| 'sitemap.xml': 'App\Controllers\SitemapController' | ||
| 'robots.txt': 'App\Controllers\RobotsController' | ||
| 'about': AboutPage |
There was a problem hiding this comment.
Director.rules expects a controller class name (or mapping to a controller action), but AboutPage is a Page/SiteTree subclass, not a Controller. As written, this route is likely to fail or never dispatch as intended. Route to an actual controller (e.g. an AboutPageController/AboutController) or implement a controller that loads/renders the AboutPage record.
| 'about': AboutPage | |
| 'about': 'App\Controllers\AboutController' |
|
|
||
| private static $has_one = []; |
There was a problem hiding this comment.
private static $has_one = []; is redundant here (it matches the empty definition already present on Page). Consider removing it unless you plan to add relations, to reduce noise in the class definition.
| private static $has_one = []; |
Implements #147
Changes
about: AboutPageroute toapp/_config/routes.ymlapp/src/AboutPage.phpwith AboutPage model and controllerFixes #147