545 SpoonFilter #425
Open
545 SpoonFilter #425
Conversation
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
tijsverkoyen
force-pushed
the
545-spoonfilter
branch
from
69b622b to
1ec9806
Compare
Reviewer's GuideRefactors a large portion of backend and frontend string handling, validation, and URL/HTML utilities away from deprecated SpoonFilter helpers toward Symfony String, native PHP, and Symfony Validator; normalizes template/field naming conventions; tightens XSS/encoding behaviour; fixes a redirect helper typo; removes obsolete site_domains configuration; and narrows the CI workflow push trigger to master only. Sequence diagram for backend redirectToFirstAvailableLink flowsequenceDiagram
actor User
participant Browser
participant BackendKernel
participant Url as Backend_Core_Engine_Url
participant Auth as Backend_Core_Engine_Authentication
participant NavCache as cache_backend_navigation
User->>Browser: Request backend URL
Browser->>BackendKernel: HTTP request
BackendKernel->>Url: Route request
Url->>Url: getModuleFromRequest()
Url->>Url: getActionFromRequest(module, language)
Url->>Auth: isAllowedModule(module)
alt module is Dashboard and not allowed
Url->>NavCache: get()
NavCache-->>Url: navigation tree
Url->>Url: redirectToFirstAvailableLink(language, navigation)
loop navigation items
Url->>Auth: isAllowedModule(candidateModule)
Url->>Auth: isAllowedAction(candidateAction, candidateModule)
alt first allowed combination found
Url-->>BackendKernel: RedirectResponse to candidateModule/candidateAction
BackendKernel-->>Browser: 302 Redirect
Browser-->>User: Navigate to first available link
else child navigation exists
Url->>Url: redirectToFirstAvailableLink(language, children)
end
end
else module is allowed
Url-->>BackendKernel: Continue normal controller dispatch
BackendKernel-->>Browser: HTTP response
Browser-->>User: Render backend page
end
Updated class diagram for backend and frontend URL handlingclassDiagram
class Backend_Core_Engine_Url {
-string module
-string action
+__construct(KernelInterface kernel)
+getModuleFromRequest() string
+getActionFromRequest(string module, string language) string
+processRegularRequest(string module, string action, string language) void
-redirectToFirstAvailableLink(string language, array navigation) void
+setAction(string action, string module) void
+setModule(string module) void
}
class Backend_Core_Engine_Authentication {
+isAllowedModule(string module) bool
+isAllowedAction(string action, string module) bool
}
class Backend_Core_Engine_Navigation {
+getNavigationItemForCurrentlyAuthenticatedUser(array navigation) array
}
class Frontend_Core_Engine_Url {
+array parameters
+getParameter(int index, string type, mixed defaultValue) mixed
+hasParameter(int index) bool
}
Backend_Core_Engine_Url --> Backend_Core_Engine_Authentication : uses
Backend_Core_Engine_Url --> Backend_Core_Engine_Navigation : uses
Backend_Core_Engine_Url ..> Frontend_Core_Engine_Url : similar_casting_conventions
Updated class diagram for FormBuilder validation and field handlingclassDiagram
class Frontend_Modules_FormBuilder_Widgets_Form {
-Form form
-array fields
+createAction() string
+validateForm() void
}
class Backend_Modules_FormBuilder_Actions_Add {
-BackendForm form
+execute() void
+loadForm() void
+validateForm() void
}
class Backend_Modules_FormBuilder_Actions_Edit {
-BackendForm form
+execute() void
+loadForm() void
+parseFields() void
+validateForm() void
}
class Symfony_Component_Validator_ValidatorInterface {
+validate(mixed value, Constraint constraint, array groups) ConstraintViolationListInterface
}
class Symfony_Component_Validator_Constraints_Regex {
+string pattern
+string message
+__construct(array options)
}
Frontend_Modules_FormBuilder_Widgets_Form --> Symfony_Component_Validator_ValidatorInterface : uses_for_time_validation
Frontend_Modules_FormBuilder_Widgets_Form --> Symfony_Component_Validator_Constraints_Regex : time_rule
Backend_Modules_FormBuilder_Actions_Add --> Symfony_Component_Validator_ValidatorInterface : uses_for_identifier
Backend_Modules_FormBuilder_Actions_Add --> Symfony_Component_Validator_Constraints_Regex : identifier_pattern
Backend_Modules_FormBuilder_Actions_Edit --> Symfony_Component_Validator_ValidatorInterface : uses_for_identifier
Backend_Modules_FormBuilder_Actions_Edit --> Symfony_Component_Validator_Constraints_Regex : identifier_pattern
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
Backend\Modules\FormBuilder\Engine\Model::getLocalethe switch fromSpoonFilter::toCamelCase($name)tos($name)->title()->toString()changes the lookup key semantics (title-casing vs camel-casing, different handling of underscores), which is likely to break existing locale key resolution; consider keeping camelCase behavior here or adding an explicit helper that mimics the original transformation. - In
Backend\Modules\Blog\Actions\Categories::loadDataGridthe header label constructions(BL::lbl('Amount')->title()->toString())appears to calltitle()on the string returned byBL::lbl, which will fail; this should likely bes(BL::lbl('Amount'))->title()->toString()as done elsewhere. - Across the changes, the use of
s(...)->title()sometimes omits->toString()and relies on implicit casting during concatenation while other places call->toString()explicitly; it would be good to standardize on one approach (preferably always casting to string) to keep behavior and readability consistent.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `Backend\Modules\FormBuilder\Engine\Model::getLocale` the switch from `SpoonFilter::toCamelCase($name)` to `s($name)->title()->toString()` changes the lookup key semantics (title-casing vs camel-casing, different handling of underscores), which is likely to break existing locale key resolution; consider keeping camelCase behavior here or adding an explicit helper that mimics the original transformation.
- In `Backend\Modules\Blog\Actions\Categories::loadDataGrid` the header label construction `s(BL::lbl('Amount')->title()->toString())` appears to call `title()` on the string returned by `BL::lbl`, which will fail; this should likely be `s(BL::lbl('Amount'))->title()->toString()` as done elsewhere.
- Across the changes, the use of `s(...)->title()` sometimes omits `->toString()` and relies on implicit casting during concatenation while other places call `->toString()` explicitly; it would be good to standardize on one approach (preferably always casting to string) to keep behavior and readability consistent.
## Individual Comments
### Comment 1
<location> `src/Backend/Modules/Blog/Actions/Categories.php:38` </location>
<code_context>
// set headers
$this->dataGrid->setHeaderLabels([
- 'num_items' => \SpoonFilter::ucfirst(BL::lbl('Amount')),
+ 'num_items' => s(BL::lbl('Amount')->title()->toString()),
]);
</code_context>
<issue_to_address>
**issue (bug_risk):** The chained `->title()->toString()` is applied on the result of `BL::lbl()` instead of on the Symfony string, which will cause a runtime error.
`BL::lbl('Amount')` returns a string, but `->title()->toString()` is called on that string before it is passed to `s()`, which will cause `Call to a member function title() on string` at runtime.
It should be:
```php
'num_items' => s(BL::lbl('Amount'))->title()->toString(),
```
so `title()` is applied to the `UnicodeString` returned by `s()`, not to the raw string.
</issue_to_address>
### Comment 2
<location> `src/Backend/Modules/FormBuilder/Engine/Model.php:478-480` </location>
<code_context>
- $name = \SpoonFilter::toCamelCase($name);
- $class = \SpoonFilter::ucfirst($application) . '\Core\Language\Language';
- $function = 'get' . \SpoonFilter::ucfirst($type);
+ $name = s($name)->title()->toString();
+ $class = s($application)->title()->toString() . '\Core\Language\Language';
+ $function = 'get' . s($type)->title()->toString();
// execute and return value
</code_context>
<issue_to_address>
**issue (bug_risk):** Changing from `toCamelCase`/`ucfirst` to `title()` in `getLocale` alters lookup keys and may break existing translations.
Using `s(...)->title()` for `$name` changes its format compared to `�SpoonFilter::toCamelCase($name)`, especially for multi-word or underscore-separated keys, so existing camelCase locale keys may no longer match and translations could stop resolving.
To keep behavior while replacing `ucfirst`, you can leave `$name` as camelCase and only change the parts that originally used `ucfirst`:
```php
$name = \SpoonFilter::toCamelCase($name);
$class = s($application)->title()->toString() . '\\Core\\Language\\Language';
$function = 'get' . s($type)->title()->toString();
```
This keeps `getLocale()` backward compatible.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
tijsverkoyen
changed the base branch from
534-github-actions-in-line-with-gitlab
to
master
jonasdekeukelaere
approved these changes
Jan 28, 2026
…g implementation Warning: the new implementation will not match urls without a protocol. I don't think this usefull looking from the usages.
…ecialchars_decode The default contains ENT_QUOTES, so no need to use a custom method.
…hars * Charset parameter was never used * default charset in PHP is UTF8, which matches the default Spoon charset
…y_decode * Charset parameter was never used * default charset in PHP is UTF8, which matches the default Spoon charset
There was a problem hiding this comment.
Hey - I've found 5 issues, and left some high level feedback:
- There are a few obvious syntax issues introduced when replacing SpoonFilter helpers, e.g.
s($url->getModule() . ' ' . $url->getAction())->camel->toString()inBackend/Core/Engine/Form.php(missing()oncamel()) ands(BL::lbl('Amount')->title()->toString())inBlog/Actions/Categories.php(calling->title()on the result ofBL::lbl()), which need to be fixed before this can run. - The new manual link detection in
Frontend/Core/Engine/TemplateModifiers::cleanupPlainText()andBackend/Core/Engine/DataGridFunctions::cleanupPlainText()(simple~(https?://[^\s]+)~regex) is much less feature-complete thanSpoonFilter::replaceURLsWithAnchorsand may mishandle edge cases (trailing punctuation, existing<a>tags, non-HTTP links, HTML contexts); consider either reusing a more robust utility or tightening the regex/logic to avoid regressions. - The repeated construction of label keys with
s(...)->replace('_', ' ')->camel()->title()is duplicated in many places (forms, widgets, datagrids, URL/module/action handling); pulling this into a small shared helper (e.g.toTemplateKey()/toLabelKey()) would reduce the chance of subtle inconsistencies and make future changes to the naming convention much easier.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- There are a few obvious syntax issues introduced when replacing SpoonFilter helpers, e.g. `s($url->getModule() . ' ' . $url->getAction())->camel->toString()` in `Backend/Core/Engine/Form.php` (missing `()` on `camel()`) and `s(BL::lbl('Amount')->title()->toString())` in `Blog/Actions/Categories.php` (calling `->title()` on the result of `BL::lbl()`), which need to be fixed before this can run.
- The new manual link detection in `Frontend/Core/Engine/TemplateModifiers::cleanupPlainText()` and `Backend/Core/Engine/DataGridFunctions::cleanupPlainText()` (simple `~(https?://[^\s]+)~` regex) is much less feature-complete than `SpoonFilter::replaceURLsWithAnchors` and may mishandle edge cases (trailing punctuation, existing `<a>` tags, non-HTTP links, HTML contexts); consider either reusing a more robust utility or tightening the regex/logic to avoid regressions.
- The repeated construction of label keys with `s(...)->replace('_', ' ')->camel()->title()` is duplicated in many places (forms, widgets, datagrids, URL/module/action handling); pulling this into a small shared helper (e.g. `toTemplateKey()` / `toLabelKey()`) would reduce the chance of subtle inconsistencies and make future changes to the naming convention much easier.
## Individual Comments
### Comment 1
<location> `src/Backend/Core/Engine/Form.php:35` </location>
<code_context>
parent::__construct(
- $name ?? \SpoonFilter::toCamelCase($url->getModule() . '_' . $url->getAction(), '_', true),
+ $name ?? s($url->getModule() . ' ' . $url->getAction())->camel->toString(),
$action,
$method ?? 'post',
</code_context>
<issue_to_address>
**issue (bug_risk):** String helper call is missing parentheses, causing a runtime error.
`s($url->getModule() . ' ' . $url->getAction())->camel->toString()` accesses `camel` as a property, which will throw at runtime. Call it as a method instead: `s(...)->camel()->toString()`.
</issue_to_address>
### Comment 2
<location> `src/Backend/Modules/Blog/Actions/Categories.php:37-38` </location>
<code_context>
// set headers
$this->dataGrid->setHeaderLabels([
- 'num_items' => \SpoonFilter::ucfirst(BL::lbl('Amount')),
+ 'num_items' => s(BL::lbl('Amount')->title()->toString()),
]);
</code_context>
<issue_to_address>
**issue (bug_risk):** Method chaining on `BL::lbl()` is mis-parenthesized and will not compile.
`BL::lbl('Amount')` returns a string, so calling `->title()` on it is invalid. You probably meant to chain on the `Stringy` wrapper instead:
```php
'num_items' => s(BL::lbl('Amount'))->title()->toString(),
```
</issue_to_address>
### Comment 3
<location> `src/Frontend/Core/Engine/TemplateModifiers.php:475-476` </location>
<code_context>
{
- // detect links
- $var = SpoonFilter::replaceURLsWithAnchors($var);
+ // replace links with a tags
+ $var = s($var)->replaceMatches(
+ '~(https?://[^\s]+)~',
</code_context>
<issue_to_address>
**issue (bug_risk):** The new URL-to-anchor replacement is much more naive than `replaceURLsWithAnchors` and may mis-handle HTML and punctuation.
Previously `SpoonFilter::replaceURLsWithAnchors()` encapsulated safer link detection and anchor generation; the new `~(https?://[^\s]+)~` on the raw string can:
- Match inside existing HTML (attributes, already-linked URLs), breaking markup.
- Swallow trailing punctuation into the URL.
- Create nested/malformed anchors in user-supplied HTML.
Consider reusing the existing helper or, if you must inline this, tightening the regex and/or applying it only to plain text (e.g. after stripping tags) to avoid HTML corruption and XSS risk.
</issue_to_address>
### Comment 4
<location> `src/Backend/Core/Engine/DataGridFunctions.php:26-27` </location>
<code_context>
{
- // detect links
- $var = SpoonFilter::replaceURLsWithAnchors($var);
+ // replace links with a tags
+ $var = s($var)->replaceMatches(
+ '~(https?://[^\s]+)~',
+ '<a href="$1" rel="nofollow noopener">$1</a>'
</code_context>
<issue_to_address>
**issue (bug_risk):** Same naive link replacement pattern here, with an explicit `rel` that may not be desired in all contexts.
`cleanupPlainText()` now uses a simple URL regex and always wraps matches as `<a href="$1" rel="nofollow noopener">`. This risks:
- Breaking existing markup.
- Including trailing punctuation/entities in the URL.
- Forcing a specific `rel` that may not suit all callers.
Consider reusing the dedicated linkification helper, or making the regex and `rel` handling more robust/configurable.
</issue_to_address>
### Comment 5
<location> `src/Backend/Modules/FormBuilder/Ajax/SaveField.php:231-239` </location>
<code_context>
if ($type !== 'paragraph') {
if ($values !== '') {
- $values = \SpoonFilter::htmlspecialchars($values);
+ $values = htmlspecialchars($values);
}
if ($defaultValues !== '') {
- $defaultValues = \SpoonFilter::htmlspecialchars($defaultValues);
+ $defaultValues = htmlspecialchars($defaultValues);
}
}
</code_context>
<issue_to_address>
**🚨 suggestion (security):** Switching from SpoonFilter HTML escaping to bare `htmlspecialchars()` changes encoding flags and charset handling.
`\SpoonFilter::htmlspecialchars()` previously encoded both single and double quotes and respected the app charset. The raw `htmlspecialchars()` calls now rely on defaults (`ENT_COMPAT`, default encoding), so single quotes are no longer escaped and behavior may change if the default encoding isn’t UTF‑8—especially for HTML attributes. To retain prior behavior, explicitly pass flags and encoding, e.g. `htmlspecialchars($values, ENT_QUOTES, 'UTF-8')`.
```suggestion
// htmlspecialchars except for paragraphs
if ($type !== 'paragraph') {
if ($values !== '') {
$values = htmlspecialchars($values, ENT_QUOTES, 'UTF-8');
}
if ($defaultValues !== '') {
$defaultValues = htmlspecialchars($defaultValues, ENT_QUOTES, 'UTF-8');
}
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull request overview
This pull request refactors the codebase to replace legacy SpoonFilter utility methods with modern alternatives: Symfony String component for string manipulation, native PHP functions for HTML encoding/decoding and filtering, and Symfony Validator for validation. Additionally, it removes the deprecated site_domains setting from Core and fixes a typo in the backend navigation helper.
Changes:
- Replace SpoonFilter string methods (ucfirst, toCamelCase) with Symfony String s()->title()/camel() across 100+ files
- Replace SpoonFilter HTML encoding/decoding methods with native PHP htmlspecialchars, htmlentities, and their decode equivalents
- Replace SpoonFilter validation methods (isURL, isValidAgainstRegexp, isNumeric, isInteger) with Symfony Validator constraints and native PHP filter functions
- Remove deprecated Core
site_domainssetting from installer, settings form, test data, and validation - Fix typo in Backend URL helper:
redirectToFistAvailableLink→redirectToFirstAvailableLink
Reviewed changes
Copilot reviewed 112 out of 112 changed files in this pull request and generated 38 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/data/test_db.sql | Remove site_domains test fixture |
| src/Frontend/** | Replace SpoonFilter with Symfony String/native PHP in Search, Tags, Profiles, Mailmotor, FormBuilder, Blog modules and core classes |
| src/Common/** | Replace SpoonFilter in Twig modifiers, core model/form, and meta repository |
| src/Backend/Modules/Settings/** | Remove site_domains setting form fields and validation |
| src/Backend/Modules/** | Replace SpoonFilter across all backend modules (Users, Tags, Pages, MediaLibrary, Locale, Groups, FormBuilder, Extensions, Blog, etc.) |
| src/Backend/Core/** | Replace SpoonFilter in core engine classes, language, authentication, and installers |
| .github/workflows/ci.yml | Restrict CI push trigger to master branch only |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Member
Author
Fixed.
Correct, won't bother.
Myeah. True. Will ask the opinion of the other devs. |
tijsverkoyen
requested review from
absumo,
ilaria-orlando and
jonasdekeukelaere
jonasdekeukelaere
approved these changes
Jan 29, 2026
ilaria-orlando
approved these changes
Jan 30, 2026
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 by Sourcery
Replace deprecated SpoonFilter usage across backend and frontend with Symfony String utilities, native PHP functions, and Symfony Validator; clean up related URL, HTML, and regex handling; and remove obsolete site domain configuration handling.
Bug Fixes:
Enhancements:
CI:
Tests: