Add file-based PHP custom blocks (no eval)

[mambax7]

Summary

• New file-based PHP block system: Custom blocks now reference a PHP file and function name (filename.php|function_name) stored in htdocs/custom_blocks/, eliminating the need for eval() in new blocks
• Backward compatible: Legacy eval()-based PHP blocks still work when XOOPS_ALLOW_PHP_BLOCKS is set to true in mainfile.php
• Security hardened: Filename regex rejects path traversal, special characters, and subdirectories; function names are validated before calling

Changes

• htdocs/kernel/block.php — New executePhpBlock() method with file-based dispatch and legacy fallback
• htdocs/modules/system/class/block.php — Uses inherited executePhpBlock() from parent
• htdocs/modules/system/language/english/admin/blocksadmin.php — Updated label to "PHP Script (file-based)"
• htdocs/custom_blocks/ — New directory with 3 working examples:
  • example_welcome.php — Personalized greeting using $xoopsUser/$xoopsConfig
  • example_recent_members.php — Recent members with avatars using CriteriaCompo
  • example_site_stats.php — Site stats using XOOPS handlers (member, comment)
• docs/custom-blocks-tutorial.md — Full migration guide, API reference, and troubleshooting
• tests/unit/htdocs/kernel/XoopsBlockPhpBlockTest.php — 45 comprehensive tests

Test plan

• 45 new tests pass covering file-based execution, format validation, path traversal prevention, legacy eval blocking, and example blocks
• 113 total block tests pass (existing + new)
• Manual: Create a file-based PHP block via admin panel and verify it renders
• Manual: Verify legacy PHP blocks show deprecation warning when XOOPS_ALLOW_PHP_BLOCKS is not set
• Manual: Verify legacy PHP blocks still work when XOOPS_ALLOW_PHP_BLOCKS = true

Summary by CodeRabbit

• New Features

  • File-based PHP block support, three example blocks (welcome, recent members, site stats), and access-guard for custom_blocks.

• Documentation

  • New migration tutorial with quick-start, examples, compatibility notes, APIs, security guidance, and troubleshooting.

• Security & Improvements

  • Safer block handling (path/function validation, guarded directory), safer site-URL substitution, logging and clearer admin label.

• Tests

  • Comprehensive tests for file-based format, security checks, edge cases, and example block behavior.

[coderabbitai]

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• ✅ Review completed - (🔄 Check again to review again)

Walkthrough

Adds a file-based callback system for XOOPS PHP blocks using a filename.php|function_name format (XoopsBlock::executePhpBlock), preserves a gated legacy eval() fallback with deprecation logging, provides three example block files and an index guard, adds a migration tutorial, and a comprehensive PHPUnit test suite. (33 words)

Changes

