Repository navigation
Fix parsewidget: Container::set TypeError and deleted items - #463
Merged
Merged
Conversation
- parseWidget called Container::set('parseWidget', true). Symfony only
accepts objects there, so any {{ parsewidget() }} in a template threw
a TypeError and the page returned a 500. Nothing reads that flag, so
it is removed.
- getForId built a widget without data when the item behind the id no
longer exists (e.g. a deleted content block), which crashed outside
the try. It now throws a Frontend exception, and parseWidget calls it
inside the try: debug shows the error, production renders nothing.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Reviewer's guide (collapsed on small PRs)Reviewer's GuideFixes parseWidget() failures by removing invalid container state mutations and handling deleted ID-backed widgets inside the caught execution path, allowing production to render no content and debug mode to report the missing widget. Sequence diagram for parseWidget missing-item handlingsequenceDiagram
participant Template
participant TemplateModifiers
participant Widget
participant Database
participant ErrorHandler
Template->>TemplateModifiers: parseWidget(module, action, id)
TemplateModifiers->>Widget: getForId(kernel, module, action, id)
Widget->>Database: getVar(query, parameters)
Database-->>Widget: null or false
Widget-->>TemplateModifiers: FrontendException
TemplateModifiers->>ErrorHandler: catch Exception
ErrorHandler-->>TemplateModifiers: debug error or empty output
TemplateModifiers-->>Template: string
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/Frontend/Core/Engine/Block/Widget.php" line_range="255-256" />
<code_context>
- );
+ // A lookup by id matches on the data column, so no data means the item (and its widget) no longer exists
+ $data = $kernel->getContainer()->get('database')->getVar($query, $parameters);
+ if (is_numeric($id) && ($data === null || $data === false)) {
+ throw new FrontendException('No ' . $module . '::' . $action . ' widget found for id ' . $id . '.');
+ }
+
</code_context>
<issue_to_address>
**Orphaned widgets still crash pages**
When a deleted item still has a matching widget row in `modules_extras`, `getForId()` treats any matching `modules_extras` row as a valid item, so an orphaned row lets the widget execute with a deleted item; the widget can then fail before `parseWidget()`'s `catch (Exception)` handles it, and the page returns a 500.
Validate that the referenced item exists before executing the widget, or handle failures from widget execution that are not `Exception`s.
</issue_to_address>
### Comment 2
<location path="src/Frontend/Core/Engine/TemplateModifiers.php" line_range="454" />
<code_context>
+ $action,
+ $id
+ );
$extra->execute();
- $content = $extra->getContent();
- FrontendModel::getContainer()->set('parseWidget', null);
</code_context>
<issue_to_address>
**Concurrent deletion still returns 500**
When a deletion removes the content block after `getForId()` reads its widget row but before the widget executes, `Detail::execute()` looks up the content block after the widget row was read, and throws `ContentBlockNotFound` if deletion has since removed it. The catch resolves to the frontend engine exception, not this global exception, so it escapes `parseWidget()` and the page returns 500.
Catch global `\Exception` in `parseWidget()` (or explicitly catch `ContentBlockNotFound`) so this deletion race follows the normal production fallback.
</issue_to_address>Sourcery assessment
Approval pending. 2 findings to address first.
Blocking findings: src/Frontend/Core/Engine/Block/Widget.php:256, src/Frontend/Core/Engine/TemplateModifiers.php:454
tijsverkoyen
approved these changes
Nov 2, 2026
jonasdekeukelaere
approved these changes
Nov 2, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
{{ parsewidget() }}is broken: any template that uses it returns a 500.parseWidget()callsContainer::set('parseWidget', true). Symfony's container only accepts objects there, so it throwsContainer::set(): Argument #2 ($service) must be of type ?object, true given. Nothing in core reads that flag, so the twoset()calls are removed.modules_extrasrow is removed as well),getForId()built a widget without data, and the widget then failed on a null template path. That happened outside thetry, so it always ended in a 500.getForId()now throws aFrontend\Core\Engine\Exceptionwhen a lookup by id finds nothing, andparseWidget()calls it inside thetry. Debug mode shows the error, production renders nothing. Lookups without an id (widgets with NULL data) behave as before.Found on sumocoders-be-2026, which renders a content block on every page with
{{ parsewidget('ContentBlocks', 'Detail', 8) }}.Test plan
phpcs.xml.dist) andphp -lon both files{{ parsewidget('ContentBlocks', 'Detail', <id>) }}on a fresh install🤖 Generated with Claude Code
Summary by Sourcery
Prevent
parseWidget()from crashing templates when rendering valid or deleted widgets.Bug Fixes:
parseWidget()failures caused by invalid container state handling.Enhancements: