Files required for Tailwind CSS based themes - #1687
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a Tailwind/DaisyUI XOOPS form renderer with HTML/JS safety helpers and themed rendering; registers it in the core loader; vendors Alpine.js docs/protection; and adds PHPUnit tests exercising rendering, escaping, calendar locale resolution, and buffer/asset safety. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Form as XoopsThemeForm
participant Renderer as XoopsFormRendererTailwind
participant Theme as xoTheme
Client->>Form: request renderThemeForm()
Form->>Renderer: renderThemeForm(form)
Renderer->>Renderer: iterate elements and call renderForm*()
alt xoTheme available
Renderer->>Theme: register/inject assets (Spectrum, editor, calendar, locale JS)
else no xoTheme
Renderer->>Client: emit <link>/<script> tags and inline JS (once)
end
Renderer->>Client: return assembled HTML (DaisyUI card, labels, controls, hidden fields, validation JS)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1687 +/- ##
=============================================
+ Coverage 0 19.36% +19.36%
- Complexity 0 7567 +7567
=============================================
Files 0 621 +621
Lines 0 39801 +39801
=============================================
+ Hits 0 7709 +7709
- Misses 0 32092 +32092 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@htdocs/class/xoopsform/renderer/XoopsFormRendererTailwind.php`:
- Around line 2-9: Add the required XOOPS source header block at the very top of
the file (before any code) to replace the shortened disclaimer currently used;
update the file that defines class XoopsFormRendererTailwind
(XoopsFormRendererTailwind.php) to include the standard XOOPS copyright/license
header block exactly as required by project guidelines so every source file
begins with that header.
- Around line 471-472: The renderFormLabel() return currently outputs an opening
<label> with id from $element->getName() and inner text from
$element->getValue() but never closes the tag; update the return in
renderFormLabel() to append the closing </label> so the output becomes a
well-formed label element (keep using $element->getName() and
$element->getValue()).
- Around line 42-47: The button renderer currently injects raw values from
$element (getType(), getName(), getValue(), getExtra(), title) causing XSS;
update XoopsFormRendererTailwind.php to HTML-escape all user-supplied outputs
using htmlspecialchars($value, ENT_QUOTES, 'UTF-8') (create/ reuse a helper like
escapeAttr()) for type, name, id, title, value, captions and descriptions
referenced in this renderer, and do not directly echo getExtra() unless it has
been validated/whitelisted—either escape attribute values inside extra or
parse/validate extra attributes before outputting; apply the same
htmlspecialchars pattern to the other affected locations (lines noted in the
comment) to ensure consistent output encoding.
- Around line 33-733: Add missing `@throws` tags to all new public method PHPDoc
blocks in this class (e.g. renderFormButton, renderFormButtonTray,
renderFormCheckBox, renderFormColorPicker, renderFormDhtmlTextArea,
renderFormRadio, renderFormSelect, renderFormText, renderFormTextArea,
renderFormTextDateSelect, renderThemeForm, renderFormFile, renderFormLabel,
renderFormPassword, renderFormElementTray and addThemeFormBreak) by updating
each method's PHPDoc to include a `@throws` entry (use a generic Exception class
such as \Exception or a more specific exception if applicable) so every public
method contains `@param`, `@return` and `@throws` tags per coding guidelines.
- Around line 209-215: The renderer's renderFormColorPicker() emits assets via
echo when $GLOBALS['xoTheme'] is not set, causing side effects and inconsistent
output; change these echoes to returnable markup and have
renderFormColorPicker() append that asset HTML into its return string instead of
printing; detect $GLOBALS['xoTheme'] (as currently done) and when absent build
the same <script> and <link> tags using XOOPS_URL into a local $assets string,
then concatenate $assets with the existing returned input HTML from
renderFormColorPicker() so callers only receive returned HTML and no direct echo
side effects.
In `@htdocs/xoops_lib/Frameworks/alpine/README.md`:
- Around line 13-15: The fenced code block containing the line
"xoops_lib/Frameworks/alpine/alpine.min.js" is missing a language identifier and
triggers MD040; edit the README.md fenced block (the triple-backtick block that
contains that path string) to add a language token such as text (i.e., change
``` to ```text) so the markdown linter stops flagging it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f95e7162-86fc-4084-8aec-2eef6e5cce60
⛔ Files ignored due to path filters (1)
htdocs/xoops_lib/Frameworks/alpine/alpine.min.jsis excluded by!**/*.min.js
📒 Files selected for processing (4)
htdocs/class/xoopsform/renderer/XoopsFormRendererTailwind.phphtdocs/class/xoopsload.phphtdocs/xoops_lib/Frameworks/alpine/README.mdhtdocs/xoops_lib/Frameworks/alpine/index.php
Comment on lines
+33
to
+733
| /** | ||
| * Render support for XoopsFormButton | ||
| * | ||
| * @param XoopsFormButton $element form element | ||
| * | ||
| * @return string rendered form element | ||
| */ | ||
| public function renderFormButton(XoopsFormButton $element) | ||
| { | ||
| return '<button type="' . $element->getType() . '"' | ||
| . ' class="btn btn-neutral" name="' . $element->getName() . '"' | ||
| . ' id="' . $element->getName() . '" title="' . $element->getValue() . '"' | ||
| . ' value="' . $element->getValue() . '"' | ||
| . $element->getExtra() . '>' . $element->getValue() . '</button>'; | ||
| } | ||
|
|
||
| /** | ||
| * Render support for XoopsFormButtonTray | ||
| * | ||
| * @param XoopsFormButtonTray $element form element | ||
| * | ||
| * @return string rendered form element | ||
| */ | ||
| public function renderFormButtonTray(XoopsFormButtonTray $element) | ||
| { | ||
| $ret = '<div class="flex flex-wrap gap-2">'; | ||
| if ($element->_showDelete) { | ||
| $ret .= '<button type="submit" class="btn btn-error" name="delete" id="delete"' | ||
| . ' onclick="this.form.elements.op.value=\'delete\'">' . _DELETE . '</button>'; | ||
| } | ||
| $ret .= '<input type="button" class="btn btn-error" name="cancel" id="cancel"' | ||
| . ' onClick="history.go(-1);return true;" value="' . _CANCEL . '">' | ||
| . '<button type="reset" class="btn btn-warning" name="reset" id="reset">' . _RESET . '</button>' | ||
| . '<button type="' . $element->getType() . '" class="btn btn-success" name="' . $element->getName() | ||
| . '" id="' . $element->getName() . '" ' . $element->getExtra() | ||
| . '>' . $element->getValue() . '</button>' | ||
| . '</div>'; | ||
|
|
||
| return $ret; | ||
| } | ||
|
|
||
| /** | ||
| * Render support for XoopsFormCheckBox | ||
| * | ||
| * @param XoopsFormCheckBox $element form element | ||
| * | ||
| * @return string rendered form element | ||
| */ | ||
| public function renderFormCheckBox(XoopsFormCheckBox $element) | ||
| { | ||
| $elementName = $element->getName(); | ||
| $elementId = $elementName; | ||
| $elementOptions = $element->getOptions(); | ||
| if (count($elementOptions) > 1 && substr($elementName, -2, 2) !== '[]') { | ||
| $elementName .= '[]'; | ||
| $element->setName($elementName); | ||
| } | ||
|
|
||
| switch ((int) ($element->columns)) { | ||
| case 0: | ||
| return $this->renderCheckedInline($element, 'checkbox', $elementId, $elementName); | ||
| case 1: | ||
| return $this->renderCheckedOneColumn($element, 'checkbox', $elementId, $elementName); | ||
| default: | ||
| return $this->renderCheckedColumnar($element, 'checkbox', $elementId, $elementName); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Render an inline checkbox or radio element | ||
| * | ||
| * @param XoopsFormCheckBox|XoopsFormRadio $element element being rendered | ||
| * @param string $type 'checkbox' or 'radio' | ||
| * @param string $elementId input 'id' attribute of element | ||
| * @param string $elementName input 'name' attribute of element | ||
| * @return string | ||
| */ | ||
| protected function renderCheckedInline($element, $type, $elementId, $elementName) | ||
| { | ||
| $ret = '<div class="flex flex-wrap gap-4">'; | ||
| $idSuffix = 0; | ||
| $elementValue = $element->getValue(); | ||
| foreach ($element->getOptions() as $value => $name) { | ||
| ++$idSuffix; | ||
| $ret .= '<label class="label cursor-pointer gap-2">'; | ||
| $ret .= "<input class='" . $type . " " . $type . "-primary' type='" . $type . "'" | ||
| . " name='{$elementName}' id='{$elementId}{$idSuffix}' title='" | ||
| . htmlspecialchars(strip_tags($name), ENT_QUOTES | ENT_HTML5) . "' value='" | ||
| . htmlspecialchars($value, ENT_QUOTES | ENT_HTML5) . "'"; | ||
| if (is_array($elementValue) ? in_array($value, $elementValue) : $value == $elementValue) { | ||
| $ret .= ' checked'; | ||
| } | ||
| $ret .= $element->getExtra() . '>'; | ||
| $ret .= '<span class="label-text">' . $name . $element->getDelimeter() . '</span>'; | ||
| $ret .= '</label>'; | ||
| } | ||
| $ret .= '</div>'; | ||
|
|
||
| return $ret; | ||
| } | ||
|
|
||
| /** | ||
| * Render a single column checkbox or radio element | ||
| * | ||
| * @param XoopsFormCheckBox|XoopsFormRadio $element element being rendered | ||
| * @param string $type 'checkbox' or 'radio' | ||
| * @param string $elementId input 'id' attribute of element | ||
| * @param string $elementName input 'name' attribute of element | ||
| * @return string | ||
| */ | ||
| protected function renderCheckedOneColumn($element, $type, $elementId, $elementName) | ||
| { | ||
| $ret = '<div class="flex flex-col gap-2">'; | ||
| $idSuffix = 0; | ||
| $elementValue = $element->getValue(); | ||
| foreach ($element->getOptions() as $value => $name) { | ||
| ++$idSuffix; | ||
| $ret .= '<label class="label cursor-pointer justify-start gap-2">'; | ||
| $ret .= "<input class='" . $type . " " . $type . "-primary' type='" . $type . "'" | ||
| . " name='{$elementName}' id='{$elementId}{$idSuffix}' title='" | ||
| . htmlspecialchars(strip_tags($name), ENT_QUOTES | ENT_HTML5) . "' value='" | ||
| . htmlspecialchars($value, ENT_QUOTES | ENT_HTML5) . "'"; | ||
| if (is_array($elementValue) ? in_array($value, $elementValue) : $value == $elementValue) { | ||
| $ret .= ' checked'; | ||
| } | ||
| $ret .= $element->getExtra() . '>'; | ||
| $ret .= '<span class="label-text">' . $name . $element->getDelimeter() . '</span>'; | ||
| $ret .= '</label>'; | ||
| } | ||
| $ret .= '</div>'; | ||
|
|
||
| return $ret; | ||
| } | ||
|
|
||
| /** | ||
| * Render a multicolumn checkbox or radio element | ||
| * | ||
| * @param XoopsFormCheckBox|XoopsFormRadio $element element being rendered | ||
| * @param string $type 'checkbox' or 'radio' | ||
| * @param string $elementId input 'id' attribute of element | ||
| * @param string $elementName input 'name' attribute of element | ||
| * @return string | ||
| */ | ||
| protected function renderCheckedColumnar($element, $type, $elementId, $elementName) | ||
| { | ||
| $ret = '<div class="grid grid-cols-2 md:grid-cols-3 gap-2">'; | ||
| $idSuffix = 0; | ||
| $elementValue = $element->getValue(); | ||
| foreach ($element->getOptions() as $value => $name) { | ||
| ++$idSuffix; | ||
| $ret .= '<label class="label cursor-pointer justify-start gap-2">'; | ||
| $ret .= "<input class='" . $type . " " . $type . "-primary' type='" . $type . "'" | ||
| . " name='{$elementName}' id='{$elementId}{$idSuffix}' title='" | ||
| . htmlspecialchars(strip_tags($name), ENT_QUOTES | ENT_HTML5) . "' value='" | ||
| . htmlspecialchars($value, ENT_QUOTES | ENT_HTML5) . "'"; | ||
| if (is_array($elementValue) ? in_array($value, $elementValue) : $value == $elementValue) { | ||
| $ret .= ' checked'; | ||
| } | ||
| $ret .= $element->getExtra() . '>'; | ||
| $ret .= '<span class="label-text">' . $name . $element->getDelimeter() . '</span>'; | ||
| $ret .= '</label>'; | ||
| } | ||
| $ret .= '</div>'; | ||
|
|
||
| return $ret; | ||
| } | ||
|
|
||
| /** | ||
| * Render support for XoopsFormColorPicker | ||
| * | ||
| * @param XoopsFormColorPicker $element form element | ||
| * | ||
| * @return string rendered form element | ||
| */ | ||
| public function renderFormColorPicker(XoopsFormColorPicker $element) | ||
| { | ||
| if (isset($GLOBALS['xoTheme'])) { | ||
| $GLOBALS['xoTheme']->addScript('include/spectrum.js'); | ||
| $GLOBALS['xoTheme']->addStylesheet('include/spectrum.css'); | ||
| } else { | ||
| echo '<script type="text/javascript" src="' . XOOPS_URL . '/include/spectrum.js"></script>'; | ||
| echo '<link rel="stylesheet" type="text/css" href="' . XOOPS_URL . '/include/spectrum.css">'; | ||
| } | ||
| return '<input class="input input-bordered w-24 h-10 p-1" type="color" name="' . $element->getName() | ||
| . '" title="' . $element->getTitle() . '" id="' . $element->getName() | ||
| . '" size="7" maxlength="7" value="' . $element->getValue() . '"' . $element->getExtra() . '>'; | ||
| } | ||
|
|
||
| /** | ||
| * Render support for XoopsFormDhtmlTextArea | ||
| * | ||
| * @param XoopsFormDhtmlTextArea $element form element | ||
| * | ||
| * @return string rendered form element | ||
| */ | ||
| public function renderFormDhtmlTextArea(XoopsFormDhtmlTextArea $element) | ||
| { | ||
| xoops_loadLanguage('formdhtmltextarea'); | ||
| $ret = ''; | ||
| $ret .= $this->renderFormDhtmlTAXoopsCode($element) . "<br>\n"; | ||
| $ret .= $this->renderFormDhtmlTATypography($element); | ||
| $ret .= "<br>\n"; | ||
| $ret .= "<textarea class='textarea textarea-bordered w-full font-mono' id='" . $element->getName() . "' name='" . $element->getName() | ||
| . "' title='" . $element->getTitle() . "' onselect=\"xoopsSavePosition('" . $element->getName() | ||
| . "');\" onclick=\"xoopsSavePosition('" . $element->getName() | ||
| . "');\" onkeyup=\"xoopsSavePosition('" . $element->getName() . "');\" cols='" | ||
| . $element->getCols() . "' rows='" . $element->getRows() . "'" . $element->getExtra() | ||
| . '>' . $element->getValue() . "</textarea>\n"; | ||
|
|
||
| if (empty($element->skipPreview)) { | ||
| if (empty($GLOBALS['xoTheme'])) { | ||
| $element->js .= implode('', file(XOOPS_ROOT_PATH . '/class/textsanitizer/image/image.js')); | ||
| } else { | ||
| $GLOBALS['xoTheme']->addScript( | ||
| '/class/textsanitizer/image/image.js', | ||
| ['type' => 'text/javascript'], | ||
| ); | ||
| } | ||
| $button = "<button type='button' class='btn btn-primary btn-sm' onclick=\"form_instantPreview('" . XOOPS_URL | ||
| . "', '" . $element->getName() . "','" . XOOPS_URL . "/images', " . (int) $element->doHtml . ", '" | ||
| . $GLOBALS['xoopsSecurity']->createToken() . "')\" title='" . _PREVIEW . "'>" . _PREVIEW . "</button>"; | ||
|
|
||
| $ret .= '<br>' . "<div id='" . $element->getName() . "_hidden' class='card bg-base-200 mt-2'>" | ||
| . "<div class='card-body p-4'>" | ||
| . "<div class='card-title text-sm'>" . $button . "</div>" | ||
| . "<div id='" . $element->getName() . "_hidden_data'>" . _XOOPS_FORM_PREVIEW_CONTENT . '</div>' | ||
| . '</div></div>'; | ||
| } | ||
| $javascript_file = XOOPS_URL . '/include/formdhtmltextarea.js'; | ||
| $javascript_file_element = 'include_formdhtmltextarea_js'; | ||
| $javascript = ($element->js ? '<script type="text/javascript">' . $element->js . '</script>' : ''); | ||
| $javascript .= <<<EOJS | ||
| <script> | ||
| var el = document.getElementById('{$javascript_file_element}'); | ||
| if (el === null) { | ||
| var xformtag = document.createElement('script'); | ||
| xformtag.id = '{$javascript_file_element}'; | ||
| xformtag.type = 'text/javascript'; | ||
| xformtag.src = '{$javascript_file}'; | ||
| document.body.appendChild(xformtag); | ||
| } | ||
| </script> | ||
| EOJS; | ||
|
|
||
| return $javascript . $ret; | ||
| } | ||
|
|
||
| /** | ||
| * Render xoopscode buttons for editor, include calling text sanitizer extensions | ||
| * | ||
| * @param XoopsFormDhtmlTextArea $element form element | ||
| * | ||
| * @return string rendered buttons for xoopscode assistance | ||
| */ | ||
| protected function renderFormDhtmlTAXoopsCode(XoopsFormDhtmlTextArea $element) | ||
| { | ||
| $textarea_id = $element->getName(); | ||
| $code = "<div class='flex flex-wrap gap-1'>"; | ||
| $btn = "btn btn-neutral btn-sm"; | ||
| $code .= "<button type='button' class='{$btn}' onclick='xoopsCodeUrl(\"{$textarea_id}\", \"" . htmlspecialchars(_ENTERURL, ENT_QUOTES | ENT_HTML5) . "\", \"" . htmlspecialchars(_ENTERWEBTITLE, ENT_QUOTES | ENT_HTML5) . "\");' title='" . _XOOPS_FORM_ALT_URL . "'><span class='fa-solid fa-link' aria-hidden='true'></span></button>"; | ||
| $code .= "<button type='button' class='{$btn}' onclick='xoopsCodeEmail(\"{$textarea_id}\", \"" . htmlspecialchars(_ENTEREMAIL, ENT_QUOTES | ENT_HTML5) . "\", \"" . htmlspecialchars(_ENTERWEBTITLE, ENT_QUOTES | ENT_HTML5) . "\");' title='" . _XOOPS_FORM_ALT_EMAIL . "'><span class='fa-solid fa-envelope' aria-hidden='true'></span></button>"; | ||
| $code .= "<button type='button' class='{$btn}' onclick='xoopsCodeImg(\"{$textarea_id}\", \"" . htmlspecialchars(_ENTERIMGURL, ENT_QUOTES | ENT_HTML5) . "\", \"" . htmlspecialchars(_ENTERIMGPOS, ENT_QUOTES | ENT_HTML5) . "\", \"" . htmlspecialchars(_IMGPOSRORL, ENT_QUOTES | ENT_HTML5) . "\", \"" . htmlspecialchars(_ERRORIMGPOS, ENT_QUOTES | ENT_HTML5) . "\", \"" . htmlspecialchars(_XOOPS_FORM_ALT_ENTERWIDTH, ENT_QUOTES | ENT_HTML5) . "\");' title='" . _XOOPS_FORM_ALT_IMG . "'><span class='fa-solid fa-file-image' aria-hidden='true'></span></button>"; | ||
| $code .= "<button type='button' class='{$btn}' onclick='openWithSelfMain(\"" . XOOPS_URL . "/imagemanager.php?target={$textarea_id}\",\"imgmanager\",400,430);' title='" . _XOOPS_FORM_ALT_IMAGE . "'><span class='fa-solid fa-file-image' aria-hidden='true'></span><small> Manager</small></button>"; | ||
| $code .= "<button type='button' class='{$btn}' onclick='openWithSelfMain(\"" . XOOPS_URL . "/misc.php?action=showpopups&type=smilies&target={$textarea_id}\",\"smilies\",300,475);' title='" . _XOOPS_FORM_ALT_SMILEY . "'><span class='fa-solid fa-face-smile' aria-hidden='true'></span></button>"; | ||
|
|
||
| $myts = \MyTextSanitizer::getInstance(); | ||
| $extensions = array_filter($myts->config['extensions']); | ||
| foreach (array_keys($extensions) as $key) { | ||
| $extension = $myts->loadExtension($key); | ||
| $result = $extension->encode($textarea_id); | ||
| $encode = $result[0] ?? ''; | ||
| $js = $result[1] ?? ''; | ||
| if (empty($encode)) { | ||
| continue; | ||
| } | ||
| // Extensions output Bootstrap classes — remap the common ones to DaisyUI. | ||
| $encode = str_replace(['btn-default', 'btn-secondary'], 'btn btn-neutral btn-sm', $encode); | ||
|
|
||
| $code .= $encode; | ||
| if (!empty($js)) { | ||
| $element->js .= $js; | ||
| } | ||
| } | ||
| $code .= "<button type='button' class='{$btn}' onclick='xoopsCodeCode(\"{$textarea_id}\", \"" . htmlspecialchars(_ENTERCODE, ENT_QUOTES | ENT_HTML5) . "\");' title='" . _XOOPS_FORM_ALT_CODE . "'><span class='fa-solid fa-code' aria-hidden='true'></span></button>"; | ||
| $code .= "<button type='button' class='{$btn}' onclick='xoopsCodeQuote(\"{$textarea_id}\", \"" . htmlspecialchars(_ENTERQUOTE, ENT_QUOTES | ENT_HTML5) . "\");' title='" . _XOOPS_FORM_ALT_QUOTE . "'><span class='fa-solid fa-quote-right' aria-hidden='true'></span></button>"; | ||
| $code .= "</div>"; | ||
|
|
||
| $xoopsPreload = XoopsPreload::getInstance(); | ||
| $xoopsPreload->triggerEvent('core.class.xoopsform.formdhtmltextarea.codeicon', [&$code]); | ||
|
|
||
| return $code; | ||
| } | ||
|
|
||
| /** | ||
| * Render typography controls for editor (font, size, color) | ||
| * | ||
| * @param XoopsFormDhtmlTextArea $element form element | ||
| * | ||
| * @return string rendered typography controls | ||
| */ | ||
| protected function renderFormDhtmlTATypography(XoopsFormDhtmlTextArea $element) | ||
| { | ||
| $textarea_id = $element->getName(); | ||
| $hiddentext = $element->_hiddenText; | ||
|
|
||
| $fontarray = !empty($GLOBALS['formtextdhtml_fonts']) ? $GLOBALS['formtextdhtml_fonts'] : [ | ||
| 'Arial', 'Courier', 'Georgia', 'Helvetica', 'Impact', 'Verdana', 'Haettenschweiler', | ||
| ]; | ||
|
|
||
| $colorArray = [ | ||
| 'Black' => '000000', 'Blue' => '38AAFF', 'Brown' => '987857', | ||
| 'Green' => '79D271', 'Grey' => '888888', 'Orange' => 'FFA700', | ||
| 'Paper' => 'E0E0E0', 'Purple' => '363E98', 'Red' => 'FF211E', | ||
| 'White' => 'FEFEFE', 'Yellow' => 'FFD628', | ||
| ]; | ||
|
|
||
| $btn = "btn btn-neutral btn-sm"; | ||
| $menuCls = "dropdown-content menu bg-base-100 rounded-box z-50 p-2 shadow max-h-64 overflow-y-auto flex-nowrap"; | ||
|
|
||
| $fontStr = "<div class='flex flex-wrap gap-1 mt-2'>"; | ||
|
|
||
| // Size dropdown | ||
| $fontStr .= "<div class='dropdown'>" | ||
| . "<div tabindex='0' role='button' class='{$btn}' title='" . _SIZE . "'><span class='fa-solid fa-text-height'></span></div>" | ||
| . "<ul tabindex='0' class='{$menuCls}'>"; | ||
| foreach ($GLOBALS['formtextdhtml_sizes'] as $value => $name) { | ||
| $fontStr .= "<li><a href=\"javascript:xoopsSetElementAttribute('size', '{$value}', '{$textarea_id}', '{$hiddentext}');\">{$name}</a></li>"; | ||
| } | ||
| $fontStr .= "</ul></div>"; | ||
|
|
||
| // Font dropdown | ||
| $fontStr .= "<div class='dropdown'>" | ||
| . "<div tabindex='0' role='button' class='{$btn}' title='" . _FONT . "'><span class='fa-solid fa-font'></span></div>" | ||
| . "<ul tabindex='0' class='{$menuCls}'>"; | ||
| foreach ($fontarray as $font) { | ||
| $fontStr .= "<li><a href=\"javascript:xoopsSetElementAttribute('font', '{$font}', '{$textarea_id}', '{$hiddentext}');\">{$font}</a></li>"; | ||
| } | ||
| $fontStr .= "</ul></div>"; | ||
|
|
||
| // Color dropdown | ||
| $fontStr .= "<div class='dropdown'>" | ||
| . "<div tabindex='0' role='button' class='{$btn}' title='" . _COLOR . "'><span class='fa-solid fa-palette'></span></div>" | ||
| . "<ul tabindex='0' class='{$menuCls}'>"; | ||
| foreach ($colorArray as $color => $hex) { | ||
| $fontStr .= "<li><a href=\"javascript:xoopsSetElementAttribute('color', '{$hex}', '{$textarea_id}', '{$hiddentext}');\"><span style=\"color:#{$hex};\">{$color}</span></a></li>"; | ||
| } | ||
| $fontStr .= "</ul></div>"; | ||
|
|
||
| // Style buttons | ||
| $fontStr .= "<div class='join'>"; | ||
| $fontStr .= "<button type='button' class='btn btn-neutral btn-sm join-item' onclick='xoopsMakeBold(\"{$hiddentext}\", \"{$textarea_id}\");' title='" . _XOOPS_FORM_ALT_BOLD . "'><span class='fa-solid fa-bold'></span></button>"; | ||
| $fontStr .= "<button type='button' class='btn btn-neutral btn-sm join-item' onclick='xoopsMakeItalic(\"{$hiddentext}\", \"{$textarea_id}\");' title='" . _XOOPS_FORM_ALT_ITALIC . "'><span class='fa-solid fa-italic'></span></button>"; | ||
| $fontStr .= "<button type='button' class='btn btn-neutral btn-sm join-item' onclick='xoopsMakeUnderline(\"{$hiddentext}\", \"{$textarea_id}\");' title='" . _XOOPS_FORM_ALT_UNDERLINE . "'><span class='fa-solid fa-underline'></span></button>"; | ||
| $fontStr .= "<button type='button' class='btn btn-neutral btn-sm join-item' onclick='xoopsMakeLineThrough(\"{$hiddentext}\", \"{$textarea_id}\");' title='" . _XOOPS_FORM_ALT_LINETHROUGH . "'><span class='fa-solid fa-strikethrough'></span></button>"; | ||
| $fontStr .= "</div>"; | ||
|
|
||
| // Align buttons | ||
| $fontStr .= "<div class='join'>"; | ||
| $fontStr .= "<button type='button' class='btn btn-neutral btn-sm join-item' onclick='xoopsMakeLeft(\"{$hiddentext}\", \"{$textarea_id}\");' title='" . _XOOPS_FORM_ALT_LEFT . "'><span class='fa-solid fa-align-left'></span></button>"; | ||
| $fontStr .= "<button type='button' class='btn btn-neutral btn-sm join-item' onclick='xoopsMakeCenter(\"{$hiddentext}\", \"{$textarea_id}\");' title='" . _XOOPS_FORM_ALT_CENTER . "'><span class='fa-solid fa-align-center'></span></button>"; | ||
| $fontStr .= "<button type='button' class='btn btn-neutral btn-sm join-item' onclick='xoopsMakeRight(\"{$hiddentext}\", \"{$textarea_id}\");' title='" . _XOOPS_FORM_ALT_RIGHT . "'><span class='fa-solid fa-align-right'></span></button>"; | ||
| $fontStr .= "</div>"; | ||
|
|
||
| // Length check button | ||
| $maxlength = $element->configs['maxlength'] ?? 0; | ||
| $fontStr .= "<button type='button' class='{$btn}' onclick=\"XoopsCheckLength('" | ||
| . $element->getName() . "', '" . $maxlength . "', '" | ||
| . _XOOPS_FORM_ALT_LENGTH . "', '" . _XOOPS_FORM_ALT_LENGTH_MAX . "');\" title='" | ||
| . _XOOPS_FORM_ALT_CHECKLENGTH . "'><span class='fa-solid fa-square-check'></span></button>"; | ||
| $fontStr .= "</div>"; | ||
|
|
||
| return $fontStr; | ||
| } | ||
|
|
||
| /** | ||
| * Render support for XoopsFormElementTray | ||
| * | ||
| * @param XoopsFormElementTray $element form element | ||
| * | ||
| * @return string rendered form element | ||
| */ | ||
| public function renderFormElementTray(XoopsFormElementTray $element) | ||
| { | ||
| $count = 0; | ||
| $inline = (\XoopsFormElementTray::ORIENTATION_VERTICAL === $element->getOrientation()); | ||
| $ret = $inline ? '<div class="flex flex-wrap items-center gap-2">' : '<div class="space-y-2">'; | ||
| foreach ($element->getElements() as $ele) { | ||
| if ($count > 0 && !$inline) { | ||
| $ret .= $element->getDelimeter(); | ||
| } | ||
| if ($inline) { | ||
| $ret .= '<span class="inline-flex items-center gap-1">'; | ||
| } | ||
| if ($ele->getCaption() != '') { | ||
| $ret .= '<label for="' . $ele->getName() . '" class="label-text">' | ||
| . $ele->getCaption() | ||
| . ($ele->isRequired() ? '<span class="text-error ms-1">*</span>' : '') | ||
| . '</label> '; | ||
| } | ||
| $ret .= $ele->render() . NWLINE; | ||
| if ($inline) { | ||
| $ret .= '</span>'; | ||
| } | ||
| if (!$ele->isHidden()) { | ||
| ++$count; | ||
| } | ||
| } | ||
| $ret .= '</div>'; | ||
|
|
||
| return $ret; | ||
| } | ||
|
|
||
| /** | ||
| * Render support for XoopsFormFile | ||
| * | ||
| * @param XoopsFormFile $element form element | ||
| * | ||
| * @return string rendered form element | ||
| */ | ||
| public function renderFormFile(XoopsFormFile $element) | ||
| { | ||
| return '<input type="hidden" name="MAX_FILE_SIZE" value="' . $element->getMaxFileSize() . '">' | ||
| . '<input type="file" class="file-input file-input-bordered w-full" name="' . $element->getName() | ||
| . '" id="' . $element->getName() | ||
| . '" title="' . $element->getTitle() . '" ' . $element->getExtra() . '>' | ||
| . '<input type="hidden" name="xoops_upload_file[]" id="xoops_upload_file[]" value="' | ||
| . $element->getName() . '">'; | ||
| } | ||
|
|
||
| /** | ||
| * Render support for XoopsFormLabel | ||
| * | ||
| * @param XoopsFormLabel $element form element | ||
| * | ||
| * @return string rendered form element | ||
| */ | ||
| public function renderFormLabel(XoopsFormLabel $element) | ||
| { | ||
| return '<label class="label label-text" id="' . $element->getName() . '">' . $element->getValue(); | ||
| } | ||
|
|
||
| /** | ||
| * Render support for XoopsFormPassword | ||
| * | ||
| * @param XoopsFormPassword $element form element | ||
| * | ||
| * @return string rendered form element | ||
| */ | ||
| public function renderFormPassword(XoopsFormPassword $element) | ||
| { | ||
| return '<input class="input input-bordered w-full" type="password" name="' | ||
| . $element->getName() . '" id="' . $element->getName() . '" size="' . $element->getSize() | ||
| . '" maxlength="' . $element->getMaxlength() . '" value="' . $element->getValue() . '"' | ||
| . $element->getExtra() . ' ' . ($element->autoComplete ? '' : 'autocomplete="off" ') . '/>'; | ||
| } | ||
|
|
||
| /** | ||
| * Render support for XoopsFormRadio | ||
| * | ||
| * @param XoopsFormRadio $element form element | ||
| * | ||
| * @return string rendered form element | ||
| */ | ||
| public function renderFormRadio(XoopsFormRadio $element) | ||
| { | ||
| $elementName = $element->getName(); | ||
| $elementId = $elementName; | ||
|
|
||
| switch ((int) ($element->columns)) { | ||
| case 0: | ||
| return $this->renderCheckedInline($element, 'radio', $elementId, $elementName); | ||
| case 1: | ||
| return $this->renderCheckedOneColumn($element, 'radio', $elementId, $elementName); | ||
| default: | ||
| return $this->renderCheckedColumnar($element, 'radio', $elementId, $elementName); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Render support for XoopsFormSelect | ||
| * | ||
| * @param XoopsFormSelect $element form element | ||
| * | ||
| * @return string rendered form element | ||
| */ | ||
| public function renderFormSelect(XoopsFormSelect $element) | ||
| { | ||
| $ele_name = $element->getName(); | ||
| $ele_title = $element->getTitle(); | ||
| $ele_value = $element->getValue(); | ||
| $ele_options = $element->getOptions(); | ||
| $ret = '<select class="select select-bordered w-full" size="' | ||
| . $element->getSize() . '"' . $element->getExtra(); | ||
| if ($element->isMultiple() != false) { | ||
| $ret .= ' name="' . $ele_name . '[]" id="' . $ele_name . '" title="' . $ele_title | ||
| . '" multiple="multiple">'; | ||
| } else { | ||
| $ret .= ' name="' . $ele_name . '" id="' . $ele_name . '" title="' . $ele_title . '">'; | ||
| } | ||
| foreach ($ele_options as $value => $name) { | ||
| $ret .= '<option value="' . htmlspecialchars($value, ENT_QUOTES | ENT_HTML5) . '"'; | ||
| if (count($ele_value) > 0 && in_array($value, $ele_value)) { | ||
| $ret .= ' selected'; | ||
| } | ||
| $ret .= '>' . $name . '</option>'; | ||
| } | ||
| $ret .= '</select>'; | ||
|
|
||
| return $ret; | ||
| } | ||
|
|
||
| /** | ||
| * Render support for XoopsFormText | ||
| * | ||
| * @param XoopsFormText $element form element | ||
| * | ||
| * @return string rendered form element | ||
| */ | ||
| public function renderFormText(XoopsFormText $element) | ||
| { | ||
| return "<input class='input input-bordered w-full' type='text' name='" | ||
| . $element->getName() . "' title='" . $element->getTitle() . "' id='" . $element->getName() | ||
| . "' size='" . $element->getSize() . "' maxlength='" . $element->getMaxlength() | ||
| . "' value='" . $element->getValue() . "'" . $element->getExtra() . '>'; | ||
| } | ||
|
|
||
| /** | ||
| * Render support for XoopsFormTextArea | ||
| * | ||
| * @param XoopsFormTextArea $element form element | ||
| * | ||
| * @return string rendered form element | ||
| */ | ||
| public function renderFormTextArea(XoopsFormTextArea $element) | ||
| { | ||
| return "<textarea class='textarea textarea-bordered w-full' name='" | ||
| . $element->getName() . "' id='" . $element->getName() . "' title='" . $element->getTitle() | ||
| . "' rows='" . $element->getRows() . "' cols='" . $element->getCols() . "'" | ||
| . $element->getExtra() . '>' . $element->getValue() . '</textarea>'; | ||
| } | ||
|
|
||
| /** | ||
| * Render support for XoopsFormTextDateSelect | ||
| * | ||
| * @param XoopsFormTextDateSelect $element form element | ||
| * | ||
| * @return string rendered form element | ||
| */ | ||
| public function renderFormTextDateSelect(XoopsFormTextDateSelect $element) | ||
| { | ||
| static $included = false; | ||
| if (file_exists(XOOPS_ROOT_PATH . '/language/' . $GLOBALS['xoopsConfig']['language'] . '/calendar.php')) { | ||
| include_once XOOPS_ROOT_PATH . '/language/' . $GLOBALS['xoopsConfig']['language'] . '/calendar.php'; | ||
| } else { | ||
| include_once XOOPS_ROOT_PATH . '/language/english/calendar.php'; | ||
| } | ||
|
|
||
| $ele_name = $element->getName(); | ||
| $ele_value = $element->getValue(false); | ||
| if (is_string($ele_value)) { | ||
| $display_value = $ele_value; | ||
| $ele_value = time(); | ||
| } elseif ($ele_value === 0) { | ||
| $display_value = ''; | ||
| $ele_value = time(); | ||
| } else { | ||
| $display_value = date(_SHORTDATESTRING, $ele_value); | ||
| } | ||
|
|
||
| $jstime = formatTimestamp($ele_value, 'm/d/Y'); | ||
| if (isset($GLOBALS['xoTheme']) && is_object($GLOBALS['xoTheme'])) { | ||
| $GLOBALS['xoTheme']->addScript('include/calendar.js'); | ||
| $GLOBALS['xoTheme']->addStylesheet('include/calendar-blue.css'); | ||
| if (!$included) { | ||
| $included = true; | ||
| $GLOBALS['xoTheme']->addScript('', '', ' | ||
| var calendar = null; | ||
| function selected(cal, date) { cal.sel.value = date; } | ||
| function closeHandler(cal) { | ||
| cal.hide(); | ||
| Calendar.removeEvent(document, "mousedown", checkCalendar); | ||
| } | ||
| function checkCalendar(ev) { | ||
| var el = Calendar.is_ie ? Calendar.getElement(ev) : Calendar.getTargetElement(ev); | ||
| for (; el != null; el = el.parentNode) | ||
| if (el == calendar.element || el.tagName == "A") break; | ||
| if (el == null) { calendar.callCloseHandler(); Calendar.stopEvent(ev); } | ||
| } | ||
| function showCalendar(id) { | ||
| var el = xoopsGetElementById(id); | ||
| if (calendar != null) { calendar.hide(); } | ||
| else { | ||
| var cal = new Calendar(true, "' . $jstime . '", selected, closeHandler); | ||
| calendar = cal; | ||
| cal.setRange(1900, 2100); | ||
| calendar.create(); | ||
| } | ||
| calendar.sel = el; | ||
| calendar.parseDate(el.value); | ||
| calendar.showAtElement(el); | ||
| Calendar.addEvent(document, "mousedown", checkCalendar); | ||
| return false; | ||
| } | ||
| Calendar._DN = new Array("' . _CAL_SUNDAY . '", "' . _CAL_MONDAY . '", "' . _CAL_TUESDAY . '", "' . _CAL_WEDNESDAY . '", "' . _CAL_THURSDAY . '", "' . _CAL_FRIDAY . '", "' . _CAL_SATURDAY . '", "' . _CAL_SUNDAY . '"); | ||
| Calendar._MN = new Array("' . _CAL_JANUARY . '", "' . _CAL_FEBRUARY . '", "' . _CAL_MARCH . '", "' . _CAL_APRIL . '", "' . _CAL_MAY . '", "' . _CAL_JUNE . '", "' . _CAL_JULY . '", "' . _CAL_AUGUST . '", "' . _CAL_SEPTEMBER . '", "' . _CAL_OCTOBER . '", "' . _CAL_NOVEMBER . '", "' . _CAL_DECEMBER . '"); | ||
| Calendar._TT = {}; | ||
| Calendar._TT["TOGGLE"] = "' . _CAL_TGL1STD . '"; | ||
| Calendar._TT["PREV_YEAR"] = "' . _CAL_PREVYR . '"; | ||
| Calendar._TT["PREV_MONTH"] = "' . _CAL_PREVMNTH . '"; | ||
| Calendar._TT["GO_TODAY"] = "' . _CAL_GOTODAY . '"; | ||
| Calendar._TT["NEXT_MONTH"] = "' . _CAL_NXTMNTH . '"; | ||
| Calendar._TT["NEXT_YEAR"] = "' . _CAL_NEXTYR . '"; | ||
| Calendar._TT["SEL_DATE"] = "' . _CAL_SELDATE . '"; | ||
| Calendar._TT["DRAG_TO_MOVE"] = "' . _CAL_DRAGMOVE . '"; | ||
| Calendar._TT["PART_TODAY"] = "(' . _CAL_TODAY . ')"; | ||
| Calendar._TT["MON_FIRST"] = "' . _CAL_DISPM1ST . '"; | ||
| Calendar._TT["SUN_FIRST"] = "' . _CAL_DISPS1ST . '"; | ||
| Calendar._TT["CLOSE"] = "' . _CLOSE . '"; | ||
| Calendar._TT["TODAY"] = "' . _CAL_TODAY . '"; | ||
| Calendar._TT["DEF_DATE_FORMAT"] = "' . _SHORTDATESTRING . '"; | ||
| Calendar._TT["TT_DATE_FORMAT"] = "' . _SHORTDATESTRING . '"; | ||
| Calendar._TT["WK"] = ""; | ||
| '); | ||
| } | ||
| } | ||
| return '<div class="join w-full">' | ||
| . '<input class="input input-bordered join-item w-full" type="text" name="' . $ele_name . '" id="' . $ele_name | ||
| . '" size="' . $element->getSize() . '" maxlength="' . $element->getMaxlength() | ||
| . '" value="' . $display_value . '"' . $element->getExtra() . '>' | ||
| . '<button class="btn btn-neutral join-item" type="button"' | ||
| . ' onclick="return showCalendar(\'' . $ele_name . '\');">' | ||
| . '<i class="fa-solid fa-calendar" aria-hidden="true"></i></button>' | ||
| . '</div>'; | ||
| } | ||
|
|
||
| /** | ||
| * Render support for XoopsThemeForm | ||
| * | ||
| * @param XoopsThemeForm $form form to render | ||
| * | ||
| * @return string rendered form | ||
| */ | ||
| public function renderThemeForm(XoopsThemeForm $form) | ||
| { | ||
| $ele_name = $form->getName(); | ||
|
|
||
| $ret = '<div class="card bg-base-100 shadow">'; | ||
| $ret .= '<form name="' . $ele_name . '" id="' . $ele_name . '" action="' | ||
| . $form->getAction() . '" method="' . $form->getMethod() | ||
| . '" onsubmit="return xoopsFormValidate_' . $ele_name . '();"' . $form->getExtra() | ||
| . ' class="card-body">' | ||
| . '<h3 class="card-title">' . $form->getTitle() . '</h3>'; | ||
| $hidden = ''; | ||
|
|
||
| foreach ($form->getElements() as $element) { | ||
| if (!is_object($element)) { // see $form->addBreak() | ||
| $ret .= $element; | ||
| continue; | ||
| } | ||
| if ($element->isHidden()) { | ||
| $hidden .= $element->render(); | ||
| continue; | ||
| } | ||
|
|
||
| $ret .= '<div class="form-control w-full mb-4 grid grid-cols-1 md:grid-cols-12 gap-2 md:items-start">'; | ||
| if (($caption = $element->getCaption()) != '') { | ||
| $ret .= '<label for="' . $element->getName() . '" class="label md:col-span-3 md:justify-end">' | ||
| . '<span class="label-text">' . $element->getCaption() | ||
| . ($element->isRequired() ? '<span class="text-error ms-1">*</span>' : '') | ||
| . '</span></label>'; | ||
| } else { | ||
| $ret .= '<div class="md:col-span-3"></div>'; | ||
| } | ||
| $ret .= '<div class="md:col-span-9">'; | ||
| $ret .= $element->render(); | ||
| if (($desc = $element->getDescription()) != '') { | ||
| $ret .= '<div class="label"><span class="label-text-alt text-base-content/60">' . $desc . '</span></div>'; | ||
| } | ||
| $ret .= '</div>'; | ||
| $ret .= '</div>'; | ||
| } | ||
| if (count($form->getRequired()) > 0) { | ||
| $ret .= NWLINE . '<div class="text-sm text-base-content/60 mt-2"><span class="text-error">*</span> = ' . _REQUIRED . '</div>' . NWLINE; | ||
| } | ||
| $ret .= $hidden; | ||
| $ret .= '</form></div>'; | ||
| $ret .= $form->renderValidationJS(true); | ||
|
|
||
| return $ret; | ||
| } | ||
|
|
||
| /** | ||
| * Support for themed addBreak | ||
| * | ||
| * @param XoopsThemeForm $form | ||
| * @param string $extra pre-rendered content for break row | ||
| * @param string $class class for row | ||
| * | ||
| * @return void | ||
| */ | ||
| public function addThemeFormBreak(XoopsThemeForm $form, $extra, $class) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add @throws tags to new public method PHPDoc blocks.
Public methods include @param and @return, but @throws is missing throughout the new class.
As per coding guidelines: "New public methods must have PHPDoc with @param, @return, and @throws tags."
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[failure] 123-123: Define a constant instead of duplicating this literal " checked" 3 times.
[failure] 414-414: Refactor this function to reduce its Cognitive Complexity from 16 to the 15 allowed.
[failure] 44-44: Define a constant instead of duplicating this literal "" title="" 5 times.
[failure] 236-236: Define a constant instead of duplicating this literal "' title='" 3 times.
[failure] 118-118: Define a constant instead of duplicating this literal "-primary' type='" 3 times.
[failure] 292-292: Define a constant instead of duplicating this literal "");' title='" 5 times.
[warning] 526-526: Remove the literal "false" boolean value.
[failure] 291-291: Define a constant instead of duplicating this literal "btn btn-neutral btn-sm" 3 times.
[warning] 698-698: Remove this unused "$caption" local variable.
[failure] 67-67: Define a constant instead of duplicating this literal "" id="" 7 times.
[failure] 120-120: Define a constant instead of duplicating this literal "' value='" 4 times.
🪛 PHPMD (2.15.0)
[error] 207-219: renderFormColorPicker accesses the super-global variable $GLOBALS. (undefined)
(Superglobals)
[error] 207-219: renderFormColorPicker accesses the super-global variable $GLOBALS. (undefined)
(Superglobals)
[error] 207-219: renderFormColorPicker accesses the super-global variable $GLOBALS. (undefined)
(Superglobals)
[error] 212-215: The method renderFormColorPicker uses an else expression. Else clauses are basically not necessary and you can simplify the code by not using them. (undefined)
(ElseExpression)
[error] 228-278: renderFormDhtmlTextArea accesses the super-global variable $GLOBALS. (undefined)
(Superglobals)
[error] 228-278: renderFormDhtmlTextArea accesses the super-global variable $GLOBALS. (undefined)
(Superglobals)
[error] 228-278: renderFormDhtmlTextArea accesses the super-global variable $GLOBALS. (undefined)
(Superglobals)
[error] 245-250: The method renderFormDhtmlTextArea uses an else expression. Else clauses are basically not necessary and you can simplify the code by not using them. (undefined)
(ElseExpression)
[error] 261-261: The variable $javascript_file is not named in camelCase. (undefined)
(CamelCaseVariableName)
[error] 262-262: The variable $javascript_file_element is not named in camelCase. (undefined)
(CamelCaseVariableName)
[warning] 262-262: Avoid excessively long variable names like $javascript_file_element. Keep variable name length under 20. (undefined)
(LongVariable)
[error] 289-289: The variable $textarea_id is not named in camelCase. (undefined)
(CamelCaseVariableName)
[error] 298-298: Avoid using static access to class '\MyTextSanitizer' in method 'renderFormDhtmlTAXoopsCode'. (undefined)
(StaticAccess)
[warning] 304-304: Avoid variables with short names like $js. Configured minimum length is 3. (undefined)
(ShortVariable)
[error] 320-320: Avoid using static access to class 'XoopsPreload' in method 'renderFormDhtmlTAXoopsCode'. (undefined)
(StaticAccess)
[error] 333-405: renderFormDhtmlTATypography accesses the super-global variable $GLOBALS. (undefined)
(Superglobals)
[error] 333-405: renderFormDhtmlTATypography accesses the super-global variable $GLOBALS. (undefined)
(Superglobals)
[error] 333-405: renderFormDhtmlTATypography accesses the super-global variable $GLOBALS. (undefined)
(Superglobals)
[error] 335-335: The variable $textarea_id is not named in camelCase. (undefined)
(CamelCaseVariableName)
[warning] 414-443: The method renderFormElementTray() has a Cyclomatic Complexity of 10. The configured cyclomatic complexity threshold is 10. (undefined)
(CyclomaticComplexity)
[error] 520-520: The variable $ele_name is not named in camelCase. (undefined)
(CamelCaseVariableName)
[error] 521-521: The variable $ele_title is not named in camelCase. (undefined)
(CamelCaseVariableName)
[error] 522-522: The variable $ele_value is not named in camelCase. (undefined)
(CamelCaseVariableName)
[error] 523-523: The variable $ele_options is not named in camelCase. (undefined)
(CamelCaseVariableName)
[error] 529-531: The method renderFormSelect uses an else expression. Else clauses are basically not necessary and you can simplify the code by not using them. (undefined)
(ElseExpression)
[error] 581-666: renderFormTextDateSelect accesses the super-global variable $GLOBALS. (undefined)
(Superglobals)
[error] 581-666: renderFormTextDateSelect accesses the super-global variable $GLOBALS. (undefined)
(Superglobals)
[error] 581-666: renderFormTextDateSelect accesses the super-global variable $GLOBALS. (undefined)
(Superglobals)
[error] 581-666: renderFormTextDateSelect accesses the super-global variable $GLOBALS. (undefined)
(Superglobals)
[error] 581-666: renderFormTextDateSelect accesses the super-global variable $GLOBALS. (undefined)
(Superglobals)
[error] 581-666: renderFormTextDateSelect accesses the super-global variable $GLOBALS. (undefined)
(Superglobals)
[error] 581-666: renderFormTextDateSelect accesses the super-global variable $GLOBALS. (undefined)
(Superglobals)
[error] 586-588: The method renderFormTextDateSelect uses an else expression. Else clauses are basically not necessary and you can simplify the code by not using them. (undefined)
(ElseExpression)
[error] 590-590: The variable $ele_name is not named in camelCase. (undefined)
(CamelCaseVariableName)
[error] 591-591: The variable $ele_value is not named in camelCase. (undefined)
(CamelCaseVariableName)
[error] 593-593: The variable $display_value is not named in camelCase. (undefined)
(CamelCaseVariableName)
[error] 598-600: The method renderFormTextDateSelect uses an else expression. Else clauses are basically not necessary and you can simplify the code by not using them. (undefined)
(ElseExpression)
[error] 675-722: Avoid assigning values to variables in if clauses and the like (line '698', column '18'). (undefined)
(IfStatementAssignment)
[error] 675-722: Avoid assigning values to variables in if clauses and the like (line '708', column '18'). (undefined)
(IfStatementAssignment)
[error] 677-677: The variable $ele_name is not named in camelCase. (undefined)
(CamelCaseVariableName)
[warning] 698-698: Avoid unused local variables such as '$caption'. (undefined)
(UnusedLocalVariable)
[error] 703-705: The method renderThemeForm uses an else expression. Else clauses are basically not necessary and you can simplify the code by not using them. (undefined)
(ElseExpression)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/class/xoopsform/renderer/XoopsFormRendererTailwind.php` around lines
33 - 733, Add missing `@throws` tags to all new public method PHPDoc blocks in
this class (e.g. renderFormButton, renderFormButtonTray, renderFormCheckBox,
renderFormColorPicker, renderFormDhtmlTextArea, renderFormRadio,
renderFormSelect, renderFormText, renderFormTextArea, renderFormTextDateSelect,
renderThemeForm, renderFormFile, renderFormLabel, renderFormPassword,
renderFormElementTray and addThemeFormBreak) by updating each method's PHPDoc to
include a `@throws` entry (use a generic Exception class such as \Exception or a
more specific exception if applicable) so every public method contains `@param`,
`@return` and `@throws` tags per coding guidelines.
There was a problem hiding this comment.
Pull request overview
This PR adds the core runtime/assets needed to support Tailwind CSS–based XOOPS themes, including a new Tailwind/DaisyUI form renderer and a shared Alpine.js runtime under xoops_lib/Frameworks/.
Changes:
- Add Alpine.js runtime directory under
htdocs/xoops_lib/Frameworks/alpine/with documentation and a bundledalpine.min.js. - Introduce
XoopsFormRendererTailwind(Tailwind CSS + DaisyUI output) and register it in the core class map. - Add a security
index.phpto prevent direct access to the Alpine framework directory.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
htdocs/xoops_lib/Frameworks/alpine/README.md |
Documents how to install and reference Alpine.js via browse.php. |
htdocs/xoops_lib/Frameworks/alpine/index.php |
Blocks direct access to the framework directory. |
htdocs/xoops_lib/Frameworks/alpine/alpine.min.js |
Adds the Alpine.js runtime file for theme usage. |
htdocs/class/xoopsload.php |
Registers the Tailwind form renderer in the core class map. |
htdocs/class/xoopsform/renderer/XoopsFormRendererTailwind.php |
Implements a Tailwind/DaisyUI form renderer for XOOPS forms. |
Comment on lines
+42
to
+46
| return '<button type="' . $element->getType() . '"' | ||
| . ' class="btn btn-neutral" name="' . $element->getName() . '"' | ||
| . ' id="' . $element->getName() . '" title="' . $element->getValue() . '"' | ||
| . ' value="' . $element->getValue() . '"' | ||
| . $element->getExtra() . '>' . $element->getValue() . '</button>'; |
There was a problem hiding this comment.
HTML attributes are built from $element->getType()/getName()/getValue()/getExtra() without escaping. If any of these contain quotes or user-controlled content, it can break the markup and potentially enable XSS. Please apply htmlspecialchars(..., ENT_QUOTES|ENT_HTML5) (and consider sanitizing/whitelisting getExtra() usage) when interpolating into attributes and element text.
| */ | ||
| public function renderFormLabel(XoopsFormLabel $element) | ||
| { | ||
| return '<label class="label label-text" id="' . $element->getName() . '">' . $element->getValue(); |
There was a problem hiding this comment.
renderFormLabel() returns an opening but never closes it. This produces invalid HTML and can cause layout/DOM issues; please close the tag (or switch to a /
if a label is not intended).
Suggested change
| return '<label class="label label-text" id="' . $element->getName() . '">' . $element->getValue(); | |
| return '<label class="label label-text" id="' . $element->getName() . '">' . $element->getValue() . '</label>'; |
Comment on lines
+416
to
+419
| $count = 0; | ||
| $inline = (\XoopsFormElementTray::ORIENTATION_VERTICAL === $element->getOrientation()); | ||
| $ret = $inline ? '<div class="flex flex-wrap items-center gap-2">' : '<div class="space-y-2">'; | ||
| foreach ($element->getElements() as $ele) { |
There was a problem hiding this comment.
The orientation handling in renderFormElementTray() appears inverted relative to XoopsFormElementTray::getOrientation() docs (ORIENTATION_VERTICAL is documented as “up and down”). Currently, ORIENTATION_VERTICAL selects the inline/flex layout. Please align the condition and wrapper classes so vertical orientation stacks elements and horizontal orientation lays them out inline.
Comment on lines
+31
to
+40
| class XoopsFormRendererTailwind implements XoopsFormRendererInterface | ||
| { | ||
| /** | ||
| * Render support for XoopsFormButton | ||
| * | ||
| * @param XoopsFormButton $element form element | ||
| * | ||
| * @return string rendered form element | ||
| */ | ||
| public function renderFormButton(XoopsFormButton $element) |
There was a problem hiding this comment.
A new renderer implementation is being added, but there are existing PHPUnit tests for form rendering infrastructure (e.g., XoopsFormRendererTest) that don’t exercise this renderer. Please add a small unit test that instantiates XoopsFormRendererTailwind and asserts key outputs for at least one element (and/or that it can be set on the singleton) to prevent regressions.
Centralize HTML escaping via esc() helper applied to all attribute output. Strip tags from getExtra() to prevent injection. Fix four correctness bugs: renderFormLabel missing </label>, element tray orientation reversed, renderFormColorPicker echoing side-effects, element render() output capture. Collapse three renderChecked* variants into one method via switch on columns. Extract constants for duplicated literals. Add copyright header and XOOPS_ROOT_PATH guard. Add PHPUnit test file covering XSS escaping, label closing, and tray orientation. Fix Alpine README fenced block language hint.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@htdocs/class/xoopsform/renderer/XoopsFormRendererTailwind.php`:
- Around line 595-600: In XoopsFormRendererTailwind.php update the select-option
matching to use strict comparison: inside the loop where $selected is computed
(currently using in_array($optValue, $value)), change that call to
in_array($optValue, $value, true) so the selected state uses strict type
comparison (keep the existing is_array($value) && count($value) > 0 check and
only replace the in_array call).
- Around line 207-224: The option label $name is output unescaped into the span
causing XSS risk; update the label rendering in XoopsFormRendererTailwind
(inside the loop over $element->getOptions()) to escape the visible text using
the existing esc helper (and optionally strip tags) instead of raw $name — e.g.
replace the span content that currently uses $name with
$this->esc(strip_tags((string)$name)) (keeping $delimeter unchanged) so the
output uses $this->esc(...) and prevents injection while preserving the rest of
the input construction ($inputId, $checked, $extra, etc.).
- Around line 234-241: The isOptionChecked helper currently uses
in_array($optionValue, $current) which allows loose comparisons and can mis-mark
values like 0 or '' as checked; update the in_array call inside isOptionChecked
to use strict comparison (third param true) so the check becomes
in_array($optionValue, $current, true), keeping the existing string cast
fallback for non-array $current unchanged.
In `@tests/unit/htdocs/class/xoopsforms/XoopsFormRendererTailwindTest.php`:
- Around line 13-18: The test currently calls xoops_load('XoopsFormElement')
etc. which breaks the isolation rule; either document that xoopsforms tests are
exempt or remove xoops_load dependencies by autoloading or mocking the required
classes: update XoopsFormRendererTailwindTest to stop calling xoops_load and
instead require or autoload minimal class stubs/mocks for XoopsFormElement,
XoopsFormButton, XoopsFormElementTray, XoopsFormLabel,
XoopsFormRendererInterface and XoopsFormRendererTailwind (or use class_exists
checks and fallback stubs) so the test can run without a XOOPS installation and
matches the isolation pattern used by other unit tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 83804f9e-49ec-4e2d-88b5-0e214a177d02
📒 Files selected for processing (3)
htdocs/class/xoopsform/renderer/XoopsFormRendererTailwind.phphtdocs/xoops_lib/Frameworks/alpine/README.mdtests/unit/htdocs/class/xoopsforms/XoopsFormRendererTailwindTest.php
Comment on lines
+753
to
+759
|
|
||
| $ret = '<div class="card bg-base-100 shadow">'; | ||
| $ret .= '<form name="' . $formName . '" id="' . $formName . '"' | ||
| . ' action="' . $this->esc($form->getAction()) . '"' | ||
| . ' method="' . $this->esc($form->getMethod()) . '"' | ||
| . ' onsubmit="return xoopsFormValidate_' . $formName . '();"' | ||
| . $this->renderExtra($form) |
There was a problem hiding this comment.
renderThemeForm() passes $form (a XoopsThemeForm / XoopsForm) into renderExtra(), but renderExtra() is type-hinted to XoopsFormElement. This will throw a TypeError at runtime when rendering a theme form. Consider widening renderExtra() to accept both XoopsFormElement and XoopsForm (or extracting a separate helper for form extras) and ensure the returned string preserves the leading-space semantics of getExtra().
Suggested change
| $ret = '<div class="card bg-base-100 shadow">'; | |
| $ret .= '<form name="' . $formName . '" id="' . $formName . '"' | |
| . ' action="' . $this->esc($form->getAction()) . '"' | |
| . ' method="' . $this->esc($form->getMethod()) . '"' | |
| . ' onsubmit="return xoopsFormValidate_' . $formName . '();"' | |
| . $this->renderExtra($form) | |
| $extra = (string) $form->getExtra(); | |
| $formExtra = ''; | |
| if ($extra !== '') { | |
| $formExtra = preg_match('/^\s/', $extra) ? $extra : ' ' . $extra; | |
| } | |
| $ret = '<div class="card bg-base-100 shadow">'; | |
| $ret .= '<form name="' . $formName . '" id="' . $formName . '"' | |
| . ' action="' . $this->esc($form->getAction()) . '"' | |
| . ' method="' . $this->esc($form->getMethod()) . '"' | |
| . ' onsubmit="return xoopsFormValidate_' . $formName . '();"' | |
| . $formExtra |
| $count = 0; | ||
| foreach ($element->getElements() as $ele) { | ||
| if ($count > 0 && !$isVertical) { | ||
| $ret .= $this->esc($element->getDelimeter()); |
There was a problem hiding this comment.
renderFormElementTray() escapes $element->getDelimeter() via esc(). Other renderers output the delimiter verbatim, and delimiters commonly contain markup like <br>; escaping will change behavior (e.g., rendering <br> text). Use the delimiter as raw HTML (or a dedicated, well-documented escaping/allowlist strategy) to preserve expected output.
Suggested change
| $ret .= $this->esc($element->getDelimeter()); | |
| $ret .= (string) $element->getDelimeter(); |
Comment on lines
+211
to
+219
| $ret .= '<label class="' . $labelCls . '">' | ||
| . '<input class="' . $type . ' ' . $type . '-primary" type="' . $type . '"' | ||
| . ' name="' . $this->esc($elementName) . '"' | ||
| . ' id="' . $inputId . '"' | ||
| . ' title="' . $this->esc(strip_tags((string) $name)) . '"' | ||
| . ' value="' . $this->esc($value) . '"' | ||
| . $checked . $extra . '>' | ||
| . '<span class="label-text">' . $name . $delimeter . '</span>' | ||
| . '</label>'; |
There was a problem hiding this comment.
In renderChecked(), the option label $name is inserted into the DOM unescaped (<span class="label-text">...). Since option names can come from dynamic sources (DB/module code), this allows HTML/JS injection in checkbox/radio labels, while other parts of this renderer explicitly escape values for XSS defense. Escape the label text (and consider whether the delimiter should be treated as raw or escaped) to make output consistently safe.
| } | ||
| $ret .= '<div class="md:col-span-9">'; | ||
| $ret .= $this->renderElementHtml($element); | ||
| $desc = $element->getDescription(); |
There was a problem hiding this comment.
renderThemeForm() outputs $element->getDescription() without escaping. If descriptions can contain untrusted content (e.g., derived from DB/config), this can introduce XSS in the Tailwind renderer even though most other attributes/text are escaped. Consider using the encoded form (getDescription(true)) or explicitly escaping here, or clearly documenting that descriptions are expected to contain trusted HTML.
Suggested change
| $desc = $element->getDescription(); | |
| $desc = $element->getDescription(true); |
Comment on lines
+9
to
+21
| ## Installation | ||
|
|
||
| Download the latest minified build from the [Alpine.js releases](https://github.com/alpinejs/alpine/releases) and place it here: | ||
|
|
||
| ```text | ||
| xoops_lib/Frameworks/alpine/alpine.min.js | ||
| ``` | ||
|
|
||
| Or via CDN download: | ||
|
|
||
| ```bash | ||
| curl -sL "https://cdn.jsdelivr.net/npm/alpinejs@3/dist/cdn.min.js" -o alpine.min.js | ||
| ``` |
There was a problem hiding this comment.
This README instructs users to download Alpine.js and place it in this directory, but this PR already adds alpine.min.js to the repo. Please clarify whether Alpine is intended to be shipped/vendored (and how to update it, including the pinned version) or whether the file should be excluded and the README used as the installation step, to avoid confusing/inconsistent setup instructions.
Comment on lines
+20
to
+28
| /** | ||
| * Unit tests for XoopsFormRendererTailwind. | ||
| * | ||
| * Focus areas: | ||
| * - renderer can be instantiated | ||
| * - HTML attributes are escaped (XSS defense) | ||
| * - renderFormLabel produces a properly closed label element | ||
| * - renderFormElementTray picks the correct container class for orientation | ||
| */ |
There was a problem hiding this comment.
The Tailwind renderer introduces substantial new rendering behavior, but the accompanying unit test only covers a few methods (button, label, tray orientation). Adding targeted tests for renderThemeForm() (to cover form-level extras/hidden handling) and for checkbox/radio option label escaping would help prevent regressions and would have caught the current renderExtra($form) type mismatch.
…sons Critical: renderExtra was typed XoopsFormElement but called with XoopsForm, causing TypeError in renderThemeForm. Widened to accept any object with getExtra(). Escape option labels in renderChecked to prevent XSS. Use strict in_array comparison in isOptionChecked and select rendering. Escape element description in renderThemeForm. Add tests for checkbox option escaping and renderThemeForm. Update Alpine README to reflect vendored alpine.min.js.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
tests/unit/htdocs/class/xoopsforms/XoopsFormRendererTailwindTest.php (1)
13-18:⚠️ Potential issue | 🟠 MajorKeep this unit test isolated from XOOPS runtime loaders.
Top-level
xoops_load()calls couple the test to XOOPS runtime state, which breaks unit-test isolation expectations.Based on learnings: "Applies to tests/**/*.php : Tests must be fully isolated with no XOOPS installation required for unit tests".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/htdocs/class/xoopsforms/XoopsFormRendererTailwindTest.php` around lines 13 - 18, The test contains top-level xoops_load() calls (for XoopsFormElement, XoopsFormButton, XoopsFormElementTray, XoopsFormLabel, XoopsFormRendererInterface, XoopsFormRendererTailwind) which couples it to XOOPS runtime; remove those top-level calls and instead ensure isolation by loading or providing test doubles inside the test lifecycle (e.g., move loading into XoopsFormRendererTailwindTest::setUp() using require_once of local test stubs or by creating lightweight mock classes when class_exists() is false), or register a simple autoloader for the test namespace; update XoopsFormRendererTailwindTest to reference those local stubs/mocks so the test runs without any xoops_load() dependency.htdocs/class/xoopsform/renderer/XoopsFormRendererTailwind.php (1)
103-820:⚠️ Potential issue | 🟠 MajorAdd
@throwstags to all new public method PHPDoc blocks.Public methods currently document
@paramand@returnbut omit@throws.As per coding guidelines: "New public methods must have PHPDoc with
@param,@return, and@throwstags."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/class/xoopsform/renderer/XoopsFormRendererTailwind.php` around lines 103 - 820, Update the PHPDoc for every new public method (e.g. renderFormButton, renderFormButtonTray, renderFormCheckBox, renderFormColorPicker, renderFormDhtmlTextArea, renderFormElementTray, renderFormFile, renderFormLabel, renderFormPassword, renderFormRadio, renderFormSelect, renderFormText, renderFormTextArea, renderFormTextDateSelect, renderThemeForm, addThemeFormBreak) to include an `@throws` tag; list the specific exception type(s) the method can throw (use the real exception class if known from called code) or add a generic `@throws` \Exception as a placeholder and document when callers should expect it, then run a quick grep to ensure every public method PHPDoc now contains `@param`, `@return` and `@throws`.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@htdocs/class/xoopsform/renderer/XoopsFormRendererTailwind.php`:
- Around line 22-35: The class docblock for XoopsFormRendererTailwind is missing
required XOOPS metadata tags; update the docblock above the
XoopsFormRendererTailwind class to include `@category`, `@package`, `@author`,
`@copyright`, `@license`, and `@link` tags with appropriate values (e.g., category
like "XoopsForm", package as the module or subsystem name, author name/email,
copyright statement, SPDX or URL license identifier, and a reference link to the
project or file) so the header meets the coding guidelines.
- Around line 659-663: The dynamic language include uses
$GLOBALS['xoopsConfig']['language'] directly which can allow directory
traversal; update the include logic around the include_once lines to
canonicalize and validate the resolved path using realpath before including:
compute the candidate path by joining XOOPS_ROOT_PATH . '/language/' and the
language value, call realpath() on the candidate and on the language directory
(XOOPS_ROOT_PATH . '/language'), ensure the candidate realpath startsWith the
language directory realpath and that the language token matches a safe pattern
(e.g., only letters/numbers/underscore), and only then include it; if validation
fails or realpath returns false, fall back to the hardcoded english calendar
include to prevent unsafe inclusion (apply these checks to both the calendar.php
candidate and the fallback logic in XoopsFormRendererTailwind.php).
In `@tests/unit/htdocs/class/xoopsforms/XoopsFormRendererTailwindTest.php`:
- Around line 1-29: Add the required XOOPS file header at the very top of this
file and update the docblock for the class XoopsFormRendererTailwindTest to
include the mandatory tags: `@category`, `@package`, `@author`, `@copyright`, `@license`,
and `@link`; ensure the header block appears before the declare(strict_types=1)
line and that the class-level docblock (above "class
XoopsFormRendererTailwindTest extends TestCase") contains those tags with
appropriate project/licensing values consistent with other XOOPS files.
---
Duplicate comments:
In `@htdocs/class/xoopsform/renderer/XoopsFormRendererTailwind.php`:
- Around line 103-820: Update the PHPDoc for every new public method (e.g.
renderFormButton, renderFormButtonTray, renderFormCheckBox,
renderFormColorPicker, renderFormDhtmlTextArea, renderFormElementTray,
renderFormFile, renderFormLabel, renderFormPassword, renderFormRadio,
renderFormSelect, renderFormText, renderFormTextArea, renderFormTextDateSelect,
renderThemeForm, addThemeFormBreak) to include an `@throws` tag; list the specific
exception type(s) the method can throw (use the real exception class if known
from called code) or add a generic `@throws` \Exception as a placeholder and
document when callers should expect it, then run a quick grep to ensure every
public method PHPDoc now contains `@param`, `@return` and `@throws`.
In `@tests/unit/htdocs/class/xoopsforms/XoopsFormRendererTailwindTest.php`:
- Around line 13-18: The test contains top-level xoops_load() calls (for
XoopsFormElement, XoopsFormButton, XoopsFormElementTray, XoopsFormLabel,
XoopsFormRendererInterface, XoopsFormRendererTailwind) which couples it to XOOPS
runtime; remove those top-level calls and instead ensure isolation by loading or
providing test doubles inside the test lifecycle (e.g., move loading into
XoopsFormRendererTailwindTest::setUp() using require_once of local test stubs or
by creating lightweight mock classes when class_exists() is false), or register
a simple autoloader for the test namespace; update XoopsFormRendererTailwindTest
to reference those local stubs/mocks so the test runs without any xoops_load()
dependency.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f6868861-9694-4c2d-b5a2-a52024128a68
📒 Files selected for processing (3)
htdocs/class/xoopsform/renderer/XoopsFormRendererTailwind.phphtdocs/xoops_lib/Frameworks/alpine/README.mdtests/unit/htdocs/class/xoopsforms/XoopsFormRendererTailwindTest.php
Comment on lines
+22
to
+26
| /** | ||
| * Tailwind CSS + DaisyUI form renderer | ||
| * | ||
| * Renders XOOPS form elements using Tailwind CSS utility classes combined with | ||
| * DaisyUI component classes (.btn, .input, .select, .textarea, .checkbox, .radio, |
There was a problem hiding this comment.
This new renderer’s class docblock doesn’t follow the renderer docblock pattern used by the existing Bootstrap renderers (e.g., @category/@package/@link tags). For consistency with the rest of htdocs/class/xoopsform/renderer/*, consider adding a full class docblock with the standard tags.
Guard configs property access with property_exists + is_array check. Simplify renderFormSelect — getValue always returns array. Reorder renderFormTextDateSelect branches: blank → numeric → literal. Widen renderExtra PHPDoc to accept any object with getExtra(). Make renderElementHtml defensive against non-string render returns.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
htdocs/class/xoopsform/renderer/XoopsFormRendererTailwind.php (1)
671-675:⚠️ Potential issue | 🟠 MajorPath traversal vulnerability remains unaddressed.
The language config value is used directly in an
include_oncepath withoutrealpath()validation or sanitization. A malformed$GLOBALS['xoopsConfig']['language']value like../../etc/passwdcould be exploited.🔧 Recommended hardening
+ $language = preg_replace('/[^a-zA-Z0-9_]/', '', (string) ($GLOBALS['xoopsConfig']['language'] ?? 'english')); + $languageBase = realpath(XOOPS_ROOT_PATH . '/language'); + $calendarPath = realpath(XOOPS_ROOT_PATH . '/language/' . $language . '/calendar.php'); + if ($languageBase !== false + && $calendarPath !== false + && str_starts_with($calendarPath, $languageBase . DIRECTORY_SEPARATOR) + ) { + include_once $calendarPath; + } else { + include_once XOOPS_ROOT_PATH . '/language/english/calendar.php'; + } - if (file_exists(XOOPS_ROOT_PATH . '/language/' . $GLOBALS['xoopsConfig']['language'] . '/calendar.php')) { - include_once XOOPS_ROOT_PATH . '/language/' . $GLOBALS['xoopsConfig']['language'] . '/calendar.php'; - } else { - include_once XOOPS_ROOT_PATH . '/language/english/calendar.php'; - }As per coding guidelines: "Validate file paths with realpath() and boundary checks to prevent directory traversal."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/class/xoopsform/renderer/XoopsFormRendererTailwind.php` around lines 671 - 675, The include_once calls use $GLOBALS['xoopsConfig']['language'] directly, allowing path traversal; change this to validate and constrain the language value before building the path: normalize the input (e.g., use basename or a strict whitelist/allowedLanguages array), reject or fallback on invalid values, construct the target path with XOOPS_ROOT_PATH . '/language/' . $lang . '/calendar.php', call realpath() on that constructed path and verify it resides under realpath(XOOPS_ROOT_PATH . '/language') (and that the resolved path is a file and readable) before performing include_once; update the logic around the include_once lines in XoopsFormRendererTailwind.php to use the sanitized $lang and realpath checks and fall back to 'english' if validation fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@htdocs/class/xoopsform/renderer/XoopsFormRendererTailwind.php`:
- Around line 104-105: The current stripping of '<' and '>' in
XoopsFormRendererTailwind.php corrupts legitimate attribute values (e.g.,
onclick handlers); instead of removing characters in the $extra assignment (used
by getExtra()), HTML-encode them so attributes remain valid while preventing tag
injection — replace the str_replace approach with an HTML-encoding call that
converts < and > (and quotes) to entities using UTF-8 (e.g., via
htmlspecialchars/htmlentities with appropriate flags) and ensure getExtra()
consumers expect encoded output; alternatively, if you must keep stripping, add
a clear doc comment on XoopsFormRendererTailwind::getExtra() explaining the
limitation so module authors pre-encode extras.
- Around line 604-608: The conditional uses an overly defensive comparison;
change the check in XoopsFormRendererTailwind (around the select element
rendering where $element is used) from `$element->isMultiple() !== false` to
simply `$element->isMultiple()` so the multiple branch and the single-selection
branch remain the same (ensure the branches that build the name/id/title
attributes and add `[]` and `multiple="multiple"` still reference $element,
$name and $title as before).
- Line 228: The span output in XoopsFormRendererTailwind.php concatenates
$delimeter without escaping, creating an XSS risk; update the output to escape
the delimiter using the renderer's esc method (e.g., use $this->esc((string)
$delimeter)) where the span is built in the class (look for the code that builds
'. '<span class="label-text">' . $this->esc((string) $name) . $delimeter .
'</span>'); ensure the delimiter is cast to string before escaping to avoid type
issues.
- Around line 420-422: The loop over $GLOBALS['formtextdhtml_sizes'] can trigger
a warning if that global is undefined; update XoopsFormRendererTailwind.php to
first obtain a safe local array (e.g. $sizes =
isset($GLOBALS['formtextdhtml_sizes']) &&
is_array($GLOBALS['formtextdhtml_sizes']) ? $GLOBALS['formtextdhtml_sizes'] :
[]) and then iterate over $sizes when building $fontStr (preserving the existing
$this->esc(), $textarea_id and $hiddentext usage), or alternatively wrap the
foreach in an if-check like if (!empty($GLOBALS['formtextdhtml_sizes']) &&
is_array(...)) to guard against undefined globals.
- Around line 831-835: The addThemeFormBreak method in XoopsFormRendererTailwind
outputs $extra directly which risks XSS; wrap $extra with HTML-escaping (e.g.,
htmlspecialchars($extra, ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8')) before passing
it to XoopsThemeForm::addElement so only safe markup is rendered, and keep the
rest of the markup concatenation unchanged; also tighten the preg_replace call
in addThemeFormBreak by removing the unnecessary 'i' modifier and the redundant
space in the character class (use '/[^A-Za-z0-9\s_-]/' or equivalent) to avoid
flagged duplicates.
---
Duplicate comments:
In `@htdocs/class/xoopsform/renderer/XoopsFormRendererTailwind.php`:
- Around line 671-675: The include_once calls use
$GLOBALS['xoopsConfig']['language'] directly, allowing path traversal; change
this to validate and constrain the language value before building the path:
normalize the input (e.g., use basename or a strict whitelist/allowedLanguages
array), reject or fallback on invalid values, construct the target path with
XOOPS_ROOT_PATH . '/language/' . $lang . '/calendar.php', call realpath() on
that constructed path and verify it resides under realpath(XOOPS_ROOT_PATH .
'/language') (and that the resolved path is a file and readable) before
performing include_once; update the logic around the include_once lines in
XoopsFormRendererTailwind.php to use the sanitized $lang and realpath checks and
fall back to 'english' if validation fails.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 89cb2f08-adbe-4bd3-80b0-a8eb09e98470
📒 Files selected for processing (1)
htdocs/class/xoopsform/renderer/XoopsFormRendererTailwind.php
| . ' title="' . $this->esc(strip_tags((string) $name)) . '"' | ||
| . ' value="' . $this->esc($value) . '"' | ||
| . $checked . $extra . '>' | ||
| . '<span class="label-text">' . $this->esc((string) $name) . $delimeter . '</span>' |
There was a problem hiding this comment.
Escape $delimeter to prevent XSS.
The delimiter is output directly into HTML without escaping. If a module sets a delimiter containing HTML characters, this becomes an injection point.
- . '<span class="label-text">' . $this->esc((string) $name) . $delimeter . '</span>'
+ . '<span class="label-text">' . $this->esc((string) $name) . $this->esc($delimeter) . '</span>'📝 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.
Suggested change
| . '<span class="label-text">' . $this->esc((string) $name) . $delimeter . '</span>' | |
| . '<span class="label-text">' . $this->esc((string) $name) . $this->esc($delimeter) . '</span>' |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/class/xoopsform/renderer/XoopsFormRendererTailwind.php` at line 228,
The span output in XoopsFormRendererTailwind.php concatenates $delimeter without
escaping, creating an XSS risk; update the output to escape the delimiter using
the renderer's esc method (e.g., use $this->esc((string) $delimeter)) where the
span is built in the class (look for the code that builds '. '<span
class="label-text">' . $this->esc((string) $name) . $delimeter . '</span>');
ensure the delimiter is cast to string before escaping to avoid type issues.
Comment on lines
+831
to
+835
| public function addThemeFormBreak(XoopsThemeForm $form, $extra, $class) | ||
| { | ||
| $class = ($class != '') ? preg_replace('/[^A-Za-z0-9\s_-]/i', '', $class) : ''; | ||
| $form->addElement('<div class="divider col-span-full ' . $class . '"><span class="font-semibold">' . $extra . '</span></div>'); | ||
| } |
There was a problem hiding this comment.
Escape $extra content to prevent XSS.
The $extra parameter is rendered directly into HTML without escaping. While the docblock describes it as "pre-rendered content", if any upstream code passes unsanitized user input, this creates an XSS vector.
public function addThemeFormBreak(XoopsThemeForm $form, $extra, $class)
{
- $class = ($class != '') ? preg_replace('/[^A-Za-z0-9\s_-]/i', '', $class) : '';
- $form->addElement('<div class="divider col-span-full ' . $class . '"><span class="font-semibold">' . $extra . '</span></div>');
+ $class = ($class !== '') ? preg_replace('/[^A-Za-z0-9\s_-]/', '', $class) : '';
+ $form->addElement('<div class="divider col-span-full ' . $class . '"><span class="font-semibold">' . $this->esc($extra) . '</span></div>');
}Note: The i modifier is unnecessary since the character class A-Za-z already covers both cases, and SonarCloud flags a duplicate in the class (\s contains space).
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 833-833: Remove duplicates in this character class.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/class/xoopsform/renderer/XoopsFormRendererTailwind.php` around lines
831 - 835, The addThemeFormBreak method in XoopsFormRendererTailwind outputs
$extra directly which risks XSS; wrap $extra with HTML-escaping (e.g.,
htmlspecialchars($extra, ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8')) before passing
it to XoopsThemeForm::addElement so only safe markup is rendered, and keep the
rest of the markup concatenation unchanged; also tighten the preg_replace call
in addThemeFormBreak by removing the unnecessary 'i' modifier and the redundant
space in the character class (use '/[^A-Za-z0-9\s_-]/' or equivalent) to avoid
flagged duplicates.
Scrutinizer flags is_string() on XoopsFormElement::render() as always false because the base class method returns null. Cast to string directly — the cast coerces whatever concrete subclasses return without triggering type-narrowing warnings.
Comment on lines
+266
to
+271
| // so renderers remain side-effect free. | ||
| $assets = '<script type="text/javascript" src="' . XOOPS_URL . '/include/spectrum.js"></script>' | ||
| . '<link rel="stylesheet" type="text/css" href="' . XOOPS_URL . '/include/spectrum.css">'; | ||
| } | ||
| $name = $this->esc($element->getName(false)); | ||
| $title = $this->esc($element->getTitle(false)); |
There was a problem hiding this comment.
When $xoTheme is not available, the Spectrum JS/CSS tags are prepended every time renderFormColorPicker() is called. If a form contains multiple color pickers this will duplicate assets in the HTML; consider adding a static/instance guard so the assets are only emitted once per request.
Comment on lines
+2
to
+6
| /** | ||
| * XOOPS Kernel Class | ||
| * | ||
| * You may not change or alter any portion of this comment or credits | ||
| * of supporting developers from this source code or any supporting source code |
There was a problem hiding this comment.
This new renderer’s file/class docblock metadata is inconsistent with the other renderer classes (Bootstrap3/4/5) which include @category/@package/@link-style tags. For consistency and tooling support, align this header/docblock with the existing renderer files’ structure.
…xity warnings
Extract repeated HTML attribute fragments and editor-toolbar button
building into constants and a helper method to satisfy SonarQube's
literal-duplication rule, and reduce renderThemeForm cognitive
complexity below the 15-point limit.
- Add ATTR_NAME, ATTR_ID, ATTR_TITLE, ATTR_TITLE_LEAD, ATTR_VALUE,
ATTR_SIZE, ATTR_MAXLENGTH constants for HTML attribute fragments
used 3-11 times each across form element renderers
- Extract renderThemeFormField() helper from renderThemeForm() loop
body to lower cognitive complexity from 16 to within the allowed 15
- Add renderEditorButton() helper used by both renderFormDhtmlTAXoopsCode
and renderFormDhtmlTATypography, eliminating 14 duplicated
onclick/title literal fragments and consolidating the inline
'btn btn-neutral btn-sm join-item' string via BTN_NEUTRAL_SM
- Remove redundant `i` flag from the class-name sanitiser regex in
addThemeFormBreak (the A-Z + a-z ranges already cover both cases)
No behavioural changes — output HTML is byte-identical to the previous
implementation. Lint passes; the existing XSS/escape test suite is
unaffected (the three errors present in the test run are pre-existing
bootstrap class-loading issues unrelated to this refactor).
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
htdocs/class/xoopsform/renderer/XoopsFormRendererTailwind.php (2)
119-126:⚠️ Potential issue | 🟠 MajorUse the encoded
getExtra()path instead of rewriting attribute values.This helper currently strips
<and>out of the raw fragment, so legitimate values are changed before they reach the browser (x < 5becomesx 5).htdocs/class/xoopsform/formelement.php:348-358already provides an encodedgetExtra(true)path for this case, so mutating the payload here is both lossy and unnecessary.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/class/xoopsform/renderer/XoopsFormRendererTailwind.php` around lines 119 - 126, The code is mutating the raw extra attribute (stripping '<' and '>') which loses legitimate content; instead call the encoded path to preserve safe characters: replace the current logic in XoopsFormRendererTailwind.php that reads $extra = (string) $element->getExtra(); ... str_replace(...) and return ' ' . $extra with a single safe return that uses the element's encoded extra: return ' ' . (string) $element->getExtra(true); thereby removing the str_replace mutation and using the built-in encoded path on the $element->getExtra method.
718-722:⚠️ Potential issue | 🟠 MajorCanonicalize the language include before
include_once.The candidate path is assembled from
$GLOBALS['xoopsConfig']['language']and used directly, so a malformed language token can escape the intendedlanguage/boundary. Resolve both the base directory and candidate file withrealpath()and only include when the candidate stays under that root; otherwise fall back toenglish/calendar.php.Safer include pattern
- if (file_exists(XOOPS_ROOT_PATH . '/language/' . $GLOBALS['xoopsConfig']['language'] . '/calendar.php')) { - include_once XOOPS_ROOT_PATH . '/language/' . $GLOBALS['xoopsConfig']['language'] . '/calendar.php'; - } else { - include_once XOOPS_ROOT_PATH . '/language/english/calendar.php'; - } + $languageBase = realpath(XOOPS_ROOT_PATH . '/language'); + $language = preg_replace('/[^A-Za-z0-9_]/', '', (string) ($GLOBALS['xoopsConfig']['language'] ?? 'english')); + $calendarPath = realpath(XOOPS_ROOT_PATH . '/language/' . $language . '/calendar.php'); + if ($languageBase !== false + && $calendarPath !== false + && str_starts_with($calendarPath, $languageBase . DIRECTORY_SEPARATOR) + ) { + include_once $calendarPath; + } else { + include_once XOOPS_ROOT_PATH . '/language/english/calendar.php'; + }As per coding guidelines: "Validate file paths with realpath() and boundary checks to prevent directory traversal".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/class/xoopsform/renderer/XoopsFormRendererTailwind.php` around lines 718 - 722, The include uses a candidate path built from XOOPS_ROOT_PATH and $GLOBALS['xoopsConfig']['language'] without canonicalization, allowing directory traversal; instead compute the language root realpath (realpath(XOOPS_ROOT_PATH . '/language')) and realpath() the candidate file built from that root + '/' + $GLOBALS['xoopsConfig']['language'] + '/calendar.php', then check that the candidate realpath is non-false and starts with the language root realpath (boundary check) and is a file before calling include_once; if the check fails, fall back to the canonical english/calendar.php include. Use the existing variables (XOOPS_ROOT_PATH and $GLOBALS['xoopsConfig']['language']) and update the include block in XoopsFormRendererTailwind.php accordingly.tests/unit/htdocs/class/xoopsforms/XoopsFormRendererTailwindTest.php (1)
28-33:⚠️ Potential issue | 🟠 MajorRemove
xoops_load()from the unit-test path.These calls make the test depend on a live XOOPS bootstrap instead of the classes it is asserting, so the file no longer runs as an isolated unit test. Replace them with test-owned stubs/fixtures or a local bootstrap that lives entirely inside the test environment.
Based on learnings: "Applies to tests/**/*.php : Tests must be fully isolated with no XOOPS installation required for unit tests".
Also applies to: 131-143
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/htdocs/class/xoopsforms/XoopsFormRendererTailwindTest.php` around lines 28 - 33, The test currently calls global bootstrap helper xoops_load('XoopsFormElement'), xoops_load('XoopsFormButton'), xoops_load('XoopsFormElementTray'), xoops_load('XoopsFormLabel'), xoops_load('XoopsFormRendererInterface'), and xoops_load('XoopsFormRendererTailwind') which makes it depend on a live XOOPS installation; remove all xoops_load(...) calls in XoopsFormRendererTailwindTest.php and instead provide test-local stubs or a small test bootstrap that defines the required classes/interfaces (e.g., XoopsFormElement, XoopsFormButton, XoopsFormElementTray, XoopsFormLabel, XoopsFormRendererInterface, XoopsFormRendererTailwind) within the test suite so the assertions run in isolation; apply the same replacement for the other occurrences referenced around lines 131-143.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@htdocs/class/xoopsform/renderer/XoopsFormRendererTailwind.php`:
- Around line 741-745: The calendar trigger is rendered even when
$GLOBALS['xoTheme'] is missing, but the assets (calendar.js, calendar-blue.css)
and initialization (showCalendar()) are only registered inside the xoTheme
branch, producing a dead control; update XoopsFormRendererTailwind to mirror
renderFormColorPicker() fallback: ensure the calendar asset registration and the
one-time init script are returned/echoed when xoTheme is not available and set
the same $included flag so assets/init run only once; modify both calendar
inclusion blocks (the one around addScript/addStylesheet and the second instance
referenced) to build and return the stylesheet/script HTML and the
showCalendar() init snippet when $GLOBALS['xoTheme'] is not an object, rather
than doing nothing.
- Around line 746-793: The concatenated locale constants (e.g., Calendar._DN,
Calendar._MN, Calendar._TT entries using _CAL_SUNDAY, _CAL_MONDAY, _CAL_TGL1STD,
_CAL_PREVYR, etc.) are injected directly into a single-quoted JS string and can
break JS if they contain quotes/backslashes/newlines; wrap each constant with
json_encode() when embedding (so Calendar._DN, Calendar._MN and each
Calendar._TT["..."] use json_encode(_CAL_...) instead of raw concatenation) to
produce properly escaped JS string literals and keep the surrounding PHP/JS
concatenation consistent.
---
Duplicate comments:
In `@htdocs/class/xoopsform/renderer/XoopsFormRendererTailwind.php`:
- Around line 119-126: The code is mutating the raw extra attribute (stripping
'<' and '>') which loses legitimate content; instead call the encoded path to
preserve safe characters: replace the current logic in
XoopsFormRendererTailwind.php that reads $extra = (string) $element->getExtra();
... str_replace(...) and return ' ' . $extra with a single safe return that uses
the element's encoded extra: return ' ' . (string) $element->getExtra(true);
thereby removing the str_replace mutation and using the built-in encoded path on
the $element->getExtra method.
- Around line 718-722: The include uses a candidate path built from
XOOPS_ROOT_PATH and $GLOBALS['xoopsConfig']['language'] without
canonicalization, allowing directory traversal; instead compute the language
root realpath (realpath(XOOPS_ROOT_PATH . '/language')) and realpath() the
candidate file built from that root + '/' + $GLOBALS['xoopsConfig']['language']
+ '/calendar.php', then check that the candidate realpath is non-false and
starts with the language root realpath (boundary check) and is a file before
calling include_once; if the check fails, fall back to the canonical
english/calendar.php include. Use the existing variables (XOOPS_ROOT_PATH and
$GLOBALS['xoopsConfig']['language']) and update the include block in
XoopsFormRendererTailwind.php accordingly.
In `@tests/unit/htdocs/class/xoopsforms/XoopsFormRendererTailwindTest.php`:
- Around line 28-33: The test currently calls global bootstrap helper
xoops_load('XoopsFormElement'), xoops_load('XoopsFormButton'),
xoops_load('XoopsFormElementTray'), xoops_load('XoopsFormLabel'),
xoops_load('XoopsFormRendererInterface'), and
xoops_load('XoopsFormRendererTailwind') which makes it depend on a live XOOPS
installation; remove all xoops_load(...) calls in
XoopsFormRendererTailwindTest.php and instead provide test-local stubs or a
small test bootstrap that defines the required classes/interfaces (e.g.,
XoopsFormElement, XoopsFormButton, XoopsFormElementTray, XoopsFormLabel,
XoopsFormRendererInterface, XoopsFormRendererTailwind) within the test suite so
the assertions run in isolation; apply the same replacement for the other
occurrences referenced around lines 131-143.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0865f322-3d7b-4f76-be76-4f0363b66126
📒 Files selected for processing (2)
htdocs/class/xoopsform/renderer/XoopsFormRendererTailwind.phptests/unit/htdocs/class/xoopsforms/XoopsFormRendererTailwindTest.php
Comment on lines
+493
to
+497
| $fontStr .= $this->renderEditorButton($styleBtn, 'xoopsMakeBold("' . $hiddentext . '", "' . $textarea_id . '")', _XOOPS_FORM_ALT_BOLD, 'fa-solid fa-bold'); | ||
| $fontStr .= $this->renderEditorButton($styleBtn, 'xoopsMakeItalic("' . $hiddentext . '", "' . $textarea_id . '")', _XOOPS_FORM_ALT_ITALIC, 'fa-solid fa-italic'); | ||
| $fontStr .= $this->renderEditorButton($styleBtn, 'xoopsMakeUnderline("' . $hiddentext . '", "' . $textarea_id . '")', _XOOPS_FORM_ALT_UNDERLINE, 'fa-solid fa-underline'); | ||
| $fontStr .= $this->renderEditorButton($styleBtn, 'xoopsMakeLineThrough("' . $hiddentext . '", "' . $textarea_id . '")', _XOOPS_FORM_ALT_LINETHROUGH, 'fa-solid fa-strikethrough'); | ||
| $fontStr .= '</div>'; |
There was a problem hiding this comment.
renderEditorButton() writes $title directly into title='...' without escaping, but these call sites pass raw translation constants (e.g. _XOOPS_FORM_ALT_BOLD, _XOOPS_FORM_ALT_ITALIC). If those strings contain quotes/special chars the HTML becomes malformed and can turn into an injection vector. Either escape $title inside renderEditorButton() (and pass raw titles everywhere) or ensure all call sites wrap the title in esc().
| $ret = '<div class="flex flex-wrap gap-2">'; | ||
| if ($element->_showDelete) { | ||
| $ret .= '<button type="submit" class="btn btn-error" name="delete" id="delete"' | ||
| . ' onclick="this.form.elements.op.value=\'delete\'">' . _DELETE . '</button>'; |
There was a problem hiding this comment.
In renderFormButtonTray(), _DELETE is inserted as raw HTML in the delete button text. For consistent escaping (and to avoid malformed markup if translations contain special characters), escape _DELETE before outputting it.
Suggested change
| . ' onclick="this.form.elements.op.value=\'delete\'">' . _DELETE . '</button>'; | |
| . ' onclick="this.form.elements.op.value=\'delete\'">' . $this->esc(_DELETE) . '</button>'; |
| } | ||
| $ret .= '<input type="button" class="btn btn-error" name="cancel" id="cancel"' | ||
| . ' onClick="history.go(-1);return true;" value="' . $this->esc(_CANCEL) . '">' | ||
| . '<button type="reset" class="btn btn-warning" name="reset" id="reset">' . _RESET . '</button>' |
There was a problem hiding this comment.
In renderFormButtonTray(), _RESET is inserted as raw HTML in the reset button text. For consistent escaping (and to avoid malformed markup if translations contain special characters), escape _RESET before outputting it.
Suggested change
| . '<button type="reset" class="btn btn-warning" name="reset" id="reset">' . _RESET . '</button>' | |
| . '<button type="reset" class="btn btn-warning" name="reset" id="reset">' . $this->esc(_RESET) . '</button>' |
… button tray, calendar
Body addition (inserted after the existing language-include bullet):
- resolveCalendarLanguageFile() now performs a two-layer check: the
character-level allowlist blocks traversal at the regex stage, and a
realpath boundary check canonicalizes the candidate and verifies it sits
under XOOPS_ROOT_PATH/language before returning. Catches symlink escapes
that the regex alone cannot see, and matches the explicit realpath()
hardening asked for in the review feedback. Test coverage expanded to 8
cases: traversal, absolute path, null byte, space, empty string, missing
per-language calendar.php, missing config, and explicit english.
Comment on lines
+293
to
+306
| $columns = (int) ($element->columns ?? 0); | ||
| switch ($columns) { | ||
| case 0: | ||
| $containerCls = 'flex flex-wrap gap-4'; | ||
| $labelCls = 'label cursor-pointer gap-2'; | ||
| break; | ||
| case 1: | ||
| $containerCls = 'flex flex-col gap-2'; | ||
| $labelCls = 'label cursor-pointer justify-start gap-2'; | ||
| break; | ||
| default: | ||
| $containerCls = 'grid grid-cols-2 md:grid-cols-3 gap-2'; | ||
| $labelCls = 'label cursor-pointer justify-start gap-2'; | ||
| break; |
There was a problem hiding this comment.
For checkbox/radio rendering, values of $element->columns > 1 currently all map to a fixed responsive grid (2 cols on small, 3 on md) and ignore the actual numeric column count. XoopsFormCheckBox/XoopsFormRadio document that a positive integer n should mean n options per line; consider translating $columns into the corresponding Tailwind grid-cols-* class (with a reasonable clamp) so the renderer respects the element’s contract.
Comment on lines
+311
to
+327
| $elementValue = $element->getValue(); | ||
| $extra = $this->renderExtra($element); | ||
| $delimeter = $element->getDelimeter(); | ||
|
|
||
| foreach ($element->getOptions() as $value => $name) { | ||
| ++$idSuffix; | ||
| $checked = $this->isOptionChecked($value, $elementValue) ? ' checked' : ''; | ||
| $inputId = $this->esc($elementId . $idSuffix); | ||
| $ret .= '<label class="' . $labelCls . '">' | ||
| . '<input class="' . $type . ' ' . $type . '-primary" type="' . $type . '"' | ||
| . self::ATTR_NAME . $this->esc($elementName) . '"' | ||
| . ' id="' . $inputId . '"' | ||
| . self::ATTR_TITLE_LEAD . $this->esc(strip_tags((string) $name)) . '"' | ||
| . self::ATTR_VALUE . $this->esc($value) . '"' | ||
| . $checked . $extra . '>' | ||
| . '<span class="label-text">' . $this->esc((string) $name) . $delimeter . '</span>' | ||
| . '</label>'; |
There was a problem hiding this comment.
Typo: local variable is named $delimeter (and later used as such). Consider renaming to $delimiter for readability and consistency with the underlying method name getDelimeter()/concept (delimiter) to avoid propagating the misspelling.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
htdocs/class/xoopsform/renderer/XoopsFormRendererTailwind.php (1)
208-214: 🛠️ Refactor suggestion | 🟠 MajorComplete the remaining public PHPDocs with
@throws.Several new public methods still stop at
@param/@return, so the class is not aligned with the repository PHPDoc requirement yet.As per coding guidelines: "New public methods must have PHPDoc with
@param,@return, and@throwstags."Also applies to: 227-233, 256-262, 351-357, 623-631, 672-678, 691-697, 705-711, 726-732, 740-746, 774-780, 794-800, 1101-1109
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/class/xoopsform/renderer/XoopsFormRendererTailwind.php` around lines 208 - 214, The PHPDoc for public methods in class XoopsFormRendererTailwind (starting at the block documenting "Render support for XoopsFormButton") is missing `@throws` tags; update each public method's PHPDoc (including the blocks listed in the review: the docblock that mentions XoopsFormButton and the other public methods in this class) to include an appropriate `@throws` annotation that documents the exceptions the method can actually throw (use the concrete exception types thrown by the implementation, or if none are thrown directly, add a generic `@throws` \Exception or `@throws` \RuntimeException per project convention), e.g., add "@throws \Exception" (or the specific exception class) to the PHPDoc for the methods in XoopsFormRendererTailwind so every public method has `@param`, `@return` and `@throws` as required.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@htdocs/class/xoopsform/renderer/XoopsFormRendererTailwind.php`:
- Around line 474-481: renderEditorButton currently only sets a title attribute
(via self::ATTR_TITLE_SQ and $this->esc($title)) which doesn't provide a
reliable accessible name for icon-only controls; update
renderEditorButton(string $class, string $onclickJs, string $title, string
$iconClass, string $trailingHtml = '') to also emit an aria-label attribute with
the same escaped $title (use $this->esc($title)) alongside the title, and then
update the calendar trigger code (the calendar button referenced around lines
877-879) to pass the same title so the aria-label mirrors the visual tooltip;
keep ATTR_TITLE_SQ usage intact but add the aria-label attribute generation to
the helper so all buttons using renderEditorButton gain a proper accessible
name.
- Around line 493-503: The popup URLs interpolate the raw textarea name
($textareaIdRaw) into query strings, so names containing &, #, = break the
target parameter; fix by URL-encoding the id before building $urlImageMgr and
$urlSmilies (use rawurlencode on $textareaIdRaw) and then pass the encoded value
into the buildJsCall/openWithSelfMain calls that construct the image and smilies
popup URLs in class XoopsFormRendererTailwind (references: $textareaIdRaw,
$urlImageMgr, $urlSmilies, buildJsCall('openWithSelfMain', ...)).
- Around line 147-167: The helper renderElementHtml currently returns
((string)$rendered).$echoed which reverses echoed vs returned output and throws
away any buffers opened by render; update renderElementHtml (use $bufferLevel,
$rendered, $echoed) to capture all buffer contents above the baseline and
preserve original ordering by: declare a $result variable outside try/finally,
in try assign $rendered = call_user_func([$element,'render']) and $echoed =
(string)ob_get_clean(), then in finally collect any remaining buffers into a
string by repeatedly calling ob_get_clean() while ob_get_level() > $bufferLevel
and prepend/append them so the final output is
echo-then-extra-buffers-then-rendered (e.g. $result = $echoed . $extra .
((string)$rendered)); return $result after the finally block.
---
Duplicate comments:
In `@htdocs/class/xoopsform/renderer/XoopsFormRendererTailwind.php`:
- Around line 208-214: The PHPDoc for public methods in class
XoopsFormRendererTailwind (starting at the block documenting "Render support for
XoopsFormButton") is missing `@throws` tags; update each public method's PHPDoc
(including the blocks listed in the review: the docblock that mentions
XoopsFormButton and the other public methods in this class) to include an
appropriate `@throws` annotation that documents the exceptions the method can
actually throw (use the concrete exception types thrown by the implementation,
or if none are thrown directly, add a generic `@throws` \Exception or `@throws`
\RuntimeException per project convention), e.g., add "@throws \Exception" (or
the specific exception class) to the PHPDoc for the methods in
XoopsFormRendererTailwind so every public method has `@param`, `@return` and `@throws`
as required.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0eac3038-a1ef-48ac-961f-9491c1d448f7
📒 Files selected for processing (1)
htdocs/class/xoopsform/renderer/XoopsFormRendererTailwind.php
Comment on lines
+147
to
+167
| $bufferLevel = ob_get_level(); | ||
| ob_start(); | ||
| try { | ||
| /** @var mixed $rendered */ | ||
| $rendered = call_user_func([$element, 'render']); | ||
| $echoed = (string) ob_get_clean(); | ||
|
|
||
| return ((string) $rendered) . $echoed; | ||
| } finally { | ||
| // Peel any buffer levels that are still above the baseline. Covers | ||
| // two cases: | ||
| // 1. render() threw before ob_get_clean() ran — our buffer is | ||
| // still open one level above the baseline. | ||
| // 2. render() opened its own buffer and returned normally | ||
| // without closing it — ob_get_clean() only popped the top | ||
| // level, leaving our outer buffer stranded. | ||
| // In either case the invariant we want is ob_get_level() === | ||
| // $bufferLevel after the call completes. | ||
| while (ob_get_level() > $bufferLevel) { | ||
| ob_end_clean(); | ||
| } |
There was a problem hiding this comment.
Preserve buffered output order in renderElementHtml().
This helper currently captures only the top buffer and then returns $rendered . $echoed. That reverses normal echo $element->render() ordering, and any outer buffer still above the baseline gets discarded in finally instead of being included in the returned HTML.
💡 Minimal fix
try {
/** `@var` mixed $rendered */
$rendered = call_user_func([$element, 'render']);
- $echoed = (string) ob_get_clean();
-
- return ((string) $rendered) . $echoed;
+ $echoed = '';
+ while (ob_get_level() > $bufferLevel) {
+ $echoed = (string) ob_get_clean() . $echoed;
+ }
+
+ return $echoed . (string) $rendered;
} finally {
// Peel any buffer levels that are still above the baseline. Covers
// two cases:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/class/xoopsform/renderer/XoopsFormRendererTailwind.php` around lines
147 - 167, The helper renderElementHtml currently returns
((string)$rendered).$echoed which reverses echoed vs returned output and throws
away any buffers opened by render; update renderElementHtml (use $bufferLevel,
$rendered, $echoed) to capture all buffer contents above the baseline and
preserve original ordering by: declare a $result variable outside try/finally,
in try assign $rendered = call_user_func([$element,'render']) and $echoed =
(string)ob_get_clean(), then in finally collect any remaining buffers into a
string by repeatedly calling ob_get_clean() while ob_get_level() > $bufferLevel
and prepend/append them so the final output is
echo-then-extra-buffers-then-rendered (e.g. $result = $echoed . $extra .
((string)$rendered)); return $result after the finally block.
…n-only buttons
Two fixes from the latest CodeRabbit review round:
- renderFormDhtmlTAXoopsCode(): the imagemanager and smilies popup URLs
interpolated the raw textarea id into the target= query parameter.
Field names containing &, #, or = would break the query string. Use
rawurlencode() for the URL parameter while continuing to pass the raw
value to buildJsCall() and extension->encode() where it serves as a
DOM element identifier.
- renderEditorButton(): icon-only toolbar buttons relied on title for
their accessible name, but screen readers do not reliably announce
title as the accessible name. Add aria-label using the same escaped
title string so all editor controls (xoopscode, typography, style,
align, and length-check buttons) are labeled for assistive technology.
- renderFormTextDateSelect(): the calendar trigger button was icon-only
with no accessible name at all. Add aria-label using the element's
title (the human-readable label for the date field).
Also includes the two fixes from the previous approval that were not yet
pushed:
- renderThemeForm(): _REQUIRED was emitted raw in the required-fields
footer note. Wrap in $this->esc() for consistency.
- renderFormDhtmlTAXoopsCode(): $extension->encode() was receiving the
HTML-escaped textarea id. Pass $textareaIdRaw instead, and remove the
now-dead $textarea_id assignment.
Comment on lines
+1036
to
+1043
| $formName = $this->esc($form->getName(false)); | ||
|
|
||
| $ret = '<div class="card bg-base-100 shadow">'; | ||
| $ret .= '<form name="' . $formName . self::ATTR_ID . $formName . '"' | ||
| . ' action="' . $this->esc($form->getAction(false)) . '"' | ||
| . ' method="' . $this->esc($form->getMethod()) . '"' | ||
| . ' onsubmit="return xoopsFormValidate_' . $formName . '();"' | ||
| . $this->renderExtra($form) |
There was a problem hiding this comment.
renderThemeForm() escapes the form name and then uses that escaped string inside the onsubmit="return xoopsFormValidate_<name>();" handler. XoopsForm::renderValidationJS() defines the function using the raw form name ($this->getName()), so if the form name contains characters that esc() encodes (e.g. &, quotes), the handler will call a different function name and validation will break. Suggestion: keep a raw $formNameRaw for JS identifier usage (ideally sanitized to [A-Za-z0-9_]+), and only HTML-escape the copy used in name/id attributes.
Comment on lines
+311
to
+317
| $elementValue = $element->getValue(); | ||
| $extra = $this->renderExtra($element); | ||
| $delimeter = $element->getDelimeter(); | ||
|
|
||
| foreach ($element->getOptions() as $value => $name) { | ||
| ++$idSuffix; | ||
| $checked = $this->isOptionChecked($value, $elementValue) ? ' checked' : ''; |
There was a problem hiding this comment.
Typo in local variable name: $delimeter is misspelled (should be $delimiter). While it doesn’t change behavior, it makes the code harder to read/search and risks propagating the misspelling into future changes.
…aram, aria-label, escape _REQUIRED
Address latest Copilot/CodeRabbit review findings:
- renderThemeForm(): the form name was HTML-escaped and then reused inside
onsubmit="return xoopsFormValidate_<name>();". But renderValidationJS()
defines the function with the raw name. If the name contains &, quotes,
or other escapable characters, the handler calls a non-existent function
and validation silently breaks. Split into $formNameRaw (for the JS
function suffix) and $formNameAttr (for HTML attributes).
- renderFormDhtmlTAXoopsCode(): the imagemanager and smilies popup URLs
interpolated the raw textarea id into the target= query parameter. Field
names containing &, #, or = would break the query string. Use
rawurlencode() for the URL parameter while passing the raw value to
buildJsCall() and extension->encode().
- renderEditorButton(): add aria-label using the same escaped title string,
so all icon-only editor toolbar buttons are labeled for screen readers.
renderFormTextDateSelect(): add aria-label on the calendar trigger button
using the element's title.
- renderThemeForm(): escape _REQUIRED in the required-fields footer note.
- renderFormDhtmlTAXoopsCode(): pass $textareaIdRaw (not HTML-escaped
$textarea_id) to $extension->encode(), and remove the dead assignment.
- Rename $delimeter to $delimiter in renderChecked() — the local variable
misspelling propagated from the core API getDelimeter() but the variable
name should still be readable.
Full suite: 6040 tests, 10528 assertions, 0 failures.
Comment on lines
+244
to
+249
| // Invalid UTF-8 byte sequence — json_encode returns false without | ||
| // JSON_THROW_ON_ERROR, which would silently produce malformed JS. | ||
| // With the fix, esJs() catches the JsonException and returns a safe | ||
| // empty JS string literal. | ||
| $result = @$renderer->exposedEsJs("\xC3\x28"); | ||
| $this->assertSame('""', $result, 'Invalid UTF-8 must produce a safe empty JS string literal, not an empty string'); |
There was a problem hiding this comment.
The test suppresses warnings with the error-control operator (@). This can mask unexpected warnings/notices and makes failures harder to debug. Prefer capturing/suppressing the expected E_USER_WARNING via a temporary set_error_handler()/restore_error_handler() (as done in other unit tests) and optionally asserting the warning was triggered.
Suggested change
| // Invalid UTF-8 byte sequence — json_encode returns false without | |
| // JSON_THROW_ON_ERROR, which would silently produce malformed JS. | |
| // With the fix, esJs() catches the JsonException and returns a safe | |
| // empty JS string literal. | |
| $result = @$renderer->exposedEsJs("\xC3\x28"); | |
| $this->assertSame('""', $result, 'Invalid UTF-8 must produce a safe empty JS string literal, not an empty string'); | |
| $warnings = []; | |
| set_error_handler(static function (int $errno, string $errstr) use (&$warnings): bool { | |
| if (E_USER_WARNING === $errno) { | |
| $warnings[] = $errstr; | |
| return true; | |
| } | |
| return false; | |
| }); | |
| try { | |
| // Invalid UTF-8 byte sequence — json_encode returns false without | |
| // JSON_THROW_ON_ERROR, which would silently produce malformed JS. | |
| // With the fix, esJs() catches the JsonException and returns a safe | |
| // empty JS string literal. | |
| $result = $renderer->exposedEsJs("\xC3\x28"); | |
| } finally { | |
| restore_error_handler(); | |
| } | |
| $this->assertSame('""', $result, 'Invalid UTF-8 must produce a safe empty JS string literal, not an empty string'); | |
| $this->assertNotEmpty($warnings, 'Invalid UTF-8 should trigger an E_USER_WARNING'); |
…labels, URL encoding, escaping
Comprehensive sweep to close all remaining open review comments in one
pass, eliminating the iterative review round-trip.
Security / correctness:
- renderThemeForm(): the HTML-escaped form name was used in the onsubmit
handler, but renderValidationJS() defines the function with the raw name.
Split into $formNameRaw (for JS function suffix) and $formNameAttr (for
HTML attributes).
- renderFormDhtmlTAXoopsCode(): URL-encode the textarea id in the
imagemanager and smilies popup target= query parameters via
rawurlencode(). Field names containing &, #, or = would break the URL.
The raw value continues to be used for buildJsCall() and extension
encode() where it serves as a DOM element identifier.
- renderFormDhtmlTAXoopsCode(): pass $textareaIdRaw (not HTML-escaped)
to $extension->encode(). Extensions build JS like getElementById(id)
where HTML entities don't match actual DOM element IDs.
- renderThemeForm(): escape _REQUIRED in the required-fields footer note.
Accessibility:
- renderEditorButton(): add aria-label using the same escaped title so
all icon-only toolbar buttons (xoopscode, typography, style, align)
are labeled for screen readers.
- renderFormTextDateSelect(): add aria-label on the calendar trigger
button using the element's title.
- renderFormDhtmlTATypography(): add aria-label on the three dropdown
triggers (Size, Font, Color) and the length-check button — all icon-only
controls that relied on title alone.
Cleanup:
- Rename $delimeter to $delimiter in renderChecked() for readability.
The misspelling propagated from the core API getDelimeter() but the
local variable name should be correct.
- Add @throws \Throwable to renderFormElementTray() (calls
renderElementHtml which can throw).
- Document the $extra parameter contract on addThemeFormBreak(): rendered
verbatim (not escaped) matching the convention established by all
Bootstrap renderers.
Comment on lines
+1039
to
+1046
| $formNameRaw = (string) $form->getName(false); | ||
| $formNameAttr = $this->esc($formNameRaw); | ||
|
|
||
| $ret = '<div class="card bg-base-100 shadow">'; | ||
| $ret .= '<form name="' . $formNameAttr . self::ATTR_ID . $formNameAttr . '"' | ||
| . ' action="' . $this->esc($form->getAction(false)) . '"' | ||
| . ' method="' . $this->esc($form->getMethod()) . '"' | ||
| . ' onsubmit="return xoopsFormValidate_' . $formNameRaw . '();"' |
There was a problem hiding this comment.
renderThemeForm() builds the onsubmit handler using the raw form name (getName(false)), but XoopsForm::renderValidationJS() generates xoopsFormValidate_{formname}() using the encoded form name (getName() default). This can break validation (function name mismatch) and also allows malformed form names containing quotes to break out of the onsubmit="..." attribute. Use the same sanitized/encoded form name for both the onsubmit attribute and the validation JS function name (typically $form->getName()), and avoid concatenating unescaped raw values into event-handler attributes.
Suggested change
| $formNameRaw = (string) $form->getName(false); | |
| $formNameAttr = $this->esc($formNameRaw); | |
| $ret = '<div class="card bg-base-100 shadow">'; | |
| $ret .= '<form name="' . $formNameAttr . self::ATTR_ID . $formNameAttr . '"' | |
| . ' action="' . $this->esc($form->getAction(false)) . '"' | |
| . ' method="' . $this->esc($form->getMethod()) . '"' | |
| . ' onsubmit="return xoopsFormValidate_' . $formNameRaw . '();"' | |
| $formName = (string) $form->getName(); | |
| $ret = '<div class="card bg-base-100 shadow">'; | |
| $ret .= '<form name="' . $formName . self::ATTR_ID . $formName . '"' | |
| . ' action="' . $this->esc($form->getAction(false)) . '"' | |
| . ' method="' . $this->esc($form->getMethod()) . '"' | |
| . ' onsubmit="return xoopsFormValidate_' . $formName . '();"' |
…nd onsubmit=. Added a comment explaining the convention and documenting the core constraint that form names must be
JS-identifier-safe.
- renderThemeForm(): use $form->getName() (default encoding) instead of
getName(false) + manual esc(). This matches XoopsFormRendererBootstrap4
and renderValidationJS(), which both use the default-encoded name.
The earlier split into raw/escaped variants introduced a mismatch where
the onsubmit handler used the raw name while the validation function
was defined with the encoded name. XOOPS core assumes form names are
JS-identifier-safe — this renderer does not attempt to fix that broader
constraint.
…ria-labels, URL encoding, escaping
Comprehensive sweep to close all remaining open review comments in one
pass, eliminating the iterative review round-trip.
Security / correctness:
- renderThemeForm(): use $form->getName() (default encoding) for name=,
id=, and onsubmit= attributes. This matches XoopsFormRendererBootstrap4
and renderValidationJS(), which both use the default-encoded name. The
earlier split into raw/escaped variants introduced a mismatch where the
onsubmit handler called a differently-named function than what
renderValidationJS() defined. XOOPS core assumes form names are
JS-identifier-safe; this renderer does not attempt to fix that broader
constraint.
- renderFormDhtmlTAXoopsCode(): URL-encode the textarea id in the
imagemanager and smilies popup target= query parameters via
rawurlencode(). Field names containing &, #, or = would break the URL.
The raw value continues to be used for buildJsCall() and extension
encode() where it serves as a DOM element identifier.
- renderFormDhtmlTAXoopsCode(): pass $textareaIdRaw (not HTML-escaped)
to $extension->encode(). Extensions build JS like getElementById(id)
where HTML entities don't match actual DOM element IDs.
- renderThemeForm(): escape _REQUIRED in the required-fields footer note.
Accessibility:
- renderEditorButton(): add aria-label using the same escaped title so
all icon-only toolbar buttons (xoopscode, typography, style, align)
are labeled for screen readers.
- renderFormTextDateSelect(): add aria-label on the calendar trigger
button using the element's title.
- renderFormDhtmlTATypography(): add aria-label on the three dropdown
triggers (Size, Font, Color) and the length-check button — all icon-only
controls that relied on title alone.
SonarQube compliance:
- Extract ATTR_ARIA_LABEL constant for the duplicated "' aria-label='"
literal (5 occurrences).
- Replace generic RuntimeException in the buffer-leak test with a
dedicated RenderTestException class defined in the test file.
Cleanup:
- Rename $delimeter to $delimiter in renderChecked() for readability.
The misspelling propagated from the core API getDelimeter() but the
local variable name should be correct.
- Add @throws \Throwable to renderFormElementTray() (calls
renderElementHtml which can throw).
- Document the $extra parameter contract on addThemeFormBreak(): rendered
verbatim (not escaped) matching the convention established by all
Bootstrap renderers.
|
|
||
| $desc = $element->getDescription(); | ||
| $descHtml = $desc !== '' | ||
| ? '<div class="label"><span class="label-text-alt text-base-content/60">' . $this->esc($desc) . '</span></div>' |
There was a problem hiding this comment.
In themed forms, other core renderers output element captions/descriptions unescaped (i.e., allow HTML in help text). Here the description is always escaped via esc(), which will render any existing HTML markup as plain text and changes behavior vs. Bootstrap/Legacy renderers. Consider either rendering getDescription() raw (to match existing renderer behavior) or making this opt-in/configurable and documenting that Tailwind renderer treats descriptions as plain text for XSS safety.
Suggested change
| ? '<div class="label"><span class="label-text-alt text-base-content/60">' . $this->esc($desc) . '</span></div>' | |
| ? '<div class="label"><span class="label-text-alt text-base-content/60">' . $desc . '</span></div>' |
| $rendered = call_user_func([$element, 'render']); | ||
| $echoed = (string) ob_get_clean(); | ||
|
|
||
| return ((string) $rendered) . $echoed; |
There was a problem hiding this comment.
renderElementHtml() concatenates the return value before any captured echoed output (return ((string)$rendered) . $echoed;). If an element both echoes and returns (even accidentally), this reverses the natural output order (echo happens before the return is consumed), which can change markup structure. To preserve runtime ordering, append the return value after the captured buffer (i.e., $echoed . (string) $rendered).
Suggested change
| return ((string) $rendered) . $echoed; | |
| return $echoed . (string) $rendered; |
…escription raw
Address Copilot review #4060302370:
- renderElementHtml(): swap output order from $rendered . $echoed to
$echoed . (string) $rendered. Echoed content executes first in PHP's
runtime, so it should appear first in the returned string. The previous
order would reverse markup if an element both echoed and returned.
- renderThemeFormField(): render getDescription() raw (not escaped) to
match Bootstrap4 and Legacy renderers. Modules commonly use HTML in
field descriptions (links, <code>, <em>, etc.); escaping breaks that
established contract.
|
| * @copyright (c) 2000-2026 XOOPS Project (https://xoops.org) | ||
| * @license GNU GPL 2 (https://www.gnu.org/licenses/gpl-2.0.html) | ||
| */ | ||
| http_response_code(404); |
There was a problem hiding this comment.
The other Frameworks/*/index.php files return 404 using header('HTTP/1.0 404 Not Found'); and then exit; (see Frameworks/index.php and Frameworks/jquery/index.php). For consistency, consider using the same header() call here instead of only http_response_code(404).
Suggested change
| http_response_code(404); | |
| header('HTTP/1.0 404 Not Found'); |
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.
Security hardening
Escaping fixes in
XoopsFormRendererTailwind:renderEditorButton()now escapes the$titleargument internally, covering every toolbar button (xoopscode, typography, style, align) in one place. Redundant pre-escaping at the seven xoopscode call sites was removed to prevent double-encoding.renderFormButtonTray()now escapes_DELETEand_RESETbefore emitting them as button text, matching the existing treatment of_CANCEL.Calendar JS injection fix (
renderFormTextDateSelect):The calendar init script previously concatenated ~23
_CAL_*translation constants and_CLOSE/_SHORTDATESTRINGraw into JavaScript string literals. A locale containing a single double quote would break the inline<script>block. A newesJs()helper encodes values for JS string literals viajson_encodewithJSON_HEX_TAG | JSON_HEX_QUOT | JSON_HEX_APOS | JSON_HEX_AMP | JSON_UNESCAPED_UNICODE, and the locale block is now built through the newbuildCalendarLocaleJs()helper that reuses the same safe encoding between the$xoThemeand non-theme code paths.Non-theme calendar fallback:
Previously the calendar button unconditionally called
showCalendar(), butshowCalendar()and its assets (calendar.js,calendar-blue.css) were only registered inside the$xoThemebranch. In standalone contexts (AJAX handlers, custom entry points) the button was a dead control and the browser threw a ReferenceError. The fallback mirrorsrenderFormColorPicker()'s pattern: emit<script src>,<link>, and the inline init script once per request via a static flag.Calendar language-file hardening:
resolveCalendarLanguageFile()now uses two layers of validation. First, a strict allowlist rejects traversal and malformed language values before any filesystem lookup. Second, arealpath()boundary check canonicalizes both the candidate path andXOOPS_ROOT_PATH/language, and only accepts files that remain within that tree; all failures fall back toenglish/calendar.php. This also closes symlink-escape cases. Test coverage for this path now covers traversal, absolute path input, null byte, space, empty string, missing per-languagecalendar.php, missing config, and explicitenglish.Tests added:
testRenderEditorButtonEscapesTitleAttribute— direct XSS payload test via anonymous subclass.testRenderFormButtonTrayEscapesDeleteAndResetLabels— sentinel-marker test proving_DELETE,_RESET,_CANCELflow throughesc().testEsJsEncodesScriptBreakoutAndQuotes— asserts</script>, quotes, ampersand, unicode all handled.testBuildCalendarLocaleJsEncodesLocaleStrings— proves locale strings are JSON-encoded in the generated JS.testResolveCalendarLanguageFileRejectsTraversal— 8 cases covering all failure paths.Full suite: 6035 tests, 10513 assertions, 0 failures.
Summary by CodeRabbit
New Features
Bug Fixes / Security
Documentation
Tests
Chores