Cohort / File(s)	Summary
Documentation & Tutorial docs/custom-blocks-tutorial.md	New migration guide documenting the file-based block format, quick-start, examples, available APIs, security reminders, troubleshooting, and legacy XOOPS_ALLOW_PHP_BLOCKS behavior.
Example Custom Blocks htdocs/custom_blocks/example_welcome.php, htdocs/custom_blocks/example_recent_members.php, htdocs/custom_blocks/example_site_stats.php	Three example block files adding b_custom_*_show() functions that assemble and return HTML for welcome, recent-members, and site-stats blocks; include usage notes and expected outputs.
Custom Blocks Directory Guard htdocs/custom_blocks/index.php	Added index guard returning 404 to prevent directory listing/direct access.
Core Block Execution htdocs/kernel/block.php, htdocs/modules/system/class/block.php	Introduces XoopsBlock::executePhpBlock() and helpers; getContent('P') delegates to it. Implements parsing of `filename.php
Localization htdocs/modules/system/language/english/admin/blocksadmin.php	Updated _AM_SYSTEM_BLOCKS_PHP label to "PHP Script (file-based)".
Unit Tests tests/unit/htdocs/kernel/XoopsBlockPhpBlockTest.php	Comprehensive PHPUnit suite covering file-based execution, placeholder replacement, trimming, null/empty returns, missing file/function handling, security (path traversal, absolute/Windows paths), legacy eval gating, include-once semantics, example-file integration, index guard presence, and multiple edge cases.

Sequence Diagram(s)

sequenceDiagram
    participant Client as Block Renderer
    participant BlockKernel as XoopsBlock::getContent()
    participant Executor as XoopsBlock::executePhpBlock()
    participant FileSystem as File System
    participant BlockFunc as Block Function
    participant Handler as XOOPS Handlers

    rect rgba(200,150,100,0.5)
    note over BlockKernel,BlockFunc: Legacy Flow (eval)
    BlockKernel->>BlockKernel: eval(raw content) [guarded by XOOPS_ALLOW_PHP_BLOCKS]
    BlockFunc->>Handler: Query data (in-eval scope)
    Handler-->>BlockFunc: Return results
    BlockFunc-->>BlockKernel: HTML output
    end

    rect rgba(100,150,200,0.5)
    note over BlockKernel,Executor: New File-based Flow
    Client->>BlockKernel: getContent('P', "filename.php|func")
    BlockKernel->>Executor: executePhpBlock(content)
    Executor->>Executor: Parse "filename.php|function_name"
    Executor->>FileSystem: include_once custom_blocks/filename.php
    FileSystem-->>Executor: File loaded
    Executor->>BlockFunc: call function_name()
    BlockFunc->>Handler: Query data (isolated scope)
    Handler-->>BlockFunc: Return results
    BlockFunc-->>Executor: HTML string (echoed or returned)
    Executor->>Executor: Replace {X_SITEURL}
    Executor-->>BlockKernel: Processed HTML
    BlockKernel-->>Client: Rendered block
    end

    rect rgba(150,100,200,0.5)
    note over Executor: Backward compatibility / Fallback
    alt Not file-format or file invalid
        Executor->>Executor: if XOOPS_ALLOW_PHP_BLOCKS enabled
        Executor->>BlockKernel: eval(legacy content)
    else
        Executor-->>BlockKernel: return empty + log deprecation/warning
    end
    end

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely summarizes the main change: introducing a file-based PHP custom block system without eval().
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 9

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@docs/custom-blocks-tutorial.md`:
- Around line 53-55: Add a language specifier (e.g., text or plaintext) to the
fenced code blocks that show content field examples so markdownlint won't flag
them; for example change the fence around "my_block.php|b_custom_my_block_show"
to start with ```text and do the same for other content-field examples such as
"example_welcome.php|b_custom_welcome_show" and the remaining similar blocks in
this file.

In `@htdocs/custom_blocks/example_recent_members.php`:
- Line 46: The code uses $user->getVar('user_regdate') to build $date but
getVar() returns mixed; cast the value to int before passing to date() (as done
in example_welcome.php) to ensure type safety—e.g., replace usage of
$user->getVar('user_regdate') with (int)$user->getVar('user_regdate') when
computing $date so date('M j', ...) always receives an integer timestamp.
- Around line 54-55: The avatar URL is inserted directly into an img src
attribute (variable $avatarUrl) which is unsafe; update the HTML-building code
that appends to $html (the block that constructs the <img ... src="...">) to
escape $avatarUrl for HTML attribute context using an attribute-escaping
function such as htmlspecialchars with ENT_QUOTES and UTF-8 (or your framework's
equivalent) before concatenation so the src value is safely encoded.

In `@htdocs/kernel/block.php`:
- Around line 412-417: Replace the output-buffer handling around the evaluated
$raw content by using ob_get_clean() instead of the two-step ob_get_contents() +
ob_end_clean(): start the buffer with ob_start(), execute the eval/expression
that writes to the buffer (currently echo eval($raw)), then assign $content =
ob_get_clean() and continue returning str_replace('{X_SITEURL}', XOOPS_URL .
'/', $content); this simplifies and consolidates the buffer teardown around the
evaluated $raw output.

In `@tests/unit/htdocs/kernel/XoopsBlockPhpBlockTest.php`:
- Around line 1-12: Add the required XOOPS copyright header block at the very
top of tests/unit/htdocs/kernel/XoopsBlockPhpBlockTest.php (before
declare(strict_types=1)) so the file complies with XOOPS coding guidelines, and
ensure PHPUnit 9.6 compatibility by either installing the
phpunit/phpunit-attributes polyfill dependency or replacing/removing native
attributes (#[Test], #[DataProvider], #[CoversClass]) used in this test class so
the test runner recognizes them; update the file to include the header and
adjust attribute usage accordingly.
- Around line 236-272: The two test methods
contentFormatIsNotRecognizedAsFileBased and contentFormatIsRecognizedAsFileBased
are semantically different despite identical assertions, so add a one-line
clarifying comment inside each test explaining the code path being exercised
(for contentFormatIsNotRecognizedAsFileBased: "regex did NOT match -> falls
through to legacy eval path and returns '' because XOOPS_ALLOW_PHP_BLOCKS is not
enabled"; for contentFormatIsRecognizedAsFileBased: "regex matched -> attempts
file-based include but returns '' because file not found/blocked"), so readers
and SonarCloud reviewers can see why both expect '' but validate different
behavior.
- Around line 386-420: Restore any modified globals and add the missing import:
capture the original $GLOBALS['xoopsConfig'] at the start of
exampleWelcomeBlockReturnsHtmlForGuest and
exampleWelcomeBlockReturnsHtmlForLoggedInUser, set $GLOBALS['xoopsConfig'] for
the test, then after the assertions restore the original $GLOBALS['xoopsConfig']
(and keep the existing cleanup $GLOBALS['xoopsUser'] = null in the logged-in
test); also add a top-level import for the XoopsUser class (e.g., a use
statement for \XoopsUser) so instantiating XoopsUser in
exampleWelcomeBlockReturnsHtmlForLoggedInUser is PSR-12 compliant.
- Around line 165-177: The test uses restore_error_handler() and
restore_exception_handler() locally in
fileBasedBlockReturnsEmptyWhenFileNotFound which is fragile; instead, in the
test class implement setUp() to save/reset the global handlers (e.g. call
set_error_handler(...) to a noop and capture the previous handler in a property)
and implement tearDown() to call restore_error_handler() and
restore_exception_handler() to reliably restore the originals (this avoids
depending on XoopsLogger::getInstance() side-effects); remove the local restore
calls from fileBasedBlockReturnsEmptyWhenFileNotFound so the lifecycle is
managed by setUp/tearDown.
- Around line 21-470: The test class XoopsBlockPhpBlockTest is flagged for
having too many methods; split it into focused test classes such as
XoopsBlockPhpBlockExecutionTest (move execution tests like
fileBasedBlockExecutesFunctionAndReturnsContent, fileBasedBlockTrimsWhitespace,
fileBasedBlockHandlesNullReturn, multipleCallsToSameBlockFileUseIncludeOnce,
blockWithEmptyStringReturnedFromFunction), XoopsBlockPhpBlockSecurityTest (move
pathTraversalInFilenameIsRejected, absolutePathInFilenameIsRejected,
windowsPathInFilenameIsRejected, fileBasedBlockReturnsEmptyWhenFileNotFound,
fileBasedBlockReturnsEmptyWhenFunctionNotFound,
legacyBlockReturnsEmptyWhenConstantNotDefined,
contentFormatIsNotRecognizedAsFileBased and its provider), and
XoopsBlockPhpBlockExampleIntegrationTest (move exampleWelcomeBlockFileExists,
exampleRecentMembersBlockFileExists, exampleSiteStatsBlockFileExists,
exampleBlockFilesDefineExpectedFunctions,
exampleWelcomeBlockReturnsHtmlForGuest,
exampleWelcomeBlockReturnsHtmlForLoggedInUser); extract shared helpers (setUp,
$customBlocksDir, createPhpBlock, createTempBlockFile, removeTempBlockFile,
validContentFormatProvider, invalidContentFormatProvider) into a base TestCase
class or trait that each new class extends/uses so no duplication and ensure
provider methods stay public static; update class names and file names
accordingly and run tests to confirm nothing else depends on the original
XoopsBlockPhpBlockTest class.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 35e4a1fc-67ae-4b83-a57b-4a248d62289c

📥 Commits

Reviewing files that changed from the base of the PR and between efaed81 and b93286d.

📒 Files selected for processing (9)

• docs/custom-blocks-tutorial.md
• htdocs/custom_blocks/example_recent_members.php
• htdocs/custom_blocks/example_site_stats.php
• htdocs/custom_blocks/example_welcome.php
• htdocs/custom_blocks/index.php
• htdocs/kernel/block.php
• htdocs/modules/system/class/block.php
• htdocs/modules/system/language/english/admin/blocksadmin.php
• tests/unit/htdocs/kernel/XoopsBlockPhpBlockTest.php

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Missing XOOPS copyright header block.

Per coding guidelines, every PHP source file must begin with the XOOPS copyright header block. This file is missing it.

Additionally, PHPUnit attributes (#[Test], #[DataProvider], #[CoversClass]) are native to PHPUnit 10+. For PHPUnit 9.6 compatibility, ensure the phpunit/phpunit-attributes polyfill package is installed, or the test runner will silently ignore these attributes.

📄 Proposed fix for copyright header

 <?php
+/*
+ * 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
+ * which is considered copyrighted (c) material of the original comment or credit authors.
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
+ */

 declare(strict_types=1);

As per coding guidelines: "Every source file must begin with the XOOPS copyright header block."

📝 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

	<?php
	declare(strict_types=1);
	namespace kernel;
	use PHPUnit\Framework\Attributes\CoversClass;
	use PHPUnit\Framework\Attributes\DataProvider;
	use PHPUnit\Framework\Attributes\Test;
	use XoopsBlock;
	require_once XOOPS_ROOT_PATH . '/kernel/block.php';
	<?php
	/*
	* 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
	* which is considered copyrighted (c) material of the original comment or credit authors.
	* This program is distributed in the hope that it will be useful,
	* but WITHOUT ANY WARRANTY; without even the implied warranty of
	* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
	*/
	declare(strict_types=1);
	namespace kernel;
	use PHPUnit\Framework\Attributes\CoversClass;
	use PHPUnit\Framework\Attributes\DataProvider;
	use PHPUnit\Framework\Attributes\Test;
	use XoopsBlock;
	require_once XOOPS_ROOT_PATH . '/kernel/block.php';

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/unit/htdocs/kernel/XoopsBlockPhpBlockTest.php` around lines 1 - 12, Add
the required XOOPS copyright header block at the very top of
tests/unit/htdocs/kernel/XoopsBlockPhpBlockTest.php (before
declare(strict_types=1)) so the file complies with XOOPS coding guidelines, and
ensure PHPUnit 9.6 compatibility by either installing the
phpunit/phpunit-attributes polyfill dependency or replacing/removing native
attributes (#[Test], #[DataProvider], #[CoversClass]) used in this test class so
the test runner recognizes them; update the file to include the header and
adjust attribute usage accordingly.

[coderabbitai]

🧹 Nitpick | 🔵 Trivial

Fragile error handler restoration pattern.

The restore_error_handler() and restore_exception_handler() calls work here but could leave test state inconsistent if other tests also trigger XoopsLogger::getInstance(). Consider moving handler save/restore to setUp()/tearDown() for better isolation across all tests that might trigger the logger.

♻️ Suggested pattern for consistent handler management

// In setUp():
// $this->originalErrorHandler = set_error_handler(function() { return false; });
// restore_error_handler();

// In tearDown():
// restore_error_handler();
// restore_exception_handler();

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/unit/htdocs/kernel/XoopsBlockPhpBlockTest.php` around lines 165 - 177,
The test uses restore_error_handler() and restore_exception_handler() locally in
fileBasedBlockReturnsEmptyWhenFileNotFound which is fragile; instead, in the
test class implement setUp() to save/reset the global handlers (e.g. call
set_error_handler(...) to a noop and capture the previous handler in a property)
and implement tearDown() to call restore_error_handler() and
restore_exception_handler() to reliably restore the originals (this avoids
depending on XoopsLogger::getInstance() side-effects); remove the local restore
calls from fileBasedBlockReturnsEmptyWhenFileNotFound so the lifecycle is
managed by setUp/tearDown.

[Copilot]

Pull request overview

Refactors custom PHP blocks to a safer file-based callback mechanism (filename.php|function_name in htdocs/custom_blocks/) and updates docs/tests/examples to support migration away from database-stored eval() blocks.

Changes:

• Route PHP custom block rendering through a new executePhpBlock() helper instead of inlined eval() in getContent().
• Add a custom blocks directory with example block implementations and a migration/tutorial document.
• Add PHPUnit coverage for file-based block parsing/execution and some edge cases.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 12 comments.

Show a summary per file

File	Description
tests/unit/htdocs/kernel/XoopsBlockPhpBlockTest.php	Adds unit tests for the new file-based PHP block content format and behavior.
htdocs/modules/system/language/english/admin/blocksadmin.php	Updates admin label to clarify PHP blocks are now file-based.
htdocs/modules/system/class/block.php	Switches PHP custom content handling to call executePhpBlock() (inherited from XoopsBlock).
htdocs/kernel/block.php	Introduces executePhpBlock() and routes c_type=P through it.
htdocs/custom_blocks/index.php	Adds a guard entry point to prevent direct browsing/execution of the directory.
htdocs/custom_blocks/example_welcome.php	Provides an example “welcome” custom block file/function.
htdocs/custom_blocks/example_site_stats.php	Provides an example “site stats” custom block file/function.
htdocs/custom_blocks/example_recent_members.php	Provides an example “recent members” custom block file/function.
docs/custom-blocks-tutorial.md	Adds a migration guide/tutorial for moving off legacy eval-based blocks.

[Copilot]

Docs say legacy blocks “display a warning”, but the implementation currently records messages via XoopsLogger::addDeprecated(). Consider aligning the wording with what users will actually see (e.g., “a deprecation entry in the debug log”) or change the implementation to emit an E_USER_WARNING.

Suggested change

	legacy blocks display a warning and produce no output. New file-based blocks
	work without any special constant.
	legacy blocks add a deprecation entry to the XOOPS debug log and produce no output.
	New file-based blocks work without any special constant.

[Copilot]

This test calls restore_error_handler() / restore_exception_handler() but never captures the prior handler state (despite the comment). Consider explicitly saving the previous handlers (via set_error_handler(null) / set_exception_handler(...)) and restoring them in a finally so the test doesn’t depend on global handler stack state across the suite.

Suggested change

	$block = $this->createPhpBlock('nonexistent_block.php|b_custom_nonexistent_show');
	$result = $block->getContent('S', 'P');
	restore_error_handler();
	restore_exception_handler();
	$this->assertSame('', $result);
	$previousErrorHandler = set_error_handler(
	static function (): bool {
	return false;
	}
	);
	$previousExceptionHandler = set_exception_handler(
	static function (\Throwable $throwable): void {
	throw $throwable;
	}
	);
	try {
	$block = $this->createPhpBlock('nonexistent_block.php|b_custom_nonexistent_show');
	$result = $block->getContent('S', 'P');
	$this->assertSame('', $result);
	} finally {
	if ($previousErrorHandler !== null) {
	set_error_handler($previousErrorHandler);
	} else {
	restore_error_handler();
	}
	if ($previousExceptionHandler !== null) {
	set_exception_handler($previousExceptionHandler);
	} else {
	restore_exception_handler();
	}
	}

[Copilot]

executePhpBlock() retains a legacy eval() path when XOOPS_ALLOW_PHP_BLOCKS is true, but there’s no test covering the “allowed” branch (only the blocked case). Consider adding a separate-process test that defines the constant and asserts legacy eval blocks still execute (and don’t regress as the new file-based path evolves).

[Copilot]

executePhpBlock() calls the block function directly. If the included file or the block function uses echo/prints output (common in legacy blocks), that output will be sent to the response immediately instead of being returned from getContent(). Consider wrapping the include + function invocation in output buffering and combining buffered output with the function return value so getContent() stays side-effect free.

Suggested change

	include_once $filePath;
	if (!function_exists($showFunc)) {
	$logger = XoopsLogger::getInstance();
	$logger->addDeprecated("PHP block function not found: {$showFunc} in custom_blocks/{$funcFile}");
	return '';
	}
	$content = $showFunc();
	return str_replace('{X_SITEURL}', XOOPS_URL . '/', (string) $content);
	ob_start();
	include_once $filePath;
	if (!function_exists($showFunc)) {
	// Discard any output produced during include if function is missing
	ob_end_clean();
	$logger = XoopsLogger::getInstance();
	$logger->addDeprecated("PHP block function not found: {$showFunc} in custom_blocks/{$funcFile}");
	return '';
	}
	$content = $showFunc();
	$buffered = ob_get_clean();
	$combined = $buffered . (string) $content;
	return str_replace('{X_SITEURL}', XOOPS_URL . '/', $combined);

[Copilot]

addDeprecated() is intended for deprecated API usage and records a full backtrace, which is relatively expensive and can flood the deprecation log. Missing block files/functions are runtime warnings; prefer trigger_error(..., E_USER_WARNING) (or a logger API intended for warnings) and keep messages path-safe (e.g., only custom_blocks/<file>).

Suggested change

	$logger = XoopsLogger::getInstance();
	$logger->addDeprecated("PHP block file not found: custom_blocks/{$funcFile}");
	return '';
	}
	include_once $filePath;
	if (!function_exists($showFunc)) {
	$logger = XoopsLogger::getInstance();
	$logger->addDeprecated("PHP block function not found: {$showFunc} in custom_blocks/{$funcFile}");
	trigger_error(
	'PHP block file not found: custom_blocks/' . basename($funcFile),
	E_USER_WARNING
	);
	return '';
	}
	include_once $filePath;
	if (!function_exists($showFunc)) {
	trigger_error(
	'PHP block function not found: ' . $showFunc
	. ' in custom_blocks/' . basename($funcFile),
	E_USER_WARNING
	);

[Copilot]

The function_exists($showFunc) check allows calling any already-defined function (including internal/built-in ones like phpinfo) as long as the filename part points to an existing file. To avoid unexpected behavior and reduce attack surface, restrict callable functions to those defined by the included file (e.g., via ReflectionFunction($showFunc)->getFileName() === $filePath) and/or enforce an expected prefix/suffix like b_custom_*_show.

Suggested change

	try {
	$reflection = new \ReflectionFunction($showFunc);
	$functionFile = $reflection->getFileName();
	$realFilePath = realpath($filePath);
	// Only allow user-defined functions from the included file
	if ($reflection->isInternal()
	|| $functionFile === false
	|| $realFilePath === false
	|| $functionFile !== $realFilePath
	) {
	$logger = XoopsLogger::getInstance();
	$logger->addDeprecated(
	"Invalid PHP block function: {$showFunc} for custom_blocks/{$funcFile}"
	);
	return '';
	}
	} catch (\ReflectionException $e) {
	$logger = XoopsLogger::getInstance();
	$logger->addDeprecated(
	"Reflection failed for PHP block function: {$showFunc} in custom_blocks/{$funcFile}"
	);
	return '';
	}

[Copilot]

$uid is used directly in the profile URL. Cast it to int before concatenating into the query string to avoid unexpected output if getVar('uid') returns a non-scalar or stringy value.

[Copilot]

$newestUid is used in a URL query parameter without casting. Cast to int before concatenating into the URL to ensure consistent output and avoid propagating unexpected types from getVar() into HTML.

Suggested change

	$newestUid = !empty($newest) ? $newest[0]->getVar('uid') : 0;
	$newestUid = !empty($newest) ? (int)$newest[0]->getVar('uid') : 0;

[Copilot]

isResultSet() already guarantees a mysqli_result for the MySQLi driver (XoopsMySQLDatabase::isResultSet() uses is_a($result, 'mysqli_result')). The additional && $result instanceof \mysqli_result is redundant and can be removed to keep examples aligned with core DB conventions.

Suggested change

	if ($db->isResultSet($result) && $result instanceof \mysqli_result) {
	if ($db->isResultSet($result)) {

[Copilot]

The legacy PHP block fallback in executePhpBlock() still executes database-sourced content via eval($raw) when XOOPS_ALLOW_PHP_BLOCKS is enabled, which allows arbitrary PHP code execution if an attacker can modify the block content (e.g., via SQL injection or a compromised admin account). This creates a direct remote code execution path tied to a runtime configuration flag rather than to trusted, file-based code. To eliminate this risk, remove or hard-disable the eval-based path and require all custom PHP blocks to use the file-based filename.php|function_name format in custom_blocks/.

[coderabbitai]

Actionable comments posted: 6

♻️ Duplicate comments (3)

tests/unit/htdocs/kernel/XoopsBlockPhpBlockTest.php (2)

1-21: ⚠️ Potential issue | 🟡 Minor

Add the required XOOPS file header and complete the class docblock.

This new PHP source file still starts without the standard XOOPS header block, and the class docblock is missing the required metadata tags (@category, @package, @author, @copyright, @license, @link).

As per coding guidelines: "Every source file must begin with the XOOPS copyright header block" and "Class docblocks must include @category, @package, @author, @copyright, @license, and @link tags."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/unit/htdocs/kernel/XoopsBlockPhpBlockTest.php` around lines 1 - 21, Add
the standard XOOPS file header block at the top of this file and complete the
class docblock for XoopsBlockPhpBlockTest by adding the required tags:
`@category`, `@package`, `@author`, `@copyright`, `@license`, and `@link`; ensure the header
follows the project's canonical XOOPS copyright/header format and the
class-level docblock directly precedes the class declaration (class
XoopsBlockPhpBlockTest) so documentation and licensing metadata are present.

---

7-10: ⚠️ Potential issue | 🟠 Major

Verify the PHPUnit 9.6 lane can actually discover these attribute-based tests.

#[Test], #[DataProvider], and #[CoversClass] are natively supported in PHPUnit 10+/11. If the 9.6 job does not install phpunit/phpunit-attributes, this class will load but those test methods will be ignored, which breaks the stated cross-version support.

Run this read-only check to confirm whether the repository either ships the attributes polyfill or no longer runs a PHPUnit 9.6 lane:

#!/bin/bash
set -euo pipefail

echo "== Dependency manifests =="
fd 'composer(\.json|\.lock)$' -HI -E vendor | while read -r file; do
  echo "-- ${file} --"
  rg -n '"phpunit/phpunit"|"phpunit/phpunit-attributes"' "$file" || true
done

echo
echo "== CI workflow references =="
rg -n 'phpunit|9\.6|10\.5|11(\.|x)?' .github -g '*.yml' -g '*.yaml' || true

Expected result: either a PHPUnit 9.6 lane is gone, or the manifests show phpunit/phpunit-attributes is present anywhere PHPUnit 9.6 is still used.
As per coding guidelines: "Tests must work across PHPUnit 9.6, 10.5, and 11.x."

Also applies to: 20-21, 93-516

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/unit/htdocs/kernel/XoopsBlockPhpBlockTest.php` around lines 7 - 10, The
attribute-based tests in XoopsBlockPhpBlockTest.php use #[Test],
#[DataProvider], and #[CoversClass] which require the phpunit/phpunit-attributes
polyfill under PHPUnit 9.6; update the repo so 9.6 lanes will discover these
tests by either (A) ensuring composer dev deps include
"phpunit/phpunit-attributes" (add it to composer.json require-dev and
composer.lock) when "phpunit/phpunit" 9.6 is present, or (B) remove/stop running
any PHPUnit 9.6 CI job (update .github workflows) so only PHPUnit 10/11 run
natively supporting those attributes; run the provided discovery script to
verify either the polyfill is present in manifests or the 9.6 lane is removed.

docs/custom-blocks-tutorial.md (1)

53-55: ⚠️ Potential issue | 🟡 Minor

Annotate these plaintext fences.

These content-field examples are still bare fenced blocks, so markdownlint will keep flagging them. Mark them as text and keep blank lines around the fences, especially for the list item under Line 52.

Also applies to: 67-69, 100-102, 133-135, 167-169, 242-244

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docs/custom-blocks-tutorial.md` around lines 53 - 55, Change the bare fenced
blocks to explicitly annotated text fences and ensure a blank line before and
after each fence: replace ``` with ```text for the block containing the literal
"my_block.php|b_custom_my_block_show" and any other similar fenced examples, and
add a blank line above and below the fenced block (particularly when the fence
is inside a list item); apply the same update to the other occurrences of these
plaintext fences so markdownlint no longer flags them.

🤖 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/custom_blocks/example_recent_members.php`:
- Around line 1-16: The file example_recent_members.php currently contains only
an example docblock; add the standard XOOPS source header block at the very top
of the file (replacing or preceding the existing docblock) so the file matches
the repository requirement that every PHP source begins with the XOOPS copyright
header; ensure the header format and copyright years match other htdocs files
and keep the existing example docblock content (the Recent Members description)
after the XOOPS header.

In `@htdocs/custom_blocks/example_site_stats.php`:
- Around line 1-16: The file example_site_stats.php is missing the required
XOOPS file-level copyright header; add the standard XOOPS source header block
immediately at the top of the file before the existing docblock (i.e., insert
the XOOPS copyright/license header used across other htdocs PHP files so the
current example docblock remains intact and the file complies with the "Every
source file must begin with the XOOPS copyright header block" guideline).
- Around line 45-53: Replace the ad-hoc SQL block that uses
XoopsDatabaseFactory::getDatabaseConnection(), $db->prefix('xoopscomments') and
manual query/fetch with the comment handler call; call
xoops_getHandler('comment')->getCount() and assign its return to $totalPosts,
removing the manual $db->query/$db->fetchRow logic so the code uses the built-in
comment handler (getCount) instead of duplicating SQL.

In `@htdocs/kernel/block.php`:
- Around line 378-390: The current block inclusion uses file_exists() and
include_once on $filePath (constructed from XOOPS_ROOT_PATH . '/custom_blocks/'
. $funcFile) which allows symlinks in custom_blocks to escape the directory; fix
by resolving the blocks root and candidate with realpath() and validating the
candidate path starts with the realpath of XOOPS_ROOT_PATH . '/custom_blocks'
before including: after preg_match() compute $blocksRootReal =
realpath(XOOPS_ROOT_PATH . '/custom_blocks') and $candidateReal =
realpath($filePath) (handle false return), if $candidateReal is false or
strpos($candidateReal, $blocksRootReal) !== 0 then log the deprecated error with
$logger->addDeprecated("PHP block file not found: custom_blocks/{$funcFile}")
and return '' otherwise proceed to include_once the validated $candidateReal.
- Around line 389-421: The output buffering around dynamic block execution (the
include/callback path using include_once $filePath and $showFunc(), and the
legacy eval path using eval($raw)) must be wrapped in try/catch(\Throwable) with
a finally that always cleans the buffer; wrap the existing ob_start() ...
ob_get_clean() sequences so that any thrown exception still triggers
ob_get_clean() (or ob_end_clean()/ob_get_level-safe cleanup) in finally, rethrow
or log the exception in the catch, and return the sanitized content replacement
(str_replace('{X_SITEURL}', XOOPS_URL . '/', ...)) as before; apply the same
try/catch/finally pattern around the include_once/$showFunc() block and around
the eval($raw) branch to prevent output-buffer leaks.

In `@tests/unit/htdocs/kernel/XoopsBlockPhpBlockTest.php`:
- Around line 35-86: The tests currently write temp files into the real
htdocs/custom_blocks because XoopsBlock resolution is hardcoded; refactor
XoopsBlock to expose the custom-block directory via an overridable API (e.g.,
add a protected static getCustomBlocksDir() or a protected method used by the
loader in kernel/block.php instead of XOOPS_ROOT_PATH . '/custom_blocks'),
update kernel/block.php to call that API (e.g.,
XoopsBlock::getCustomBlocksDir()), and update tests (setUp, createTempBlockFile,
removeTempBlockFile) to create a per-test temp directory and either subclass
XoopsBlock or set the new accessor to point to that temp dir so temp files are
written to an isolated location instead of the checked-in htdocs tree.

---

Duplicate comments:
In `@docs/custom-blocks-tutorial.md`:
- Around line 53-55: Change the bare fenced blocks to explicitly annotated text
fences and ensure a blank line before and after each fence: replace ``` with
```text for the block containing the literal
"my_block.php|b_custom_my_block_show" and any other similar fenced examples, and
add a blank line above and below the fenced block (particularly when the fence
is inside a list item); apply the same update to the other occurrences of these
plaintext fences so markdownlint no longer flags them.

In `@tests/unit/htdocs/kernel/XoopsBlockPhpBlockTest.php`:
- Around line 1-21: Add the standard XOOPS file header block at the top of this
file and complete the class docblock for XoopsBlockPhpBlockTest by adding the
required tags: `@category`, `@package`, `@author`, `@copyright`, `@license`, and `@link`;
ensure the header follows the project's canonical XOOPS copyright/header format
and the class-level docblock directly precedes the class declaration (class
XoopsBlockPhpBlockTest) so documentation and licensing metadata are present.
- Around line 7-10: The attribute-based tests in XoopsBlockPhpBlockTest.php use
#[Test], #[DataProvider], and #[CoversClass] which require the
phpunit/phpunit-attributes polyfill under PHPUnit 9.6; update the repo so 9.6
lanes will discover these tests by either (A) ensuring composer dev deps include
"phpunit/phpunit-attributes" (add it to composer.json require-dev and
composer.lock) when "phpunit/phpunit" 9.6 is present, or (B) remove/stop running
any PHPUnit 9.6 CI job (update .github workflows) so only PHPUnit 10/11 run
natively supporting those attributes; run the provided discovery script to
verify either the polyfill is present in manifests or the 9.6 lane is removed.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 53120cc3-0745-448c-8a2a-1bb21407de32

📥 Commits

Reviewing files that changed from the base of the PR and between b93286d and 6da66d8.

📒 Files selected for processing (5)

• docs/custom-blocks-tutorial.md
• htdocs/custom_blocks/example_recent_members.php
• htdocs/custom_blocks/example_site_stats.php
• htdocs/kernel/block.php
• tests/unit/htdocs/kernel/XoopsBlockPhpBlockTest.php

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Add the standard XOOPS source header to this example file.

Like the other checked-in PHP sources under htdocs/, this file should begin with the repository’s XOOPS copyright header block instead of only the example docblock.

As per coding guidelines: "Every source file must begin with the XOOPS copyright header block."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@htdocs/custom_blocks/example_recent_members.php` around lines 1 - 16, The
file example_recent_members.php currently contains only an example docblock; add
the standard XOOPS source header block at the very top of the file (replacing or
preceding the existing docblock) so the file matches the repository requirement
that every PHP source begins with the XOOPS copyright header; ensure the header
format and copyright years match other htdocs files and keep the existing
example docblock content (the Recent Members description) after the XOOPS
header.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Add the standard XOOPS source header here.

This is a checked-in PHP source file under htdocs/, so it should start with the required XOOPS copyright header block before the example-specific docblock.

As per coding guidelines: "Every source file must begin with the XOOPS copyright header block."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@htdocs/custom_blocks/example_site_stats.php` around lines 1 - 16, The file
example_site_stats.php is missing the required XOOPS file-level copyright
header; add the standard XOOPS source header block immediately at the top of the
file before the existing docblock (i.e., insert the XOOPS copyright/license
header used across other htdocs PHP files so the current example docblock
remains intact and the file complies with the "Every source file must begin with
the XOOPS copyright header block" guideline).

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 9 comments.

[Copilot]

If XOOPS_ALLOW_PHP_BLOCKS is true, any content that looks like a file-based reference but fails the regex (e.g. file.php|func-name) will fall through to the legacy eval() branch and can trigger a fatal parse error. Consider adding a guard so strings containing a | (or matching *.php|*) are treated as an invalid file-based reference (log + return '') rather than being eval'd.

[Copilot]

This example builds avatar URLs with XOOPS_URL . '/uploads/' ... and only treats 'avatars/blank.gif' as the blank avatar. In core code, avatar URLs are typically built with XOOPS_UPLOAD_URL and the stored value may be 'blank.gif' or 'avatars/blank.gif'. Using XOOPS_UPLOAD_URL . '/' . ltrim($avatar, '/') and checking basename($avatar) === 'blank.gif' would make the example work across common configurations.

Suggested change

	$avatarUrl = XOOPS_URL . '/uploads/' . $avatar;
	if (empty($avatar) || $avatar === 'avatars/blank.gif') {
	$avatarUrl = XOOPS_URL . '/uploads/avatars/blank.gif';
	$avatarUrl = XOOPS_UPLOAD_URL . '/' . ltrim((string) $avatar, '/');
	if (empty($avatar) || basename((string) $avatar) === 'blank.gif') {
	$avatarUrl = XOOPS_UPLOAD_URL . '/avatars/blank.gif';

[Copilot]

These new PHP files don’t follow the standard XOOPS source header/docblock format used across core (copyright + “You may not change…” notice + @Package, etc.). To keep licensing/metadata consistent, consider updating the file header to match the established header blocks used in other htdocs/* PHP files.

Suggested change

	* @copyright (c) 2000-2026 XOOPS Project (https://xoops.org)
	* @license GNU GPL 2.0 or later (https://www.gnu.org/licenses/gpl-2.0.html)
	* 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 which is considered copyrighted (c) material of the
	* original comment or credit authors.
	*
	* @copyright (c) 2000-2026 XOOPS Project (https://xoops.org)
	* @license GNU GPL 2.0 or later (https://www.gnu.org/licenses/gpl-2.0.html)
	* @author XOOPS Project (https://xoops.org)
	* @package custom_blocks
	* @since 2.5.0
	* @link https://xoops.org XOOPS Project

[Copilot]

This new file header doesn’t match the standard XOOPS source header/docblock format used across core (copyright + “You may not change…” notice + @Package, etc.). Consider aligning the header block with other htdocs/* PHP files for consistency.

Suggested change

	* @copyright (c) 2000-2026 XOOPS Project (https://xoops.org)
	* @license GNU GPL 2.0 or later (https://www.gnu.org/licenses/gpl-2.0.html)
	* @see docs/custom-blocks-tutorial.md
	* 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
	* which is considered copyrighted (c) material of the original comment or credit authors.
	*
	* This program is distributed in the hope that it will be useful,
	* but WITHOUT ANY WARRANTY; without even the implied warranty of
	* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
	*
	* @category XoopsCore25
	* @package CustomBlocks
	* @author XOOPS Project (https://xoops.org)
	* @copyright (c) 2000-2026 XOOPS Project (https://xoops.org)
	* @license GNU GPL 2.0 or later (https://www.gnu.org/licenses/gpl-2.0.html)
	* @link https://xoops.org
	* @see docs/custom-blocks-tutorial.md

[Copilot]

These new PHP files don’t follow the standard XOOPS source header/docblock format used across core (copyright + “You may not change…” notice + @Package, etc.). To keep licensing/metadata consistent, consider updating the file header to match the established header blocks used in other htdocs/* PHP files.

Suggested change

	* @copyright (c) 2000-2026 XOOPS Project (https://xoops.org)
	* @license GNU GPL 2.0 or later (https://www.gnu.org/licenses/gpl-2.0.html)
	* 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 which is considered copyrighted (c) material of the
	* original comment or credit authors.
	*
	* This program is distributed in the hope that it will be useful,
	* but WITHOUT ANY WARRANTY; without even the implied warranty of
	* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
	*
	* @category Xoops\Core
	* @package CustomBlocks
	* @author XOOPS Project
	* @copyright (c) 2000-2026 XOOPS Project (https://xoops.org)
	* @license GNU GPL 2.0 or later (https://www.gnu.org/licenses/gpl-2.0.html)
	* @link https://xoops.org

[Copilot]

These new PHP files don’t follow the standard XOOPS source header/docblock format used across core (copyright + “You may not change…” notice + @Package, etc.). To keep licensing/metadata consistent, consider updating the file header to match the established header blocks used in other htdocs/* PHP files.

Suggested change

	* @copyright (c) 2000-2026 XOOPS Project (https://xoops.org)
	* @license GNU GPL 2.0 or later (https://www.gnu.org/licenses/gpl-2.0.html)
	* 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 which is considered copyrighted (c) material of the
	* original comment or credit authors.
	*
	* This program is distributed in the hope that it will be useful,
	* but WITHOUT ANY WARRANTY; without even the implied warranty of
	* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
	*
	* @category XoopsBlock
	* @package CustomBlocks
	* @author XOOPS Development Team
	* @copyright (c) 2000-2026 XOOPS Project (https://xoops.org)
	* @license GNU GPL 2.0 or later (https://www.gnu.org/licenses/gpl-2.0.html)
	* @link https://xoops.org XOOPS Project

[Copilot]

function_exists($showFunc) allows calling any already-defined global function (e.g. a built-in) even if the included file does not define it. To keep the dispatch constrained to custom block callbacks, consider enforcing a naming pattern like b_custom_*_show and/or verifying the function was introduced by the included file (e.g., diff get_defined_functions()['user'] before/after include).

[Copilot]

executePhpBlock() derives $showFunc directly from the untrusted block content field and then calls $showFunc() after including a file, only validating the string format and not which function is actually executed. An attacker who can modify block content (e.g., via SQL injection or a compromised admin account) could point a block at any existing file and invoke arbitrary global functions such as phpinfo, leaking environment variables and other sensitive configuration. To harden this, restrict allowed function names to a safe prefix (for example b_custom_...) or otherwise ensure the callable must be defined in the included file instead of accepting any global function name.

[Copilot]

The legacy branch of executePhpBlock() still passes the raw block content field straight into eval($raw), executing PHP that is stored in the database. If an attacker can write to the blocks table (via SQL injection, database compromise, or abuse of lower-privileged admin functionality), they can gain full server-side code execution by crafting a malicious PHP block. To mitigate this, remove or aggressively limit this eval path (for example by disabling it entirely, or tightly gating it behind configuration and permissions and migrating remaining blocks to the file-based format).

Suggested change

	// Legacy eval()-based PHP blocks (backward compatibility)
	if (!(defined('XOOPS_ALLOW_PHP_BLOCKS') && constant('XOOPS_ALLOW_PHP_BLOCKS') === true)) {
	$logger = XoopsLogger::getInstance();
	$logger->addDeprecated(
	'Legacy PHP block detected. Migrate to file-based format '
	. '(filename.php|function_name in custom_blocks/) or set '
	. 'XOOPS_ALLOW_PHP_BLOCKS to true in mainfile.php.'
	);
	return '';
	}
	ob_start();
	echo eval($raw);
	$content = ob_get_clean();
	return str_replace('{X_SITEURL}', XOOPS_URL . '/', $content);
	// Legacy eval()-based PHP blocks are no longer supported for security reasons.
	$logger = XoopsLogger::getInstance();
	$logger->addDeprecated(
	'Legacy eval()-based PHP blocks are no longer executed. '
	. 'Please migrate this block to the file-based format '
	. '(filename.php|function_name in custom_blocks/).'
	);
	return '';

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.

[Copilot]

XoopsLogger::addDeprecated() captures a full debug_backtrace() for each call, which is relatively expensive and can spam the deprecated log when a block is misconfigured (missing file/function) or when legacy eval blocks are present. Since these are runtime warnings/errors rather than deprecations, consider switching to addWarning() (or similar) and/or rate-limiting so one misconfigured block doesn’t generate a backtrace on every request.

[Copilot]

This legacy-eval blocked path logs via addDeprecated() on every render when XOOPS_ALLOW_PHP_BLOCKS is not enabled. Because addDeprecated() records a backtrace, a site with multiple legacy PHP blocks could incur significant overhead and very noisy logs on each request.

Consider logging this at most once per request (or once per block id), and/or using a cheaper log method (e.g. addWarning()) for this runtime configuration issue.

Suggested change

	$logger = XoopsLogger::getInstance();
	$logger->addDeprecated(
	'Legacy PHP block detected. Migrate to file-based format '
	. '(filename.php|function_name in custom_blocks/) or set '
	. 'XOOPS_ALLOW_PHP_BLOCKS to true in mainfile.php.'
	);
	static $legacyPhpBlockWarningLogged = false;
	if (!$legacyPhpBlockWarningLogged) {
	$logger = XoopsLogger::getInstance();
	$logger->addWarning(
	'Legacy PHP block detected. Migrate to file-based format '
	. '(filename.php|function_name in custom_blocks/) or set '
	. 'XOOPS_ALLOW_PHP_BLOCKS to true in mainfile.php.'
	);
	$legacyPhpBlockWarningLogged = true;
	}

[Copilot]

function_exists($showFunc) only checks for a global function name, not that the function was defined by the included block file. This means a block can call any already-defined/built-in function (e.g. phpinfo) as long as the referenced file exists, which is broader than the intended “file-based block function” dispatch.

Consider validating that the callable originates from $realPath (e.g., via ReflectionFunction($showFunc)->getFileName()), and/or restricting allowed function names (such as requiring a b_custom_... prefix) before invoking it.

Suggested change

	try {
	$reflection = new \ReflectionFunction($showFunc);
	$funcFile = $reflection->getFileName();
	if ($funcFile === false || realpath($funcFile) !== $realPath) {
	$logger = XoopsLogger::getInstance();
	$logger->addDeprecated("PHP block function origin mismatch: {$showFunc} in custom_blocks/{$funcFile}");
	return '';
	}
	} catch (\Throwable $e) {
	$logger = XoopsLogger::getInstance();
	$logger->addDeprecated("PHP block function reflection error for {$showFunc} in custom_blocks/{$funcFile}: " . $e->getMessage());
	return '';
	}

[Copilot]

The legacy fallback in executePhpBlock() still uses eval($raw) on the block content, which is loaded from the database, whenever XOOPS_ALLOW_PHP_BLOCKS is set to true. An attacker who can modify block content in the database (e.g. via SQL injection, compromised backup, or a rogue privileged admin) can inject arbitrary PHP into this field and gain server-side code execution whenever the block is rendered. To eliminate this risk, remove the eval-based execution path entirely (or strictly constrain legacy behavior to a safe, file-based mechanism) and require migration to the new file-based block format instead of enabling execution of raw PHP strings.

Suggested change

	// Legacy eval()-based PHP blocks (backward compatibility)
	if (!(defined('XOOPS_ALLOW_PHP_BLOCKS') && constant('XOOPS_ALLOW_PHP_BLOCKS') === true)) {
	$logger = XoopsLogger::getInstance();
	$logger->addDeprecated(
	'Legacy PHP block detected. Migrate to file-based format '
	. '(filename.php|function_name in custom_blocks/) or set '
	. 'XOOPS_ALLOW_PHP_BLOCKS to true in mainfile.php.'
	);
	return '';
	}
	$obLevel = ob_get_level();
	ob_start();
	try {
	echo eval($raw); // NOSONAR — legacy fallback, gated by XOOPS_ALLOW_PHP_BLOCKS
	$content = ob_get_clean();
	return str_replace('{X_SITEURL}', XOOPS_URL . '/', $content);
	} catch (\Throwable $e) {
	$logger = XoopsLogger::getInstance();
	$logger->addDeprecated('Legacy PHP block error: ' . $e->getMessage());
	return '';
	} finally {
	// Clean up only the output buffers we opened
	while (ob_get_level() > $obLevel) {
	ob_end_clean();
	}
	}
	// Legacy eval()-based PHP blocks are no longer supported for security reasons.
	$logger = XoopsLogger::getInstance();
	$logger->addDeprecated(
	'Legacy PHP block detected. Execution of raw PHP from block content is disabled. '
	. 'Migrate this block to the file-based format (filename.php|function_name in custom_blocks/).'
	);
	return '';

[coderabbitai]

Actionable comments posted: 1

♻️ Duplicate comments (1)

htdocs/kernel/block.php (1)

389-398: 🧹 Nitpick | 🔵 Trivial

Solid symlink traversal prevention.

The realpath() validation with boundary check correctly ensures the resolved path stays within custom_blocks/. The inclusion of DIRECTORY_SEPARATOR in the prefix check prevents directory-prefix attacks (e.g., custom_blocks_evil/ wouldn't match custom_blocks/).

For PHP 8.0+ readability, consider using str_starts_with():

♻️ Optional refactor

-            if ($blocksRoot === false || $realPath === false
-                || strpos($realPath, $blocksRoot . DIRECTORY_SEPARATOR) !== 0
-            ) {
+            if ($blocksRoot === false || $realPath === false
+                || !str_starts_with($realPath, $blocksRoot . DIRECTORY_SEPARATOR)
+            ) {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@htdocs/kernel/block.php` around lines 389 - 398, The check using strpos to
ensure $realPath is inside $blocksRoot can be replaced with PHP 8's clearer
str_starts_with for readability: update the conditional that currently tests
strpos($realPath, $blocksRoot . DIRECTORY_SEPARATOR) !== 0 to use
!str_starts_with($realPath, $blocksRoot . DIRECTORY_SEPARATOR) while keeping the
existing realpath() false checks and the same logging/return behavior (see
$blocksRoot, $realPath, XoopsLogger::getInstance(), and the deprecated log
message referencing $funcFile); ensure the codebase targets PHP 8+ before
applying this refactor.

🤖 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/kernel/block.php`:
- Around line 373-456: The method executePhpBlock has high cognitive complexity
and many return points; extract the file-format detection, file-based execution,
and legacy eval flow into helpers to reduce complexity: implement
isFileBasedFormat(string $raw, ?string &$funcFile, ?string &$showFunc): bool to
encapsulate the regex, executeFileBasedBlock(string $funcFile, string
$showFunc): string to perform file existence/realpath checks, include_once,
function_exists check, buffering and error handling, and
executeLegacyEvalBlock(string $raw): string to handle XOOPS_ALLOW_PHP_BLOCKS
gating, eval buffering and errors, then refactor executePhpBlock to call those
helpers and return their results.

---

Duplicate comments:
In `@htdocs/kernel/block.php`:
- Around line 389-398: The check using strpos to ensure $realPath is inside
$blocksRoot can be replaced with PHP 8's clearer str_starts_with for
readability: update the conditional that currently tests strpos($realPath,
$blocksRoot . DIRECTORY_SEPARATOR) !== 0 to use !str_starts_with($realPath,
$blocksRoot . DIRECTORY_SEPARATOR) while keeping the existing realpath() false
checks and the same logging/return behavior (see $blocksRoot, $realPath,
XoopsLogger::getInstance(), and the deprecated log message referencing
$funcFile); ensure the codebase targets PHP 8+ before applying this refactor.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: ebb411b3-62cb-4cb9-a9e6-d2ae220c5ea3

📥 Commits

Reviewing files that changed from the base of the PR and between 6da66d8 and addf58d.

📒 Files selected for processing (3)

• docs/custom-blocks-tutorial.md
• htdocs/custom_blocks/example_site_stats.php
• htdocs/kernel/block.php

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 6 comments.

[Copilot]

The legacy fallback currently refuses to eval any PHP block content containing a "|" character. That’s a backward-compatibility break even when XOOPS_ALLOW_PHP_BLOCKS=true (e.g., bitwise OR, strings, regex, etc.). Consider narrowing this guard to only block content that appears to be a file-based reference attempt (for example, matching something like /\.php\|/), and otherwise allow the legacy eval path when explicitly enabled.

Suggested change

	// Guard: content containing '|' looks like a malformed file-based reference — don't eval it
	if (strpos($raw, '|') !== false) {
	$logger = XoopsLogger::getInstance();
	$logger->addExtra('Block Warning',
	'PHP block content contains "|" but did not match file-based format. '
	. 'Check the content field syntax: filename.php|function_name'
	// Guard: content that looks like a malformed file-based reference (e.g. "*.php|") — don't eval it
	if (preg_match('/\.php\|/i', $raw)) {
	$logger = XoopsLogger::getInstance();
	$logger->addExtra(
	'Block Warning',
	'PHP block content appears to reference a PHP file but did not match the expected '
	. 'file-based format. Check the content field syntax: filename.php|function_name'

[Copilot]

The file-based function-name regex allows values that are not valid PHP function identifiers (e.g., starting with a digit). Tightening this to PHP’s identifier rules (e.g., [A-Za-z_][A-Za-z0-9_]*) will make validation clearer and avoid confusing "function not found" cases for inputs that can never be callable.

Suggested change

	if (preg_match('/^([a-zA-Z0-9_\-]+\.php)\|([a-zA-Z0-9_]+)$/', trim($raw), $matches)) {
	if (preg_match('/^([a-zA-Z0-9_\-]+\.php)\|([A-Za-z_][A-Za-z0-9_]*)$/', trim($raw), $matches)) {

[Copilot]

This index.php guard differs from the project’s common pattern of returning a simple 404 (many directories use header('HTTP/1.0 404 Not Found'); with no body). Consider aligning to that convention here to reduce information disclosure and keep guard files consistent across the tree.

Suggested change

	header('HTTP/1.1 403 Forbidden');
	exit('Restricted access');
	header('HTTP/1.0 404 Not Found');
	exit;

[Copilot]

htmlspecialchars() is called without an explicit encoding. To avoid relying on default_charset (and to match safer patterns used elsewhere in the codebase), pass the charset explicitly (typically 'UTF-8') when escaping user-controlled values like uname.

Suggested change

	$uname = htmlspecialchars($xoopsUser->getVar('uname'), ENT_QUOTES | ENT_HTML5);
	$html .= '<p>Welcome back, <strong>' . $uname . '</strong>!</p>';
	$html .= '<p>You have been a member since '
	. date('F j, Y', (int) $xoopsUser->getVar('user_regdate')) . '.</p>';
	} else {
	$sitename = htmlspecialchars($xoopsConfig['sitename'], ENT_QUOTES | ENT_HTML5);
	$uname = htmlspecialchars($xoopsUser->getVar('uname'), ENT_QUOTES | ENT_HTML5, 'UTF-8');
	$html .= '<p>Welcome back, <strong>' . $uname . '</strong>!</p>';
	$html .= '<p>You have been a member since '
	. date('F j, Y', (int) $xoopsUser->getVar('user_regdate')) . '.</p>';
	} else {
	$sitename = htmlspecialchars($xoopsConfig['sitename'], ENT_QUOTES | ENT_HTML5, 'UTF-8');

[Copilot]

htmlspecialchars() is called without an explicit encoding when escaping $xoopsConfig['sitename']. Please pass the charset explicitly (typically 'UTF-8') to avoid XSS edge cases if default_charset is misconfigured.

Suggested change

	$uname = htmlspecialchars($xoopsUser->getVar('uname'), ENT_QUOTES | ENT_HTML5);
	$html .= '<p>Welcome back, <strong>' . $uname . '</strong>!</p>';
	$html .= '<p>You have been a member since '
	. date('F j, Y', (int) $xoopsUser->getVar('user_regdate')) . '.</p>';
	} else {
	$sitename = htmlspecialchars($xoopsConfig['sitename'], ENT_QUOTES | ENT_HTML5);
	$uname = htmlspecialchars($xoopsUser->getVar('uname'), ENT_QUOTES | ENT_HTML5, 'UTF-8');
	$html .= '<p>Welcome back, <strong>' . $uname . '</strong>!</p>';
	$html .= '<p>You have been a member since '
	. date('F j, Y', (int) $xoopsUser->getVar('user_regdate')) . '.</p>';
	} else {
	$sitename = htmlspecialchars($xoopsConfig['sitename'], ENT_QUOTES | ENT_HTML5, 'UTF-8');

[Copilot]

htmlspecialchars() is used without an explicit encoding when escaping usernames (and also when escaping $avatarUrl later in this file). Please pass the charset explicitly (typically 'UTF-8') for consistent, predictable escaping.

[coderabbitai]

Actionable comments posted: 3

♻️ Duplicate comments (4)

htdocs/custom_blocks/example_recent_members.php (1)

1-16: ⚠️ Potential issue | 🟡 Minor

Add the standard XOOPS source header above the example docblock.

This file still starts with only the example-specific docblock, so it misses the required repository header for checked-in PHP sources.

As per coding guidelines: "Every source file must begin with the XOOPS copyright header block."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@htdocs/custom_blocks/example_recent_members.php` around lines 1 - 16, This
file is missing the standard XOOPS repository header; insert the required XOOPS
copyright/source header block at the very top of
htdocs/custom_blocks/example_recent_members.php before the existing file-level
docblock so the file complies with "Every source file must begin with the XOOPS
copyright header block"; keep the existing example docblock and any references
(e.g., the suggested block callback b_custom_recent_members_show) unchanged and
only prepend the official header.

tests/unit/htdocs/kernel/XoopsBlockPhpBlockTest.php (3)

42-50: ⚠️ Potential issue | 🟠 Major

These helpers still write temp blocks into the checked-in htdocs/custom_blocks/ tree.

tests/bootstrap.php:15-18 maps XOOPS_ROOT_PATH to the real repo htdocs/, so setUp() and createTempBlockFile() mutate production source files and are not parallel-safe. Please route the loader through an overridable custom-block root and point the tests at a per-test temp directory instead.

Based on learnings: "Tests must be fully isolated with no XOOPS installation required for unit tests."

Also applies to: 78-88

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/unit/htdocs/kernel/XoopsBlockPhpBlockTest.php` around lines 42 - 50,
The tests currently create files under XOOPS_ROOT_PATH (set in
tests/bootstrap.php) via setUp() and createTempBlockFile(), mutating the
checked-in htdocs/custom_blocks; change the tests to create an isolated per-test
temp directory (use sys_get_temp_dir()/tempnam or mkdir with a unique prefix)
and set $this->customBlocksDir to that path, and ensure tearDown() removes it;
update the loader/path resolution used by the code under test (make the
custom-block root overridable via a constructor arg, setter, or config/env
variable) so XoopsBlockPhpBlockTest::setUp() and createTempBlockFile() point the
loader at the per-test temp directory instead of
XOOPS_ROOT_PATH.'/custom_blocks', ensuring no writes to the repo and tests are
parallel-safe (also update the same pattern in the other affected block around
lines 78-88).

---

1-3: ⚠️ Potential issue | 🟡 Minor

Add the standard XOOPS source header before declare(strict_types=1).

This test file is still missing the required repository header block at the top.

As per coding guidelines: "Every source file must begin with the XOOPS copyright header block."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/unit/htdocs/kernel/XoopsBlockPhpBlockTest.php` around lines 1 - 3, The
file XoopsBlockPhpBlockTest.php is missing the required XOOPS copyright/header
block at the top; add the standard XOOPS source header comment block immediately
before the existing declare(strict_types=1) line so the header appears at the
very top of the file (maintain exact header text used across the repository),
leaving the existing file content (including declare(strict_types=1) and the
XoopsBlockPhpBlockTest class) unchanged.

---

7-20: ⚠️ Potential issue | 🟠 Major

Verify the PHPUnit 9.6 lane can still discover this class.

Every test here relies on native attributes, and none of the methods use the test... naming fallback. If the 9.6 job does not install the attribute support package, this whole class will be skipped there.

Run this to confirm the repo ships the attribute support needed for the advertised PHPUnit matrix:

#!/bin/bash
set -euo pipefail

fd -HI '^composer\.(json|lock)$' | while read -r file; do
  echo "== $file =="
  python - "$file" <<'PY'
import json, sys
path = sys.argv[1]
with open(path, encoding='utf-8') as fh:
    data = json.load(fh)

if path.endswith('composer.json'):
    for section in ('require', 'require-dev'):
        deps = data.get(section, {})
        for name in ('phpunit/phpunit', 'phpunit/phpunit-attributes'):
            if name in deps:
                print(f'{section}:{name}={deps[name]}')
else:
    for section in ('packages', 'packages-dev'):
        for pkg in data.get(section, []):
            if pkg.get('name') in ('phpunit/phpunit', 'phpunit/phpunit-attributes'):
                print(f'{section}:{pkg["name"]}={pkg.get("version")}')
PY
done

Expected: either PHPUnit 9.6 is no longer part of the support matrix, or the attribute support package is present. Otherwise these methods need a 9.6-compatible discovery fallback.

As per coding guidelines: "Tests must work across PHPUnit 9.6, 10.5, and 11.x."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/unit/htdocs/kernel/XoopsBlockPhpBlockTest.php` around lines 7 - 20, The
test class XoopsBlockPhpBlockTest uses native PHPUnit attributes
(PHPUnit\Framework\Attributes\Test, DataProvider, CoversClass) which PHPUnit 9.6
may not discover unless the phpunit/phpunit-attributes polyfill is shipped;
either add phpunit/phpunit-attributes to composer (require-dev) so 9.6 lanes can
discover attribute-based tests, or provide a 9.6-compatible fallback in the test
class (e.g., add test*-prefixed method aliases or `@test` docblock annotations for
each attribute-only test method in XoopsBlockPhpBlockTest) so tests run across
PHPUnit 9.6, 10.5 and 11.x.

🤖 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/custom_blocks/example_recent_members.php`:
- Around line 45-50: Reject traversal segments before composing the avatar URL
by validating $avatar: keep the existing blank/ basename('blank.gif') check but
first normalize and validate $avatar (used to build $avatarUrl with
XOOPS_UPLOAD_URL) — trim leading slashes, then if $avatar contains '..' or a
backslash '\' (e.g. via strpos($avatar, '..') !== false || strpos($avatar, '\\')
!== false) or otherwise fails a safe character check, treat it as empty and fall
back to '/avatars/blank.gif'; only append the validated, sanitized $avatar value
to XOOPS_UPLOAD_URL when it contains no traversal/backslash characters.

