Add blade support for cms/backend partials #1431
Conversation
mjauvin
added
the
enhancement
WalkthroughThis change adds Blade template engine support to the CMS partial rendering system. The Controller class now detects and renders Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
modules/cms/classes/Controller.php(2 hunks)modules/cms/classes/Partial.php(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
modules/cms/classes/Controller.php (1)
modules/cms/twig/Loader.php (1)
setObject(34-37)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: windows-latest / PHP 8.4
- GitHub Check: ubuntu-latest / PHP 8.2
- GitHub Check: windows-latest / PHP 8.3
- GitHub Check: windows-latest / PHP 8.2
- GitHub Check: ubuntu-latest / PHP 8.1
- GitHub Check: ubuntu-latest / PHP 8.3
- GitHub Check: windows-latest / PHP 8.1
- GitHub Check: ubuntu-latest / PHP 8.4
- GitHub Check: windows-latest / JavaScript
🔇 Additional comments (2)
modules/cms/classes/Controller.php (1)
28-28: LGTM! Blade import is necessary.The import is required for the Blade rendering functionality added below.
modules/cms/classes/Partial.php (1)
16-16: LGTM! Blade extension support declared.Adding 'blade' to the allowed extensions enables the CMS to recognize and process Blade-formatted partials.
| if (str_ends_with($partial->fileName, '.blade.php')) { | ||
| $partialContent = Blade::render($partial->content, $this->vars); | ||
| } | ||
| else { | ||
| $this->getLoader()->setObject($partial); | ||
| $template = $this->getTwig()->load($partial->getFilePath()); | ||
| $partialContent = $template->render(array_merge($parameters, $this->vars)); | ||
| } |
There was a problem hiding this comment.
Critical: Blade rendering path missing component variables.
The Blade rendering at line 1070 only passes $this->vars, while the Twig rendering at line 1075 passes array_merge($parameters, $this->vars). This inconsistency is problematic because:
- Lines 1022-1063 add component objects to the
$parametersarray (see line 1045:$parameters[$alias] = ... = $componentObj) - These components are only included in the Twig rendering but not in the Blade rendering
- Blade partials with components won't have access to those component instances
🔎 Proposed fix
if (str_ends_with($partial->fileName, '.blade.php')) {
- $partialContent = Blade::render($partial->content, $this->vars);
+ $partialContent = Blade::render($partial->content, array_merge($parameters, $this->vars));
}
else {
$this->getLoader()->setObject($partial);
$template = $this->getTwig()->load($partial->getFilePath());
$partialContent = $template->render(array_merge($parameters, $this->vars));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (str_ends_with($partial->fileName, '.blade.php')) { | |
| $partialContent = Blade::render($partial->content, $this->vars); | |
| } | |
| else { | |
| $this->getLoader()->setObject($partial); | |
| $template = $this->getTwig()->load($partial->getFilePath()); | |
| $partialContent = $template->render(array_merge($parameters, $this->vars)); | |
| } | |
| if (str_ends_with($partial->fileName, '.blade.php')) { | |
| $partialContent = Blade::render($partial->content, array_merge($parameters, $this->vars)); | |
| } | |
| else { | |
| $this->getLoader()->setObject($partial); | |
| $template = $this->getTwig()->load($partial->getFilePath()); | |
| $partialContent = $template->render(array_merge($parameters, $this->vars)); | |
| } |
🤖 Prompt for AI Agents
In modules/cms/classes/Controller.php around lines 1069-1076, the Blade branch
renders with only $this->vars while the Twig branch renders
array_merge($parameters, $this->vars); update the Blade rendering to pass the
same merged variables so component objects added to $parameters are available to
Blade partials (i.e. call Blade::render with array_merge($parameters,
$this->vars) instead of $this->vars).
| /** | ||
| * Returns the base file name and extension. Applies a default extension, if none found. | ||
| */ | ||
| public function getFileNameParts($fileName = null) | ||
| { | ||
| if ($fileName === null) { | ||
| $fileName = $this->fileName; | ||
| } | ||
|
|
||
| if (!strlen($extension = pathinfo($fileName, PATHINFO_EXTENSION))) { | ||
| $extension = $this->defaultExtension; | ||
| $baseFile = $fileName; | ||
| } | ||
| elseif (($extension = pathinfo($fileName, PATHINFO_EXTENSION)) === 'blade') { | ||
| $extension = 'php'; | ||
| $baseFile = $fileName; | ||
| } | ||
| else { | ||
| $pos = strrpos($fileName, '.'); | ||
| $baseFile = substr($fileName, 0, $pos); | ||
| } | ||
|
|
||
| return [$baseFile, $extension]; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify getFileNameParts usage, CMS loading system, and Blade partial handling
# 1. Search for usage of getFileNameParts
echo "=== Searching for getFileNameParts usage ==="
rg -n "getFileNameParts" --type=php
# 2. Search for file loading logic
echo -e "\n=== Searching for load() method in CMS classes ==="
rg -n -A 3 "function load\(" --type=php modules/cms/classes/ 2>/dev/null || echo "No matches found or directory not accessible"
# 3. Search for Partial class definition and its inheritance
echo -e "\n=== Searching for Partial class definition ==="
rg -n "class Partial" --type=php -A 5
# 4. Check for .blade files or .blade.php files in themes
echo -e "\n=== Searching for blade template files ==="
find . -name "*blade.php" -o -name "*.blade" 2>/dev/null | head -20 || echo "No blade files found"Repository: wintercms/winter
Length of output: 4032
🏁 Script executed:
#!/bin/bash
# Read the Partial.php file to understand the code context
echo "=== Reading modules/cms/classes/Partial.php (lines 25-55) ==="
if [ -f "modules/cms/classes/Partial.php" ]; then
sed -n '25,55p' modules/cms/classes/Partial.php | cat -n
else
echo "File not found"
fiRepository: wintercms/winter
Length of output: 1078
Remove redundant pathinfo() call.
The code has a minor inefficiency: line 40 calls pathinfo($fileName, PATHINFO_EXTENSION) again, even though it was already evaluated in the outer if condition (line 36). Cache the result to avoid the redundant call:
- if (!strlen($extension = pathinfo($fileName, PATHINFO_EXTENSION))) {
+ $extension = pathinfo($fileName, PATHINFO_EXTENSION);
+
+ if (!strlen($extension)) {
$extension = $this->defaultExtension;
$baseFile = $fileName;
}
- elseif (($extension = pathinfo($fileName, PATHINFO_EXTENSION)) === 'blade') {
+ elseif ($extension === 'blade') {
$extension = 'php';
$baseFile = $fileName;
}🤖 Prompt for AI Agents
In modules/cms/classes/Partial.php around lines 27 to 50, the code redundantly
calls pathinfo($fileName, PATHINFO_EXTENSION) a second time in the elseif; reuse
the $extension variable already obtained instead. Replace the second pathinfo()
call with a comparison against the cached $extension (e.g. if ($extension ===
'blade') ...) and keep the same logic for setting $extension and $baseFile in
each branch so $baseFile is defined for all cases.
|
@mjauvin off the top of my head I think we'd need to restrict this with either a config, or a property in theme.yaml, or both; since it would allow PHP code to be written in the markup section of a CMS object which would bypass the safemode check. Additionally the CMS / Theme Editor would need blade support added (would be a lot easier if we were to get the #431 PR merged (swapping Ace with Monaco). |
Usage
from twig:
{% partial "my_partial.blade" %}from PHP:
Actual partial file path should be:
/partials/my_partial.blade.phpActual partial file path should be:
_my_partial.blade.phpSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.