In `@htdocs/kernel/block.php`:
- Around line 419-432: Replace the current logging that appends raw
$e->getMessage() in both catch blocks (the ReflectionException catch that
references $showFunc and the Throwable catch that references $funcFile) with a
generic non-path-leaking trigger_error call; specifically, stop including
$e->getMessage(), and instead call trigger_error("PHP block function {$showFunc}
cannot be reflected" or "PHP block error in custom_blocks/".basename($funcFile),
E_USER_WARNING) as appropriate, and remove the exposure of throwable messages
from XoopsLogger::addExtra (leave detailed diagnostics out of the user-facing
warning).
- Around line 442-450: The current strpos($raw, '|') guard rejects any PHP block
containing '|' too broadly and runs before XOOPS_ALLOW_PHP_BLOCKS is considered;
update the check so it only treats strings that resemble malformed file-based
references (e.g. "filename.php|function_name") as errors. Replace the
strpos($raw, '|') branch with a targeted validation using a regex/pattern match
against $raw (for example require a .php filename, a single '|' and a valid
function identifier) and only then call
XoopsLogger::getInstance()->addExtra(...) and return ''. Do not change the order
of XOOPS_ALLOW_PHP_BLOCKS logic beyond ensuring this guard only triggers for
malformed filename.php|function_name candidates.

---

Duplicate comments:
In `@htdocs/custom_blocks/example_recent_members.php`:
- Around line 1-16: This file is missing the standard XOOPS repository header;
insert the required XOOPS copyright/source header block at the very top of
htdocs/custom_blocks/example_recent_members.php before the existing file-level
docblock so the file complies with "Every source file must begin with the XOOPS
copyright header block"; keep the existing example docblock and any references
(e.g., the suggested block callback b_custom_recent_members_show) unchanged and
only prepend the official header.

In `@tests/unit/htdocs/kernel/XoopsBlockPhpBlockTest.php`:
- Around line 42-50: The tests currently create files under XOOPS_ROOT_PATH (set
in tests/bootstrap.php) via setUp() and createTempBlockFile(), mutating the
checked-in htdocs/custom_blocks; change the tests to create an isolated per-test
temp directory (use sys_get_temp_dir()/tempnam or mkdir with a unique prefix)
and set $this->customBlocksDir to that path, and ensure tearDown() removes it;
update the loader/path resolution used by the code under test (make the
custom-block root overridable via a constructor arg, setter, or config/env
variable) so XoopsBlockPhpBlockTest::setUp() and createTempBlockFile() point the
loader at the per-test temp directory instead of
XOOPS_ROOT_PATH.'/custom_blocks', ensuring no writes to the repo and tests are
parallel-safe (also update the same pattern in the other affected block around
lines 78-88).
- Around line 1-3: The file XoopsBlockPhpBlockTest.php is missing the required
XOOPS copyright/header block at the top; add the standard XOOPS source header
comment block immediately before the existing declare(strict_types=1) line so
the header appears at the very top of the file (maintain exact header text used
across the repository), leaving the existing file content (including
declare(strict_types=1) and the XoopsBlockPhpBlockTest class) unchanged.
- Around line 7-20: The test class XoopsBlockPhpBlockTest uses native PHPUnit
attributes (PHPUnit\Framework\Attributes\Test, DataProvider, CoversClass) which
PHPUnit 9.6 may not discover unless the phpunit/phpunit-attributes polyfill is
shipped; either add phpunit/phpunit-attributes to composer (require-dev) so 9.6
lanes can discover attribute-based tests, or provide a 9.6-compatible fallback
in the test class (e.g., add test*-prefixed method aliases or `@test` docblock
annotations for each attribute-only test method in XoopsBlockPhpBlockTest) so
tests run across PHPUnit 9.6, 10.5 and 11.x.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 1c20779f-7f5c-474b-8513-dd6d6c494c18

📥 Commits

Reviewing files that changed from the base of the PR and between addf58d and 4c468fc.

📒 Files selected for processing (3)

• htdocs/custom_blocks/example_recent_members.php
• htdocs/kernel/block.php
• tests/unit/htdocs/kernel/XoopsBlockPhpBlockTest.php

[coderabbitai]

Actionable comments posted: 3

♻️ Duplicate comments (5)

htdocs/kernel/block.php (1)

383-475: ⚠️ Potential issue | 🟠 Major

Emit E_USER_WARNING for non-fatal block failures.

These branches only write to XoopsLogger, so missing files/functions and execution failures can disappear outside debug logging. The repo rule here is to raise trigger_error(..., E_USER_WARNING) and keep the logger call supplemental, using basename($funcFile) or a fixed legacy label in the warning text.

As per coding guidelines: "Use trigger_error() with E_USER_WARNING for non-fatal failures and use basename() in error messages to avoid exposing server paths."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@htdocs/kernel/block.php` around lines 383 - 475, The code currently logs
non-fatal PHP block failures only via XoopsLogger (e.g., when file missing, path
outside custom_blocks, function missing/reflection failing, try/catch Throwable
in file-based blocks, legacy eval errors, and the legacy malformed/file-format
warning/legacyWarningLogged path); update each of those branches to also call
trigger_error(...) with E_USER_WARNING using basename($funcFile) (or a fixed
label like 'legacy PHP block' for the legacy branches) and preserve the existing
XoopsLogger.addExtra/addDeprecated calls as supplemental logging; specifically
add trigger_error messages in the checks around file_exists($filePath), the
blocksRoot/realPath validation, the function_exists($showFunc) branch, the
ReflectionException catch, the outer Throwable catch for file-based execution,
the malformed '|' branch and the legacy eval Throwable catch, referencing
$funcFile and $showFunc where appropriate but wrapping paths with basename() to
avoid full path exposure.

htdocs/custom_blocks/example_recent_members.php (1)

1-16: ⚠️ Potential issue | 🟡 Minor

Add the standard XOOPS source header block.

This example file still starts with the local example docblock instead of the required XOOPS source header.

As per coding guidelines: "Every source file must begin with the XOOPS copyright header block."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@htdocs/custom_blocks/example_recent_members.php` around lines 1 - 16, Replace
the local example docblock at the top of example_recent_members.php with the
standard XOOPS source header block required by project guidelines; ensure the
header includes the XOOPS copyright and license lines and appears before any PHP
code or function definitions (e.g. before b_custom_recent_members_show if
present) so the file complies with "Every source file must begin with the XOOPS
copyright header block."

htdocs/custom_blocks/example_site_stats.php (1)

1-16: ⚠️ Potential issue | 🟡 Minor

Add the standard XOOPS source header block.

This example file still starts with its local docblock instead of the repository header required for checked-in PHP sources.

As per coding guidelines: "Every source file must begin with the XOOPS copyright header block."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@htdocs/custom_blocks/example_site_stats.php` around lines 1 - 16, The file
htdocs/custom_blocks/example_site_stats.php is missing the required XOOPS
repository header; replace the local docblock at the top with the standard XOOPS
copyright/header block used across the project (include license, copyright,
package, author tags), keeping the existing file description and any references
to the block entry point (example_site_stats.php and b_custom_site_stats_show)
below the header so the header appears as the very first lines of the file.

tests/unit/htdocs/kernel/XoopsBlockPhpBlockTest.php (2)

1-20: ⚠️ Potential issue | 🟠 Major

Add the XOOPS file header and verify PHPUnit 9.6 can discover this class.

This checked-in PHP file still lacks the standard XOOPS header, and every test here relies on PHPUnit attributes while the methods themselves do not start with test. If phpunit/phpunit-attributes is not installed, PHPUnit 9.6 will skip the class.

#!/bin/bash
set -euo pipefail

# Expectation:
# - This file should have the standard XOOPS source header.
# - If the repo supports PHPUnit 9.6 and keeps attribute-only test methods,
#   phpunit/phpunit-attributes should be present in Composer metadata.

python - <<'PY'
import json, pathlib

for name in ("composer.json", "composer.lock"):
    path = pathlib.Path(name)
    if not path.exists():
        continue
    print(f"== {name} ==")
    data = json.loads(path.read_text())
    if name == "composer.json":
        for section in ("require", "require-dev"):
            deps = data.get(section, {})
            for dep in ("phpunit/phpunit", "phpunit/phpunit-attributes"):
                if dep in deps:
                    print(f"{section}: {dep} {deps[dep]}")
    else:
        for section in ("packages", "packages-dev"):
            for pkg in data.get(section, []):
                if pkg["name"] in {"phpunit/phpunit", "phpunit/phpunit-attributes"}:
                    print(f"{section}: {pkg['name']} {pkg['version']}")
PY

python - <<'PY'
import pathlib, re
text = pathlib.Path("tests/unit/htdocs/kernel/XoopsBlockPhpBlockTest.php").read_text()
print("== public methods without test-prefix ==")
for name in re.findall(r'public function (\w+)\(', text):
    if not name.startswith("test"):
        print(name)
PY

As per coding guidelines: "Every source file must begin with the XOOPS copyright header block" and "Tests must work across PHPUnit 9.6, 10.5, and 11.x."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/unit/htdocs/kernel/XoopsBlockPhpBlockTest.php` around lines 1 - 20, Add
the standard XOOPS file header block to the top of the file and make the tests
discoverable by PHPUnit 9.6: either add conventional test-prefixed wrapper
methods inside the XoopsBlockPhpBlockTest class for every attribute-only test
(mirror each #[Test] method with a public function testXxx() that calls the
attribute method) or ensure the project composer metadata includes
phpunit/phpunit-attributes so attribute-only methods (those using #[Test],
#[DataProvider]) are supported; update XoopsBlockPhpBlockTest accordingly so
tests run under PHPUnit 9.6 without changing test logic.

---

248-300: ⚠️ Potential issue | 🟠 Major

These parsing tests don't distinguish the branches they claim to cover.

contentFormatIsRecognizedAsFileBased(), contentFormatIsNotRecognizedAsFileBased(), and pipeGuardIsInsideLegacyDisabledBranch() all assert only '', but executePhpBlock() returns '' for matched file-based content, malformed pipe content, and blocked legacy code. A regex regression or misplaced pipe guard would still pass unless these tests assert on a branch-specific effect.

Based on learnings: "New functionality must have corresponding unit tests."

Also applies to: 619-649

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/unit/htdocs/kernel/XoopsBlockPhpBlockTest.php` around lines 248 - 300,
The tests currently only assert that getContent returns '' so they can't tell
whether the file-based branch, malformed-pipe branch, or legacy-eval-blocked
branch was exercised; update contentFormatIsRecognizedAsFileBased,
contentFormatIsNotRecognizedAsFileBased and
pipeGuardIsInsideLegacyDisabledBranch to assert branch-specific side-effects
instead of only ''. Specifically, in contentFormatIsRecognizedAsFileBased(string
$content) spy/mock the internal file-inclusion path (or fileExists/include
handler) used by executePhpBlock/getContent (created via createPhpBlock) and
assert that the file-based handler was invoked and no legacy-eval warning/log
entry occurred; in contentFormatIsNotRecognizedAsFileBased assert that the
legacy-eval path (or the warning/log entry for eval being blocked) was taken and
the file-based handler was NOT invoked; and in
pipeGuardIsInsideLegacyDisabledBranch assert the guard branch around the '|'
parsing triggers the legacy-eval-blocked behavior (e.g., by asserting a specific
warning/log or that eval method was called/blocked). Use the unique symbols
createPhpBlock, getContent and executePhpBlock to locate the test scaffolding
and add spies/mocks against the file-inclusion function or logger used by those
methods so the assertions validate the correct branch rather than just the empty
result.

🤖 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/custom_blocks/example_welcome.php`:
- Around line 1-16: Replace the existing file-level docblock (the block starting
with "Example Custom Block: Welcome Message") with the required standard XOOPS
source header block at the top of example_welcome.php; locate the current header
by the phrase "Example Custom Block: Welcome Message" and update it to include
the canonical XOOPS copyright/header block used across the repo (author,
copyright, license, and required notice) so the file begins with the
repository-standard header.

In `@htdocs/custom_blocks/index.php`:
- Around line 1-11: Replace the custom file docblock in
htdocs/custom_blocks/index.php with the standard XOOPS copyright header block
required for all htdocs PHP sources; locate the top-of-file docblock and swap it
for the repository's canonical XOOPS header (including copyright, license and
project tags) so the file begins with the mandated XOOPS source header.

In `@tests/unit/htdocs/kernel/XoopsBlockPhpBlockTest.php`:
- Around line 332-367: Add a regression test that ensures callbacks defined
outside the included file are rejected: in XoopsBlockPhpBlockTest create a new
test (e.g., function externalFunctionOriginIsRejected) that defines a global
function (e.g., function external_cb() {}) before calling
$this->createPhpBlock('temp.php|external_cb'), then call $block->getContent('S',
'P') and assertSame('', $result). Use the existing createPhpBlock and getContent
helpers to exercise the code path in htdocs/kernel/block.php that verifies the
callback origin.

---

Duplicate comments:
In `@htdocs/custom_blocks/example_recent_members.php`:
- Around line 1-16: Replace the local example docblock at the top of
example_recent_members.php with the standard XOOPS source header block required
by project guidelines; ensure the header includes the XOOPS copyright and
license lines and appears before any PHP code or function definitions (e.g.
before b_custom_recent_members_show if present) so the file complies with "Every
source file must begin with the XOOPS copyright header block."

In `@htdocs/custom_blocks/example_site_stats.php`:
- Around line 1-16: The file htdocs/custom_blocks/example_site_stats.php is
missing the required XOOPS repository header; replace the local docblock at the
top with the standard XOOPS copyright/header block used across the project
(include license, copyright, package, author tags), keeping the existing file
description and any references to the block entry point (example_site_stats.php
and b_custom_site_stats_show) below the header so the header appears as the very
first lines of the file.

In `@htdocs/kernel/block.php`:
- Around line 383-475: The code currently logs non-fatal PHP block failures only
via XoopsLogger (e.g., when file missing, path outside custom_blocks, function
missing/reflection failing, try/catch Throwable in file-based blocks, legacy
eval errors, and the legacy malformed/file-format warning/legacyWarningLogged
path); update each of those branches to also call trigger_error(...) with
E_USER_WARNING using basename($funcFile) (or a fixed label like 'legacy PHP
block' for the legacy branches) and preserve the existing
XoopsLogger.addExtra/addDeprecated calls as supplemental logging; specifically
add trigger_error messages in the checks around file_exists($filePath), the
blocksRoot/realPath validation, the function_exists($showFunc) branch, the
ReflectionException catch, the outer Throwable catch for file-based execution,
the malformed '|' branch and the legacy eval Throwable catch, referencing
$funcFile and $showFunc where appropriate but wrapping paths with basename() to
avoid full path exposure.

In `@tests/unit/htdocs/kernel/XoopsBlockPhpBlockTest.php`:
- Around line 1-20: Add the standard XOOPS file header block to the top of the
file and make the tests discoverable by PHPUnit 9.6: either add conventional
test-prefixed wrapper methods inside the XoopsBlockPhpBlockTest class for every
attribute-only test (mirror each #[Test] method with a public function testXxx()
that calls the attribute method) or ensure the project composer metadata
includes phpunit/phpunit-attributes so attribute-only methods (those using
#[Test], #[DataProvider]) are supported; update XoopsBlockPhpBlockTest
accordingly so tests run under PHPUnit 9.6 without changing test logic.
- Around line 248-300: The tests currently only assert that getContent returns
'' so they can't tell whether the file-based branch, malformed-pipe branch, or
legacy-eval-blocked branch was exercised; update
contentFormatIsRecognizedAsFileBased, contentFormatIsNotRecognizedAsFileBased
and pipeGuardIsInsideLegacyDisabledBranch to assert branch-specific side-effects
instead of only ''. Specifically, in contentFormatIsRecognizedAsFileBased(string
$content) spy/mock the internal file-inclusion path (or fileExists/include
handler) used by executePhpBlock/getContent (created via createPhpBlock) and
assert that the file-based handler was invoked and no legacy-eval warning/log
entry occurred; in contentFormatIsNotRecognizedAsFileBased assert that the
legacy-eval path (or the warning/log entry for eval being blocked) was taken and
the file-based handler was NOT invoked; and in
pipeGuardIsInsideLegacyDisabledBranch assert the guard branch around the '|'
parsing triggers the legacy-eval-blocked behavior (e.g., by asserting a specific
warning/log or that eval method was called/blocked). Use the unique symbols
createPhpBlock, getContent and executePhpBlock to locate the test scaffolding
and add spies/mocks against the file-inclusion function or logger used by those
methods so the assertions validate the correct branch rather than just the empty
result.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 5ce7728c-662e-4409-9cea-1a5999dce0e2

📥 Commits

Reviewing files that changed from the base of the PR and between 4c468fc and a57d985.

📒 Files selected for processing (6)

• htdocs/custom_blocks/example_recent_members.php
• htdocs/custom_blocks/example_site_stats.php
• htdocs/custom_blocks/example_welcome.php
• htdocs/custom_blocks/index.php
• htdocs/kernel/block.php
• tests/unit/htdocs/kernel/XoopsBlockPhpBlockTest.php

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Add the standard XOOPS source header block.

This new checked-in PHP file starts with a custom docblock instead of the repository header required for PHP sources under htdocs/.

As per coding guidelines: "Every source file must begin with the XOOPS copyright header block."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@htdocs/custom_blocks/index.php` around lines 1 - 11, Replace the custom file
docblock in htdocs/custom_blocks/index.php with the standard XOOPS copyright
header block required for all htdocs PHP sources; locate the top-of-file
docblock and swap it for the repository's canonical XOOPS header (including
copyright, license and project tags) so the file begins with the mandated XOOPS
source header.

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.

[Copilot]

In this method there are a few addExtra() calls written without a space after the comma (e.g. addExtra('Block Warning',"...")). This is inconsistent with the surrounding codebase style and makes the calls harder to scan. Consider applying consistent spacing/quoting for these logger calls throughout executePhpBlock().

[Copilot]

The documentation’s htmlspecialchars() examples omit the explicit encoding argument. Elsewhere in this PR’s example block files you use 'UTF-8', which is safer and avoids relying on default_charset. Update the doc snippets here to include the encoding parameter so the docs match the recommended practice.

Suggested change

	$uname = htmlspecialchars($xoopsUser->getVar('uname'), ENT_QUOTES | ENT_HTML5);
	return '<p>Welcome back, <strong>' . $uname . '</strong>!</p>';
	}
	$sitename = htmlspecialchars($xoopsConfig['sitename'], ENT_QUOTES | ENT_HTML5);
	$uname = htmlspecialchars($xoopsUser->getVar('uname'), ENT_QUOTES | ENT_HTML5, 'UTF-8');
	return '<p>Welcome back, <strong>' . $uname . '</strong>!</p>';
	}
	$sitename = htmlspecialchars($xoopsConfig['sitename'], ENT_QUOTES | ENT_HTML5, 'UTF-8');

[Copilot]

In “Security Reminders”, the suggested escaping call htmlspecialchars($val, ENT_QUOTES | ENT_HTML5) also omits the encoding argument. For consistency with the shipped examples (and to avoid charset-dependent behavior), it should specify 'UTF-8' as the third parameter.

Suggested change

	- Always escape output with `htmlspecialchars($val, ENT_QUOTES | ENT_HTML5)`
	- Always escape output with `htmlspecialchars($val, ENT_QUOTES | ENT_HTML5, 'UTF-8')`

[Copilot]

The legacy PHP block fallback still executes arbitrary PHP from the content field via eval($raw), which enables remote code execution whenever an attacker can modify block content (e.g., SQL injection into the blocks table or abuse of any admin-level content-editing path) and XOOPS_ALLOW_PHP_BLOCKS is set to true. Even though this is gated by a config constant, the risk remains that a single database or admin compromise escalates to full server-side code execution. To harden this further, remove the eval() fallback entirely (or restrict it to a development-only build), and require all custom logic to use the file-based block mechanism under custom_blocks/ instead.

Suggested change

	$obLevel = ob_get_level();
	ob_start();
	try {
	echo eval($raw); // NOSONAR — legacy fallback, gated by XOOPS_ALLOW_PHP_BLOCKS
	$content = ob_get_clean();
	return str_replace('{X_SITEURL}', XOOPS_URL . '/', $content);
	} catch (\Throwable $e) {
	$logger = XoopsLogger::getInstance();
	$logger->addExtra('Block Warning', 'Legacy PHP block execution error');
	return '';
	} finally {
	// Clean up only the output buffers we opened
	while (ob_get_level() > $obLevel) {
	ob_end_clean();
	}
	}
	$logger = XoopsLogger::getInstance();
	$logger->addDeprecated(
	'Execution of legacy PHP block content via eval() has been disabled. '
	. 'Migrate this block to the file-based format (filename.php|function_name in custom_blocks/).'
	);
	return '';
	}
	/**
	* (HTML-) form for setting the options of the block
	*
	* @return string HTML for the form, FALSE if not defined for this block
	*/
	public function getOptions()

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.

[Copilot]

The docblock for logBlockWarning() says it logs “via XoopsLogger and PHP error log”, but the implementation only calls XoopsLogger::addExtra(). Either update the comment to match the behavior, or also emit to the PHP error log (e.g., error_log) if that’s intended.

[Copilot]

This test uses eval() to define a spoofed function name. Since the PR is explicitly moving away from eval and some security/static-analysis tooling flags eval(), consider defining the spoof function by writing a small temporary PHP file and including it instead (or otherwise avoid eval here).

[Copilot]

The executeLegacyBlock method still evaluates arbitrary PHP from the block content field via eval($raw), which is a direct remote code execution sink whenever XOOPS_ALLOW_PHP_BLOCKS is enabled and an attacker can modify block content (e.g., via SQL injection or a compromised admin account). Because the code being evaluated comes from the database, any write primitive to the blocks table can be escalated to full server-side code execution in production. To mitigate, remove the eval-based legacy path entirely (or gate it behind a non-production-only flag) and require all PHP blocks to use the new file-based callback mechanism instead of executing raw PHP strings.

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.

[Copilot]

executeFileBasedBlock() catches \Throwable but logs a very generic message (no exception type/message). This makes it hard to troubleshoot why a custom block failed, especially during migration. Consider including at least the exception class and a sanitized message (avoiding full paths) in the logger output when debug logging is enabled.

[Copilot]

setUp()/tearDown() perform filesystem operations (mkdir/unlink/rmdir) without checking return values. In CI, warnings from a failed mkdir/rmdir/unlink can fail the test run (failOnWarning=true). Consider asserting these calls succeed (or skipping the test if sys_get_temp_dir() is not writable) and making cleanup robust to partial failures.

[Copilot]

createTempBlockFile() uses file_put_contents() without checking for failure. If the write fails, PHP emits a warning (which will fail CI) and later assertions may become misleading. Consider asserting file_put_contents() returns a non-false value (or throwing a clear exception) so failures are explicit and warning-free.

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

[Copilot]

The legacy PHP block path in executeLegacyBlock() still uses eval($raw) on block content loaded from the database whenever XOOPS_ALLOW_PHP_BLOCKS is set to true, which provides a direct arbitrary code execution primitive. An attacker who can modify the content field (e.g., via SQL injection, database tampering, or a compromised/rogue admin) on a site that has enabled legacy PHP blocks can execute arbitrary PHP in the application context. To mitigate, remove or hard-disable this eval-based fallback in production (after migrating legacy blocks to the new file-based mechanism), or at minimum ensure it is only available in tightly controlled environments and avoid executing raw database content with eval() at all.

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.

[Copilot]

In this test, the return value of file_put_contents() is not checked. If writing the temp PHP block file fails, the test can still pass (because getContent() returns an empty string when the file is missing), which makes the test a false positive. Assert that the write succeeded (or skip/fail the test) before executing the block.

[Copilot]

file_put_contents() isn't checked here either. If the file write fails, the test may still pass as a "file not found" case, reducing the value of the assertion about a missing function definition. Please assert the write succeeded before calling getContent().

[Copilot]

This test writes a temp block file but doesn't verify the write succeeded. If file_put_contents() fails, the block execution will fall into the "file not found" branch and the test can still pass, making it unreliable. Add an assertion on the file write result.

[Copilot]

executeLegacyBlock() still calls eval($raw) on the PHP block content when XOOPS_ALLOW_PHP_BLOCKS is enabled, which means any attacker who can modify the block content (e.g., via SQL injection, compromised admin account, or malicious plugin) gains arbitrary server-side code execution. This keeps a high-impact RCE sink in the runtime even though a safer file-based mechanism exists and only a boolean flag gates its use. To mitigate, eliminate this eval-based path entirely or strictly confine XOOPS_ALLOW_PHP_BLOCKS to non-production/test environments and require all PHP blocks to use the vetted file-based callback format.

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.

[Copilot]

executeLegacyBlock() calls XoopsLogger::addDeprecated(), which builds a full debug_backtrace() for every request that contains at least one legacy PHP block. This can be a noticeable overhead on busy sites during migrations. Consider logging legacy-block detection via addExtra() (or gating addDeprecated() behind debug mode) to avoid collecting backtraces in normal operation.

[Copilot]

This example file doesn’t include the standard XOOPS source header block used across core PHP files (the “You may not change or alter…” header). Since these examples ship with core, consider aligning the file header with the project’s usual source-file header format for consistency/licensing clarity.

[Copilot]

This example file doesn’t include the standard XOOPS source header block used across core PHP files (the “You may not change or alter…” header). Since these examples ship with core, consider aligning the file header with the project’s usual source-file header format for consistency/licensing clarity.

[Copilot]

This example file doesn’t include the standard XOOPS source header block used across core PHP files (the “You may not change or alter…” header). Since these examples ship with core, consider aligning the file header with the project’s usual source-file header format for consistency/licensing clarity.

[sonarqubecloud]

Quality Gate passed

Issues
6 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.

[Copilot]

executeFileBasedBlock() starts output buffering before include_once. Any accidental output (or warnings/notices when display_errors is on) emitted at file scope during include will be appended to the block output. Consider moving ob_start() so it wraps only the $showFunc() invocation (keep include/validation outside the buffer), or explicitly discard/include output separately, to avoid unexpected/unsafe block output.

[Copilot]

The implementation enforces that the callback function must be defined in the referenced file (via ReflectionFunction::getFileName() equality check). The tutorial currently states only that the function must exist after including the file; please document this stricter requirement (and the implication that functions defined in included/required files will be rejected), so users don’t hit a confusing “not defined in custom_blocks/…” warning.

Suggested change

	- **function_name** — The function to call (must return an HTML string)
	The pipe `|` character separates the two parts.
	- **function_name** — The function to call (must return an HTML string), **defined
	directly in that file**.
	The pipe `|` character separates the two parts.
	> **Note:** XOOPS checks that the callback function is defined in the referenced
	> `custom_blocks/` file (using the function's source filename). If the function is
	> defined only in a file that is included or required from your block file, it will
	> be rejected and you will see a warning that it is "not defined in
	> custom_blocks/…".

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 0.00%. Comparing base (80c2008) to head (6b3f66a).
⚠️ Report is 74 commits behind head on master.

Additional details and impacted files

@@      Coverage Diff       @@
##   master   #1637   +/-   ##
==============================
==============================

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.