removed unused code

[ggoffy]

in module admin you can change module name and sorting of modules.
if you change the module name and click on "Submit" you will be redirected to confirm page.

on confirm page under "Action" there is always "No changes" as the information about oldstatus isn't provided anymore by moduladmin.php&op=confirm, status change is handled on list page of moduladmin with js

also there is never a value in column "Order" as the necessary information is also not provided by moduladmin.php&op=confirm anymore, as the weighting is handled by drag&drop

moduladmin.php&op=submit handles only changes in names
therefore the columns and the dead code can be removed

Summary by CodeRabbit

• Bug Fixes

  • Confirmation step now only applies module name changes; activation/deactivation and reordering are no longer processed during confirmation.

• UI/UX

  • Confirmation table simplified to show only module names and rename indicators; status and weight columns and related inputs were removed.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7eb5440 and 3ee60fa.

📒 Files selected for processing (1)

• htdocs/modules/system/templates/admin/system_modules_confirm.tpl

---

Walkthrough

Removed module activation/deactivation handling from the admin modules workflow: backend no longer propagates or applies newstatus, and the confirmation template was simplified to present only module rename information.

Changes

Cohort / File(s)	Summary
Backend Logic htdocs/modules/system/admin/modulesadmin/main.php	Removed newstatus propagation in confirm step and removed activation/deactivation toggle logic in submit step; submit now processes only name changes.
Confirmation Template htdocs/modules/system/templates/admin/system_modules_confirm.tpl	Reduced table from three columns to one (Module); removed per-row action/status/weight columns and hidden inputs for oldstatus/newstatus and oldweight/weight; retained module, oldname, and newname hidden fields and simplified logger markup.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Title check	❓ Inconclusive	The title 'removed unused code' is vague and generic, failing to convey what specific unused code or functionality was removed from the modules admin system.	Consider a more descriptive title such as 'Remove unused status and weight columns from module admin confirm page' to clearly indicate the scope and purpose of the changes.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ 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]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

htdocs/modules/system/templates/admin/system_modules_confirm.tpl (1)

27-27: ⚠️ Potential issue | 🟡 Minor

Stale colspan="3" — table is now single-column.

The footer <td> still spans 3 columns, but the 3-column layout (Module / Action / Order) no longer exists after this cleanup. It should be updated to colspan="1" (or just dropped, since it has no effect on a single-column table).

🛠️ Proposed fix

-                <td colspan="3">
+                <td>

🤖 Prompt for AI Agents

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

In `@htdocs/modules/system/templates/admin/system_modules_confirm.tpl` at line 27,
The footer cell in the admin modules confirmation template still uses
colspan="3" even though the table is now single-column; update the footer <td>
in system_modules_confirm.tpl to remove or change the colspan attribute to "1"
(or omit it entirely) so it no longer incorrectly spans three columns—look for
the <td colspan="3"> element in the template and replace it with a plain <td> or
<td colspan="1"> to match the single-column layout.

🤖 Prompt for all review comments with AI agents

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

Outside diff comments:
In `@htdocs/modules/system/templates/admin/system_modules_confirm.tpl`:
- Line 27: The footer cell in the admin modules confirmation template still uses
colspan="3" even though the table is now single-column; update the footer <td>
in system_modules_confirm.tpl to remove or change the colspan attribute to "1"
(or omit it entirely) so it no longer incorrectly spans three columns—look for
the <td colspan="3"> element in the template and replace it with a plain <td> or
<td colspan="1"> to match the single-column layout.

---

ℹ️ Review info

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 05cdfdd and 7eb5440.

📒 Files selected for processing (2)

• htdocs/modules/system/admin/modulesadmin/main.php
• htdocs/modules/system/templates/admin/system_modules_confirm.tpl

💤 Files with no reviewable changes (1)

• htdocs/modules/system/admin/modulesadmin/main.php

[coderabbitai]

Note

Unit test generation is a public access feature. Expect some limitations and changes as we gather feedback and continue to improve it.

---

Generating unit tests... This may take up to 20 minutes.

[Copilot]

Pull request overview

This PR removes unused code from the module administration confirmation workflow. The confirmation page previously displayed three columns (Module, Action, Order) but only the Module name changes were actually processed. Status changes (activate/deactivate) and ordering are now handled through JavaScript/AJAX on the main list page via drag-and-drop and toggle buttons.

Changes:

• Removed Action and Order columns from the confirmation table
• Removed backend processing code for status and weight changes in the submit operation
• Fixed indentation in several template sections

Reviewed changes

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

File	Description
htdocs/modules/system/templates/admin/system_modules_confirm.tpl	Removed Action and Order column headers and cells, removed hidden form fields for status/weight, improved indentation
htdocs/modules/system/admin/modulesadmin/main.php	Removed newstatus assignment in confirm case and all status change logic in submit case

[coderabbitai]

Copyable Unit Test Edits

Copyable Edits

tests/unit/htdocs/modules/system/admin/modulesadmin/MainControllerTest.php

This is a new file.

<?php
/**
 * Unit tests for modules/system/admin/modulesadmin/main.php
 *
 * Tests the module administration controller including security checks,
 * operation routing, parameter sanitization, and template data preparation.
 *
 * `@copyright`    2000-2026 XOOPS Project (https://xoops.org)
 * `@license`      GNU GPL 2.0 or later (https://www.gnu.org/licenses/gpl-2.0.html)
 * `@package`      Tests\Unit\System\ModulesAdmin
 */

declare(strict_types=1);

namespace Tests\Unit\System\ModulesAdmin;

use PHPUnit\Framework\TestCase;

/**
 * Tests for main.php module administration controller
 *
 * These tests verify the behavior of the module administration interface
 * including operation routing, security token validation, parameter handling,
 * and data preparation for templates.
 */
class MainControllerTest extends TestCase
{
    /**
     * `@var` string The source code of main.php
     */
    private string $sourceCode;

    /**
     * `@var` string Path to the main.php file under test
     */
    private string $filePath;

    protected function setUp(): void
    {
        $possiblePaths = [
            dirname(__DIR__, 6) . '/modules/system/admin/modulesadmin/main.php',
            dirname(__DIR__, 5) . '/htdocs/modules/system/admin/modulesadmin/main.php',
            dirname(__DIR__, 6) . '/htdocs/modules/system/admin/modulesadmin/main.php',
            dirname(__DIR__, 4) . '/modules/system/admin/modulesadmin/main.php',
        ];

        $this->filePath = '';
        foreach ($possiblePaths as $path) {
            if (file_exists($path)) {
                $this->filePath = $path;
                break;
            }
        }

        if ($this->filePath === '') {
            $this->markTestSkipped('main.php not found in expected locations');
        }

        $this->sourceCode = file_get_contents($this->filePath);
        $this->assertNotEmpty($this->sourceCode, 'Source file should not be empty');
    }

    // =========================================================================
    // Security and Access Control Tests
    // =========================================================================

    /**
     * Verify that the file checks user permissions before proceeding.
     */
    public function testEnforcesAdminPermissionCheck(): void
    {
        // Should check if user is object, module is object, and user is admin
        $this->assertStringContainsString(
            'if (!is_object($xoopsUser) || !is_object($xoopsModule) || !$xoopsUser->isAdmin($xoopsModule->mid()))',
            $this->sourceCode,
            'Should enforce admin permission check at the start'
        );

        // Should exit with NOPERM if not authorized
        $this->assertStringContainsString(
            'exit(_NOPERM);',
            $this->sourceCode,
            'Should exit with no permission message if unauthorized'
        );
    }

    /**
     * Verify that security token validation is performed for state-changing operations.
     */
    public function testValidatesSecurityTokenForStateChangingOps(): void
    {
        // Should check security token for confirm, submit, install_ok, update_ok, uninstall_ok
        $this->assertStringContainsString(
            "if (in_array(\$op, ['confirm', 'submit', 'install_ok', 'update_ok', 'uninstall_ok']))",
            $this->sourceCode,
            'Should identify state-changing operations'
        );

        $this->assertStringContainsString(
            '$GLOBALS[\'xoopsSecurity\']->check()',
            $this->sourceCode,
            'Should validate security token for state-changing operations'
        );

        // Should reset op to 'list' if token check fails
        $this->assertStringContainsString(
            "\$op = 'list';",
            $this->sourceCode,
            'Should reset operation to list if security check fails'
        );
    }

    /**
     * Verify that the security token is passed to templates for form submission.
     */
    public function testPassesSecurityTokenToTemplate(): void
    {
        // In the confirm operation, should assign security token to template
        $confirmSection = $this->extractOperationSection('confirm');
        $this->assertNotEmpty($confirmSection, 'Confirm operation should exist');

        $this->assertStringContainsString(
            "\$GLOBALS['xoopsSecurity']->getTokenHTML()",
            $confirmSection,
            'Should generate security token HTML for form'
        );

        $this->assertStringContainsString(
            "\$xoopsTpl->assign('input_security',",
            $confirmSection,
            'Should assign security token to template'
        );
    }

    // =========================================================================
    // Operation Routing Tests
    // =========================================================================

    /**
     * Verify that all expected operations are handled in the switch statement.
     */
    public function testHandlesAllExpectedOperations(): void
    {
        $expectedOps = [
            'list',
            'installlist',
            'order',
            'confirm',
            'display',
            'display_in_menu',
            'submit',
            'install',
            'install_ok',
            'uninstall',
            'uninstall_ok',
            'update',
            'update_ok',
        ];

        foreach ($expectedOps as $op) {
            $this->assertMatchesRegularExpression(
                "/case\s+['\"]" . preg_quote($op, '/') . "['\"]:/",
                $this->sourceCode,
                "Should handle operation: {$op}"
            );
        }
    }

    /**
     * Verify that operation parameter is retrieved using Request helper.
     */
    public function testRetrievesOperationParameterSafely(): void
    {
        $this->assertStringContainsString(
            "use Xmf\Request;",
            $this->sourceCode,
            'Should use Xmf Request helper'
        );

        $this->assertMatchesRegularExpression(
            '/\$op\s*=\s*Request::getString\s*\(\s*[\'"]op[\'"]\s*,\s*[\'"]list[\'"]\s*\)/',
            $this->sourceCode,
            'Should retrieve operation parameter with default value'
        );
    }

    /**
     * Verify that the default operation is 'list'.
     */
    public function testDefaultOperationIsList(): void
    {
        $this->assertMatchesRegularExpression(
            '/\$op\s*=\s*Request::getString\s*\([^)]*[\'"]list[\'"]/',
            $this->sourceCode,
            'Default operation should be list'
        );
    }

    // =========================================================================
    // Input Sanitization Tests
    // =========================================================================

    /**
     * Verify that POST data is sanitized using XoopsFilterInput.
     */
    public function testSanitizesPostDataUsingFilterInput(): void
    {
        $this->assertStringContainsString(
            "XoopsLoad::load('XoopsFilterInput');",
            $this->sourceCode,
            'Should load XoopsFilterInput class'
        );

        // In confirm operation, should clean module names
        $confirmSection = $this->extractOperationSection('confirm');
        $this->assertStringContainsString(
            "XoopsFilterInput::clean",
            $confirmSection,
            'Should use XoopsFilterInput to clean input'
        );

        $this->assertStringContainsString(
            "'STRING'",
            $confirmSection,
            'Should filter input as STRING type'
        );
    }

    /**
     * Verify that MyTextSanitizer is used for HTML output sanitization.
     */
    public function testUsesTextSanitizerForOutput(): void
    {
        $this->assertStringContainsString(
            '$myts = \MyTextSanitizer::getInstance();',
            $this->sourceCode,
            'Should initialize MyTextSanitizer'
        );

        // Should use htmlSpecialChars for output
        $this->assertStringContainsString(
            '$myts->htmlSpecialChars',
            $this->sourceCode,
            'Should use htmlSpecialChars for output sanitization'
        );
    }

    /**
     * Verify that module names are properly escaped in array building.
     */
    public function testEscapesModuleNamesInListOperation(): void
    {
        $listSection = $this->extractOperationSection('list');
        $this->assertNotEmpty($listSection, 'List operation should exist');

        $this->assertStringContainsString(
            'htmlspecialchars((string) $module->getVar(\'name\'), ENT_QUOTES | ENT_HTML5)',
            $listSection,
            'Should escape module names with ENT_QUOTES | ENT_HTML5 flags'
        );
    }

    /**
     * Verify that module parameters are cast to integers where expected.
     */
    public function testCastsModuleIdsToIntegers(): void
    {
        // In confirm operation, module IDs should be cast to int
        $confirmSection = $this->extractOperationSection('confirm');

        $this->assertStringContainsString(
            '(int) $mid',
            $confirmSection,
            'Should cast module ID to integer'
        );
    }

    // =========================================================================
    // Confirm Operation Tests
    // =========================================================================

    /**
     * Verify that confirm operation validates writable cache directory.
     */
    public function testConfirmOperationValidatesCacheWritability(): void
    {
        $confirmSection = $this->extractOperationSection('confirm');

        $this->assertStringContainsString(
            'if (!is_writable(XOOPS_CACHE_PATH . \'/\'))',
            $confirmSection,
            'Should check if cache path is writable'
        );

        $this->assertStringContainsString(
            '$errorMessage[] = sprintf(_MUSTWABLE,',
            $confirmSection,
            'Should add error message if cache not writable'
        );
    }

    /**
     * Verify that confirm operation exits if there are errors.
     */
    public function testConfirmOperationExitsOnErrors(): void
    {
        $confirmSection = $this->extractOperationSection('confirm');

        $this->assertStringContainsString(
            'if (count($errorMessage) > 0)',
            $confirmSection,
            'Should check for error messages'
        );

        $this->assertStringContainsString(
            'xoops_error($errorMessage);',
            $confirmSection,
            'Should display errors using xoops_error()'
        );

        $this->assertStringContainsString(
            'exit();',
            $confirmSection,
            'Should exit after displaying errors'
        );
    }

    /**
     * Verify that confirm operation processes module data correctly.
     */
    public function testConfirmOperationProcessesModuleData(): void
    {
        $confirmSection = $this->extractOperationSection('confirm');

        // Should iterate over module array
        $this->assertStringContainsString(
            'foreach ($module as $mid)',
            $confirmSection,
            'Should iterate over module array'
        );

        // Should trim and clean new names
        $this->assertStringContainsString(
            'trim((string) XoopsFilterInput::clean($newname[$mid], \'STRING\'))',
            $confirmSection,
            'Should trim and clean new module names'
        );

        // Should build modifs_mods array with mid, oldname, newname
        $this->assertStringContainsString(
            "\$modifs_mods[\$i]['mid']",
            $confirmSection,
            'Should set module ID in array'
        );

        $this->assertStringContainsString(
            "\$modifs_mods[\$i]['oldname']",
            $confirmSection,
            'Should set old name in array'
        );

        $this->assertStringContainsString(
            "\$modifs_mods[\$i]['newname']",
            $confirmSection,
            'Should set new name in array'
        );
    }

    /**
     * Verify that confirm operation assigns data to template.
     */
    public function testConfirmOperationAssignsDataToTemplate(): void
    {
        $confirmSection = $this->extractOperationSection('confirm');

        $this->assertStringContainsString(
            "\$xoopsTpl->assign('modifs_mods', \$modifs_mods);",
            $confirmSection,
            'Should assign modified modules to template'
        );
    }

    /**
     * Verify that confirm operation sets correct template.
     */
    public function testConfirmOperationSetsCorrectTemplate(): void
    {
        $confirmSection = $this->extractOperationSection('confirm');

        $this->assertStringContainsString(
            "\$GLOBALS['xoopsOption']['template_main'] = 'system_modules_confirm.tpl';",
            $confirmSection,
            'Should set template to system_modules_confirm.tpl'
        );
    }

    // =========================================================================
    // Submit Operation Tests
    // =========================================================================

    /**
     * Verify that submit operation compares old and new names.
     */
    public function testSubmitOperationComparesNames(): void
    {
        $submitSection = $this->extractOperationSection('submit');
        $this->assertNotEmpty($submitSection, 'Submit operation should exist');

        $this->assertStringContainsString(
            'if ($oldname[$mid] != $newname[$mid])',
            $submitSection,
            'Should compare old and new names before changing'
        );
    }

    /**
     * Verify that submit operation calls module change function.
     */
    public function testSubmitOperationCallsModuleChange(): void
    {
        $submitSection = $this->extractOperationSection('submit');

        $this->assertStringContainsString(
            'xoops_module_change(',
            $submitSection,
            'Should call xoops_module_change function'
        );
    }

    /**
     * Verify that submit operation flushes cpanel cache.
     */
    public function testSubmitOperationFlushesCache(): void
    {
        $submitSection = $this->extractOperationSection('submit');

        $this->assertStringContainsString(
            "xoops_load('cpanel', 'system');",
            $submitSection,
            'Should load cpanel class'
        );

        $this->assertStringContainsString(
            'XoopsSystemCpanel::flush();',
            $submitSection,
            'Should flush cpanel cache'
        );
    }

    /**
     * Verify that submit operation updates active modules cache.
     */
    public function testSubmitOperationUpdatesActiveModules(): void
    {
        $submitSection = $this->extractOperationSection('submit');

        $this->assertStringContainsString(
            'xoops_setActiveModules();',
            $submitSection,
            'Should update active modules cache'
        );
    }

    /**
     * Verify that submit operation assigns results to template.
     */
    public function testSubmitOperationAssignsResults(): void
    {
        $submitSection = $this->extractOperationSection('submit');

        $this->assertStringContainsString(
            "\$xoopsTpl->assign('result', \$ret);",
            $submitSection,
            'Should assign results to template'
        );
    }

    // =========================================================================
    // List Operation Tests
    // =========================================================================

    /**
     * Verify that list operation adds stylesheets and scripts.
     */
    public function testListOperationAddsAssets(): void
    {
        $listSection = $this->extractOperationSection('list');
        $this->assertNotEmpty($listSection, 'List operation should exist');

        $this->assertStringContainsString(
            '$xoTheme->addStylesheet(',
            $listSection,
            'Should add stylesheets'
        );

        $this->assertStringContainsString(
            '$xoTheme->addScript(',
            $listSection,
            'Should add JavaScript files'
        );
    }

    /**
     * Verify that list operation retrieves installed modules.
     */
    public function testListOperationRetrievesInstalledModules(): void
    {
        $listSection = $this->extractOperationSection('list');

        $this->assertStringContainsString(
            "\$module_handler = xoops_getHandler('module');",
            $listSection,
            'Should get module handler'
        );

        $this->assertStringContainsString(
            '$installed_mods = $module_handler->getObjects($criteria);',
            $listSection,
            'Should retrieve installed modules'
        );
    }

    /**
     * Verify that list operation checks for version updates.
     */
    public function testListOperationChecksVersionUpdates(): void
    {
        $listSection = $this->extractOperationSection('list');

        $this->assertStringContainsString(
            '$module->versionCompare(',
            $listSection,
            'Should compare module versions'
        );

        $this->assertStringContainsString(
            "\$listed_mods[\$i]['warning_update']",
            $listSection,
            'Should set warning_update flag'
        );
    }

    /**
     * Verify that list operation handles legacy version format.
     */
    public function testListOperationHandlesLegacyVersionFormat(): void
    {
        $listSection = $this->extractOperationSection('list');

        $this->assertStringContainsString(
            "strpos(\$listed_mods[\$i]['version'], '.') === false",
            $listSection,
            'Should check for version without dots (legacy format)'
        );

        // Comment should explain this is for xoops 2.5.11 compatibility
        $this->assertStringContainsString(
            '2.5.11',
            $listSection,
            'Should have comment about XOOPS 2.5.11 version handling'
        );
    }

    /**
     * Verify that list operation counts modules to install.
     */
    public function testListOperationCountsModulesToInstall(): void
    {
        $listSection = $this->extractOperationSection('list');

        $this->assertStringContainsString(
            'XoopsLists::getModulesList()',
            $listSection,
            'Should get list of available modules'
        );

        $this->assertStringContainsString(
            "\$xoopsTpl->assign('toinstall_nb', \$i);",
            $listSection,
            'Should assign count of modules to install'
        );
    }

    // =========================================================================
    // Install Operation Tests
    // =========================================================================

    /**
     * Verify that install operations retrieve module information.
     */
    public function testInstallOperationsRetrieveModuleInfo(): void
    {
        $installSection = $this->extractOperationSection('install');
        $this->assertNotEmpty($installSection, 'Install operation should exist');

        $this->assertStringContainsString(
            '$mod->loadInfoAsVar($module);',
            $installSection,
            'Should load module information'
        );
    }

    /**
     * Verify that install operations display confirmation message.
     */
    public function testInstallOperationsDisplayConfirmation(): void
    {
        $installSection = $this->extractOperationSection('install');

        $this->assertStringContainsString(
            'xoops_confirm(',
            $installSection,
            'Should call xoops_confirm for user confirmation'
        );

        $this->assertStringContainsString(
            "'op' => 'install_ok'",
            $installSection,
            'Should set next operation to install_ok'
        );
    }

    /**
     * Verify that install_ok operation calls install function.
     */
    public function testInstallOkOperationCallsInstallFunction(): void
    {
        $installOkSection = $this->extractOperationSection('install_ok');
        $this->assertNotEmpty($installOkSection, 'Install_ok operation should exist');

        $this->assertStringContainsString(
            'xoops_module_install(',
            $installOkSection,
            'Should call xoops_module_install function'
        );
    }

    /**
     * Verify that install_ok updates cache.
     */
    public function testInstallOkOperationUpdatesCache(): void
    {
        $installOkSection = $this->extractOperationSection('install_ok');

        $this->assertStringContainsString(
            'xoops_setActiveModules();',
            $installOkSection,
            'Should update active modules cache'
        );

        $this->assertStringContainsString(
            'xoops_module_delayed_clean_cache();',
            $installOkSection,
            'Should clean delayed cache'
        );
    }

    // =========================================================================
    // Update Operation Tests
    // =========================================================================

    /**
     * Verify that update operation shows confirmation.
     */
    public function testUpdateOperationShowsConfirmation(): void
    {
        $updateSection = $this->extractOperationSection('update');
        $this->assertNotEmpty($updateSection, 'Update operation should exist');

        $this->assertStringContainsString(
            'xoops_confirm(',
            $updateSection,
            'Should show confirmation dialog'
        );

        $this->assertStringContainsString(
            "'op' => 'update_ok'",
            $updateSection,
            'Should set next operation to update_ok'
        );
    }

    /**
     * Verify that update_ok operation calls update function.
     */
    public function testUpdateOkOperationCallsUpdateFunction(): void
    {
        $updateOkSection = $this->extractOperationSection('update_ok');
        $this->assertNotEmpty($updateOkSection, 'Update_ok operation should exist');

        $this->assertStringContainsString(
            'xoops_module_update(',
            $updateOkSection,
            'Should call xoops_module_update function'
        );
    }

    // =========================================================================
    // Uninstall Operation Tests
    // =========================================================================

    /**
     * Verify that uninstall operation shows confirmation.
     */
    public function testUninstallOperationShowsConfirmation(): void
    {
        $uninstallSection = $this->extractOperationSection('uninstall');
        $this->assertNotEmpty($uninstallSection, 'Uninstall operation should exist');

        $this->assertStringContainsString(
            'xoops_confirm(',
            $uninstallSection,
            'Should show confirmation dialog'
        );

        $this->assertStringContainsString(
            "_AM_SYSTEM_MODULES_RUSUREUNINS",
            $uninstallSection,
            'Should use uninstall confirmation message'
        );
    }

    /**
     * Verify that uninstall_ok operation calls uninstall function.
     */
    public function testUninstallOkOperationCallsUninstallFunction(): void
    {
        $uninstallOkSection = $this->extractOperationSection('uninstall_ok');
        $this->assertNotEmpty($uninstallOkSection, 'Uninstall_ok operation should exist');

        $this->assertStringContainsString(
            'xoops_module_uninstall(',
            $uninstallOkSection,
            'Should call xoops_module_uninstall function'
        );
    }

    // =========================================================================
    // Display Operations Tests
    // =========================================================================

    /**
     * Verify that display operation toggles module active state.
     */
    public function testDisplayOperationTogglesActiveState(): void
    {
        $displaySection = $this->extractOperationSection('display');
        $this->assertNotEmpty($displaySection, 'Display operation should exist');

        $this->assertStringContainsString(
            '$old = $module->getVar(\'isactive\');',
            $displaySection,
            'Should get current isactive state'
        );

        $this->assertStringContainsString(
            '$module->setVar(\'isactive\', !$old);',
            $displaySection,
            'Should toggle isactive state'
        );

        $this->assertStringContainsString(
            '$blocks[$i]->setVar(\'isactive\', !$old);',
            $displaySection,
            'Should toggle block active state'
        );
    }

    /**
     * Verify that display_in_menu operation toggles weight.
     */
    public function testDisplayInMenuOperationTogglesWeight(): void
    {
        $displayInMenuSection = $this->extractOperationSection('display_in_menu');
        $this->assertNotEmpty($displayInMenuSection, 'Display_in_menu operation should exist');

        $this->assertStringContainsString(
            '$old = $module->getVar(\'weight\');',
            $displayInMenuSection,
            'Should get current weight'
        );

        $this->assertStringContainsString(
            '$module->setVar(\'weight\', !$old);',
            $displayInMenuSection,
            'Should toggle weight (0 or non-zero)'
        );
    }

    // =========================================================================
    // Order Operation Tests
    // =========================================================================

    /**
     * Verify that order operation processes module ordering.
     */
    public function testOrderOperationProcessesModuleOrder(): void
    {
        $orderSection = $this->extractOperationSection('order');
        $this->assertNotEmpty($orderSection, 'Order operation should exist');

        $this->assertStringContainsString(
            "if (isset(\$_POST['mod']))",
            $orderSection,
            'Should check for mod POST data'
        );

        $this->assertStringContainsString(
            "foreach (\$_POST['mod'] as \$order)",
            $orderSection,
            'Should iterate over module order array'
        );

        // Should only change order for visible modules (weight != 0)
        $this->assertStringContainsString(
            "if (\$module->getVar('weight') != 0)",
            $orderSection,
            'Should only reorder visible modules'
        );
    }

    /**
     * Verify that order operation exits after processing.
     */
    public function testOrderOperationExitsAfterProcessing(): void
    {
        $orderSection = $this->extractOperationSection('order');

        $this->assertStringContainsString(
            'exit;',
            $orderSection,
            'Should exit after processing order (AJAX endpoint)'
        );
    }

    // =========================================================================
    // Installlist Operation Tests
    // =========================================================================

    /**
     * Verify that installlist operation shows available modules.
     */
    public function testInstalllistOperationShowsAvailableModules(): void
    {
        $installlistSection = $this->extractOperationSection('installlist');
        $this->assertNotEmpty($installlistSection, 'Installlist operation should exist');

        $this->assertStringContainsString(
            'XoopsLists::getModulesList()',
            $installlistSection,
            'Should get list of available modules'
        );

        $this->assertStringContainsString(
            '$module->loadInfo($file);',
            $installlistSection,
            'Should load module information'
        );

        $this->assertStringContainsString(
            "\$xoopsTpl->assign('toinstall_mods', \$toinstall_mods);",
            $installlistSection,
            'Should assign modules to install to template'
        );
    }

    /**
     * Verify that installlist operation escapes module names.
     */
    public function testInstalllistOperationEscapesModuleNames(): void
    {
        $installlistSection = $this->extractOperationSection('installlist');

        $this->assertStringContainsString(
            'htmlspecialchars($module->getInfo(\'name\'), ENT_QUOTES | ENT_HTML5)',
            $installlistSection,
            'Should escape module names in installlist'
        );
    }

    // =========================================================================
    // General Structure Tests
    // =========================================================================

    /**
     * Verify the file has valid PHP syntax.
     */
    public function testFileHasValidPhpSyntax(): void
    {
        $output = [];
        $returnCode = 0;
        exec('php -l ' . escapeshellarg($this->filePath) . ' 2>&1', $output, $returnCode);

        $this->assertEquals(
            0,
            $returnCode,
            'PHP syntax check failed: ' . implode("\n", $output)
        );
    }

    /**
     * Verify that file includes required dependencies.
     */
    public function testIncludesRequiredDependencies(): void
    {
        $this->assertStringContainsString(
            "include_once XOOPS_ROOT_PATH . '/class/xoopsblock.php';",
            $this->sourceCode,
            'Should include xoopsblock.php'
        );

        $this->assertStringContainsString(
            "include_once XOOPS_ROOT_PATH . '/modules/system/admin/modulesadmin/modulesadmin.php';",
            $this->sourceCode,
            'Should include modulesadmin.php functions'
        );
    }

    /**
     * Verify that breadcrumb is properly configured.
     */
    public function testConfiguresBreadcrumb(): void
    {
        $this->assertStringContainsString(
            '$xoBreadCrumb->addLink(',
            $this->sourceCode,
            'Should configure breadcrumb navigation'
        );

        $this->assertStringContainsString(
            '$xoBreadCrumb->render();',
            $this->sourceCode,
            'Should render breadcrumb'
        );
    }

    /**
     * Verify that xoops_cp_header() is called.
     */
    public function testCallsControlPanelHeader(): void
    {
        $this->assertStringContainsString(
            'xoops_cp_header();',
            $this->sourceCode,
            'Should call xoops_cp_header()'
        );
    }

    /**
     * Verify that xoops_cp_footer() is called for operations that display output.
     */
    public function testCallsControlPanelFooter(): void
    {
        $this->assertStringContainsString(
            'xoops_cp_footer();',
            $this->sourceCode,
            'Should call xoops_cp_footer()'
        );
    }

    /**
     * Verify that POST data is handled through a loop.
     */
    public function testHandlesPostDataThroughLoop(): void
    {
        $this->assertStringContainsString(
            'if (isset($_POST))',
            $this->sourceCode,
            'Should check if POST data exists'
        );

        $this->assertStringContainsString(
            'foreach ($_POST as $k => $v)',
            $this->sourceCode,
            'Should iterate over POST data'
        );

        // Note: The pattern ${$k} = $v is a legacy approach but we test that it exists
        $this->assertStringContainsString(
            '${$k} = $v;',
            $this->sourceCode,
            'Should extract POST variables (legacy pattern)'
        );
    }

    /**
     * Verify that module IDs are retrieved safely from Request.
     */
    public function testRetrievesModuleIdsSafely(): void
    {
        $this->assertStringContainsString(
            'Request::getInt(\'mid\', 0)',
            $this->sourceCode,
            'Should retrieve module ID as integer with default value'
        );
    }

    /**
     * Verify that module names are retrieved safely from Request.
     */
    public function testRetrievesModuleNamesSafely(): void
    {
        $this->assertStringContainsString(
            "Request::getString('module', '')",
            $this->sourceCode,
            'Should retrieve module name as string with default value'
        );
    }

    // =========================================================================
    // Edge Case Tests
    // =========================================================================

    /**
     * Verify that empty module arrays are handled.
     */
    public function testHandlesEmptyModuleArrays(): void
    {
        // In confirm operation, should handle empty module array
        $confirmSection = $this->extractOperationSection('confirm');

        $this->assertStringContainsString(
            "empty(\$_POST['module']) ? [] : \$_POST['module']",
            $confirmSection,
            'Should provide empty array default for missing module POST data'
        );
    }

    /**
     * Verify that module existence is checked before operations.
     */
    public function testChecksModuleExistence(): void
    {
        // Various operations should check if module > 0
        $this->assertStringContainsString(
            'if ($module_id > 0)',
            $this->sourceCode,
            'Should check if module ID is valid before operations'
        );
    }

    /**
     * Verify that file existence is checked before loading module info.
     */
    public function testChecksFileExistence(): void
    {
        $this->assertStringContainsString(
            "file_exists(XOOPS_ROOT_PATH . '/modules/' . \$file . '/xoops_version.php')",
            $this->sourceCode,
            'Should check if xoops_version.php exists before loading module'
        );
    }

    /**
     * Verify that files are properly trimmed.
     */
    public function testTrimsFileNames(): void
    {
        $this->assertStringContainsString(
            'trim((string) $file)',
            $this->sourceCode,
            'Should trim file names'
        );
    }

    /**
     * Verify that clearstatcache is called when checking files.
     */
    public function testCallsClearStatCache(): void
    {
        $this->assertStringContainsString(
            'clearstatcache();',
            $this->sourceCode,
            'Should call clearstatcache() when checking multiple files'
        );
    }

    // =========================================================================
    // Regression Tests
    // =========================================================================

    /**
     * Verify that the variable shadowing bug is present (module variable reused).
     * This is intentional in the legacy code but we document it.
     */
    public function testDocumentsModuleVariableShadowing(): void
    {
        // Line 84 retrieves Request::getArray('module') but then line 85 reuses $module
        // This is potentially confusing but appears intentional for the foreach loop
        $listSection = $this->extractOperationSection('list');

        $this->assertStringContainsString(
            "Request::getArray('module', [])",
            $listSection,
            'Should retrieve module array from request'
        );

        $this->assertStringContainsString(
            'foreach ($installed_mods as $module)',
            $listSection,
            'Should iterate installed modules using $module variable'
        );
    }

    /**
     * Verify proper increment pattern is used.
     */
    public function testUsesProperIncrementPattern(): void
    {
        $this->assertStringContainsString(
            '++$i;',
            $this->sourceCode,
            'Should use pre-increment pattern'
        );
    }

    // =========================================================================
    // Helper Methods
    // =========================================================================

    /**
     * Extract a specific case section from the switch statement.
     *
     * `@param` string $operation The operation name to extract
     * `@return` string The operation section code
     */
    private function extractOperationSection(string $operation): string
    {
        // Find case 'operation':
        $pattern = "/case\s+['\"]" . preg_quote($operation, '/') . "['\"]:\s*/";
        if (!preg_match($pattern, $this->sourceCode, $matches, PREG_OFFSET_CAPTURE)) {
            return '';
        }

        $startPos = $matches[0][1] + strlen($matches[0][0]);

        // Find the next break or case
        $nextCase = strpos($this->sourceCode, "\n    case ", $startPos);
        $nextBreak = strpos($this->sourceCode, "break;", $startPos);

        $endPos = false;
        if ($nextCase !== false && $nextBreak !== false) {
            $endPos = min($nextCase, $nextBreak + 6);
        } elseif ($nextCase !== false) {
            $endPos = $nextCase;
        } elseif ($nextBreak !== false) {
            $endPos = $nextBreak + 6;
        } else {
            // End of switch
            $endPos = strpos($this->sourceCode, '}', $startPos);
        }

        if ($endPos === false) {
            return '';
        }

        return substr($this->sourceCode, $startPos, $endPos - $startPos);
    }
}

tests/unit/htdocs/modules/system/admin/modulesadmin/README_TESTS.md

This is a new file.

# Module Administration Tests

This directory contains comprehensive unit tests for the XOOPS module administration interface.

## Test Files

### MainControllerTest.php (54 test methods)

Tests for `htdocs/modules/system/admin/modulesadmin/main.php`

**Coverage Areas:**

1. **Security and Access Control (3 tests)**
   - Admin permission enforcement
   - Security token validation for state-changing operations
   - Security token inclusion in templates

2. **Operation Routing (3 tests)**
   - All expected operations (list, install, update, uninstall, etc.)
   - Safe parameter retrieval using Request helper
   - Default operation handling

3. **Input Sanitization (5 tests)**
   - POST data sanitization with XoopsFilterInput
   - HTML output sanitization with MyTextSanitizer
   - Module name escaping with ENT_QUOTES | ENT_HTML5
   - Integer casting for module IDs
   - File name trimming

4. **Confirm Operation (6 tests)**
   - Cache directory writability validation
   - Error handling and display
   - Module data processing
   - Template data assignment
   - Correct template selection

5. **Submit Operation (5 tests)**
   - Name comparison before changes
   - Module change function calls
   - Cpanel cache flushing
   - Active modules cache updates
   - Result assignment to templates

6. **List Operation (5 tests)**
   - Asset loading (CSS/JS)
   - Installed modules retrieval
   - Version update detection
   - Legacy version format handling
   - Module installation counting

7. **Install Operations (4 tests)**
   - Module information retrieval
   - Confirmation dialog display
   - Install function execution
   - Cache updates

8. **Update Operations (2 tests)**
   - Confirmation display
   - Update function execution

9. **Uninstall Operations (2 tests)**
   - Confirmation display with proper message
   - Uninstall function execution

10. **Display Operations (2 tests)**
    - Active state toggling for modules and blocks
    - Menu display toggling via weight

11. **Order Operation (2 tests)**
    - Module ordering processing
    - AJAX endpoint exit behavior

12. **Installlist Operation (2 tests)**
    - Available modules display
    - Module name escaping

13. **General Structure (5 tests)**
    - Valid PHP syntax
    - Required dependency includes
    - Breadcrumb configuration
    - Control panel header/footer calls
    - POST data handling

14. **Edge Cases (5 tests)**
    - Empty module array handling
    - Module existence checks
    - File existence validation
    - File name trimming
    - Clearstatcache usage

15. **Regression Tests (2 tests)**
    - Module variable shadowing documentation
    - Proper increment pattern

### SystemModulesConfirmTemplateTest.php (39 test methods)

Tests for `htdocs/modules/system/templates/admin/system_modules_confirm.tpl`

**Coverage Areas:**

1. **Template Structure (3 tests)**
   - System header inclusion
   - Smarty delimiter syntax
   - Valid HTML structure

2. **Conditional Logic (3 tests)**
   - Two main conditional branches
   - Confirmation form branch
   - Result display branch

3. **Confirmation Form (6 tests)**
   - Iteration over modifications
   - Old/new name display
   - Changed name highlighting
   - Hidden fields for module data
   - Operation field
   - Security token inclusion

4. **Form Controls (1 test)**
   - Submit and cancel buttons with proper actions

5. **Result Display (4 tests)**
   - Result array iteration
   - Row display formatting
   - Logger wrapper div
   - Back link to admin

6. **Table Structure (5 tests)**
   - Proper thead/tbody/tfoot sections
   - Outer class usage
   - Module label in header
   - Alternating row styles with cycle
   - Centered text classes

7. **Smarty Constants (1 test)**
   - Localization constant usage

8. **Security (2 tests)**
   - No hardcoded sensitive data
   - POST method for forms

9. **Accessibility (2 tests)**
   - Form button classes
   - Table cellspacing

10. **Edge Cases (2 tests)**
    - Default filter for null values
    - Proper Smarty block closing

11. **Code Quality (1 test)**
    - No PHP code in template

12. **Regression Tests (3 tests)**
    - Proper colspan in footer
    - CSS class usage
    - JavaScript onclick quoting

13. **Layout (2 tests)**
    - Consistent indentation
    - Trailing whitespace handling

14. **Additional Coverage (4 tests)**
    - Hidden field value escaping
    - Module log ID
    - Safe HTML wrapping in results
    - Empty result array handling

## Running the Tests

### Using the Test Runner Script

\`\`\`bash
./run_module_tests.sh
\`\`\`

### Using PHPUnit Directly

Run both test files:
\`\`\`bash
vendor/bin/phpunit tests/unit/htdocs/modules/system/admin/modulesadmin/MainControllerTest.php
vendor/bin/phpunit tests/unit/htdocs/modules/system/templates/SystemModulesConfirmTemplateTest.php
\`\`\`

Run specific test:
\`\`\`bash
vendor/bin/phpunit --filter testEnforcesAdminPermissionCheck tests/unit/htdocs/modules/system/admin/modulesadmin/MainControllerTest.php
\`\`\`

Run with verbose output:
\`\`\`bash
vendor/bin/phpunit --verbose tests/unit/htdocs/modules/system/admin/modulesadmin/MainControllerTest.php
\`\`\`

## Test Methodology

These tests use **static code analysis** to verify behavior:

1. **Source Code Inspection**: Tests read the source files and analyze their structure
2. **Pattern Matching**: Tests use string matching and regex to verify code patterns
3. **Structural Validation**: Tests ensure proper code organization and security practices
4. **Syntax Checking**: Tests validate PHP and template syntax

This approach allows testing of procedural code without requiring a full XOOPS bootstrap or database connection.

## What These Tests Cover

✅ **Security**: Permission checks, CSRF token validation, input sanitization
✅ **Input Handling**: Parameter retrieval, type casting, XSS prevention
✅ **Output Safety**: HTML escaping, Smarty constant usage
✅ **Operation Logic**: All controller operations and their workflows
✅ **Template Structure**: Valid Smarty syntax, proper variable usage
✅ **Error Handling**: Cache writability checks, error display
✅ **Edge Cases**: Empty arrays, missing data, null values
✅ **Code Quality**: PHP syntax, proper patterns, no hardcoded data

## What These Tests Don't Cover

❌ **Runtime Behavior**: Actual execution with database connections
❌ **Integration**: Interaction with XOOPS core functions
❌ **Rendered Output**: Final HTML output after Smarty processing
❌ **User Interaction**: Form submissions, AJAX calls
❌ **Database Operations**: Module installation/update queries

For full integration testing, these tests should be complemented with:
- Functional tests using a test database
- Browser-based tests with Selenium or similar
- Manual testing in a development environment

## Total Coverage

- **93 test methods** across 2 test files
- **54 tests** for main.php controller logic
- **39 tests** for system_modules_confirm.tpl template
- Comprehensive coverage of security, input/output, operations, and edge cases

## Continuous Integration

These tests are designed to run in CI environments without requiring:
- Database connections
- Web server configuration
- XOOPS bootstrap
- Session handling

This makes them ideal for:
- Pre-commit hooks
- Pull request validation
- Continuous integration pipelines
- Quick smoke testing

tests/unit/htdocs/modules/system/templates/SystemModulesConfirmTemplateTest.php

This is a new file.

<?php
/**
 * Unit tests for modules/system/templates/admin/system_modules_confirm.tpl
 *
 * Tests the module confirmation template structure, variable usage,
 * security token handling, and output logic.
 *
 * `@copyright`    2000-2026 XOOPS Project (https://xoops.org)
 * `@license`      GNU GPL 2.0 or later (https://www.gnu.org/licenses/gpl-2.0.html)
 * `@package`      Tests\Unit\System\Templates
 */

declare(strict_types=1);

namespace Tests\Unit\System\Templates;

use PHPUnit\Framework\TestCase;

/**
 * Tests for system_modules_confirm.tpl template
 *
 * These tests verify the template structure, Smarty syntax, variable usage,
 * and security token handling in the module confirmation template.
 */
class SystemModulesConfirmTemplateTest extends TestCase
{
    /**
     * `@var` string The template content
     */
    private string $templateContent;

    /**
     * `@var` string Path to the template file under test
     */
    private string $filePath;

    protected function setUp(): void
    {
        $possiblePaths = [
            dirname(__DIR__, 5) . '/modules/system/templates/admin/system_modules_confirm.tpl',
            dirname(__DIR__, 4) . '/htdocs/modules/system/templates/admin/system_modules_confirm.tpl',
            dirname(__DIR__, 5) . '/htdocs/modules/system/templates/admin/system_modules_confirm.tpl',
            dirname(__DIR__, 3) . '/modules/system/templates/admin/system_modules_confirm.tpl',
        ];

        $this->filePath = '';
        foreach ($possiblePaths as $path) {
            if (file_exists($path)) {
                $this->filePath = $path;
                break;
            }
        }

        if ($this->filePath === '') {
            $this->markTestSkipped('system_modules_confirm.tpl not found in expected locations');
        }

        $this->templateContent = file_get_contents($this->filePath);
        $this->assertNotEmpty($this->templateContent, 'Template file should not be empty');
    }

    // =========================================================================
    // Template Structure Tests
    // =========================================================================

    /**
     * Verify that template includes system header.
     */
    public function testIncludesSystemHeader(): void
    {
        $this->assertStringContainsString(
            '{include file="db:system_header.tpl"}',
            $this->templateContent,
            'Template should include system_header.tpl'
        );
    }

    /**
     * Verify that template uses Smarty syntax correctly.
     */
    public function testUsesSmartyDelimiters(): void
    {
        $this->assertMatchesRegularExpression(
            '/<\{[^}]+\}>/',
            $this->templateContent,
            'Template should use Smarty delimiters <{ }>'
        );
    }

    /**
     * Verify that template has valid HTML structure.
     */
    public function testHasValidHtmlStructure(): void
    {
        // Should have opening and closing tags properly matched
        $this->assertStringContainsString('<form', $this->templateContent, 'Should have form tag');
        $this->assertStringContainsString('</form>', $this->templateContent, 'Should close form tag');
        $this->assertStringContainsString('<table', $this->templateContent, 'Should have table tag');
        $this->assertStringContainsString('</table>', $this->templateContent, 'Should close table tag');
    }

    // =========================================================================
    // Conditional Logic Tests
    // =========================================================================

    /**
     * Verify that template has two main conditional branches.
     */
    public function testHasTwoMainConditionalBranches(): void
    {
        $this->assertStringContainsString(
            '{if isset($modifs_mods)}',
            $this->templateContent,
            'Should check if modifs_mods is set (confirmation state)'
        );

        $this->assertStringContainsString(
            '{else}',
            $this->templateContent,
            'Should have else clause for result state'
        );

        $this->assertStringContainsString(
            '{/if}',
            $this->templateContent,
            'Should close if statement'
        );
    }

    /**
     * Verify that modifs_mods branch shows confirmation form.
     */
    public function testModifsModsbranchShowsConfirmationForm(): void
    {
        // Extract the modifs_mods branch
        $modifsStart = strpos($this->templateContent, '{if isset($modifs_mods)}');
        $elsePos = strpos($this->templateContent, '{else}', $modifsStart);
        $modifsBranch = substr($this->templateContent, $modifsStart, $elsePos - $modifsStart);

        $this->assertStringContainsString(
            '<form',
            $modifsBranch,
            'Confirmation branch should contain form'
        );

        $this->assertStringContainsString(
            'action="admin.php"',
            $modifsBranch,
            'Form should submit to admin.php'
        );

        $this->assertStringContainsString(
            'method="post"',
            $modifsBranch,
            'Form should use POST method'
        );
    }

    /**
     * Verify that else branch shows results.
     */
    public function testElseBranchShowsResults(): void
    {
        // Extract the else branch
        $elsePos = strpos($this->templateContent, '{else}');
        $endifPos = strpos($this->templateContent, '{/if}', $elsePos);
        $elseBranch = substr($this->templateContent, $elsePos, $endifPos - $elsePos);

        $this->assertStringContainsString(
            '{if isset($result)}',
            $elseBranch,
            'Result branch should check if result is set'
        );

        $this->assertStringContainsString(
            'xo-module-log',
            $elseBranch,
            'Result branch should have module log div'
        );
    }

    // =========================================================================
    // Confirmation Form Tests
    // =========================================================================

    /**
     * Verify that confirmation form iterates over modifs_mods.
     */
    public function testFormIteratesOverModifications(): void
    {
        $this->assertStringContainsString(
            '{foreach item=row from=$modifs_mods|default:null}',
            $this->templateContent,
            'Should iterate over modifs_mods array with default'
        );

        $this->assertStringContainsString(
            '{/foreach}',
            $this->templateContent,
            'Should close foreach loop'
        );
    }

    /**
     * Verify that form displays old and new module names.
     */
    public function testFormDisplaysOldAndNewNames(): void
    {
        $this->assertStringContainsString(
            '{$row.oldname}',
            $this->templateContent,
            'Should display old module name'
        );

        $this->assertStringContainsString(
            '{$row.newname}',
            $this->templateContent,
            'Should display new module name'
        );
    }

    /**
     * Verify that form highlights changed names.
     */
    public function testFormHighlightsChangedNames(): void
    {
        $this->assertStringContainsString(
            '{if $row.oldname != $row.newname}',
            $this->templateContent,
            'Should check if names are different'
        );

        $this->assertStringContainsString(
            'class="bold red"',
            $this->templateContent,
            'Should highlight changed names with bold red class'
        );

        $this->assertStringContainsString(
            '&raquo;',
            $this->templateContent,
            'Should use arrow symbol to indicate change'
        );
    }

    /**
     * Verify that form includes hidden fields for module data.
     */
    public function testFormIncludesHiddenFields(): void
    {
        $this->assertStringContainsString(
            '<input type="hidden" name="module[]"',
            $this->templateContent,
            'Should include hidden module array field'
        );

        $this->assertStringContainsString(
            'name="oldname[{$row.mid}]"',
            $this->templateContent,
            'Should include hidden oldname field with module ID index'
        );

        $this->assertStringContainsString(
            'name="newname[{$row.mid}]"',
            $this->templateContent,
            'Should include hidden newname field with module ID index'
        );

        $this->assertStringContainsString(
            'value="{$row.mid}"',
            $this->templateContent,
            'Should include module ID value'
        );
    }

    /**
     * Verify that form includes operation hidden field.
     */
    public function testFormIncludesOperationField(): void
    {
        $this->assertStringContainsString(
            '<input type="hidden" name="op" value="submit"',
            $this->templateContent,
            'Should set operation to submit'
        );

        $this->assertStringContainsString(
            '<input type="hidden" name="fct" value="modulesadmin"',
            $this->templateContent,
            'Should set fct to modulesadmin'
        );
    }

    /**
     * Verify that form includes security token.
     */
    public function testFormIncludesSecurityToken(): void
    {
        $this->assertStringContainsString(
            '{$input_security}',
            $this->templateContent,
            'Should include security token from controller'
        );
    }

    /**
     * Verify that form has submit and cancel buttons.
     */
    public function testFormHasSubmitAndCancelButtons(): void
    {
        $this->assertStringContainsString(
            'type="submit"',
            $this->templateContent,
            'Should have submit button'
        );

        $this->assertStringContainsString(
            '{$smarty.const._AM_SYSTEM_MODULES_SUBMIT}',
            $this->templateContent,
            'Should use submit constant for button label'
        );

        $this->assertStringContainsString(
            'type="button"',
            $this->templateContent,
            'Should have cancel button'
        );

        $this->assertStringContainsString(
            '{$smarty.const._AM_SYSTEM_MODULES_CANCEL}',
            $this->templateContent,
            'Should use cancel constant for button label'
        );

        $this->assertStringContainsString(
            "onclick=\"location='admin.php?fct=modulesadmin'\"",
            $this->templateContent,
            'Cancel button should redirect to module admin'
        );
    }

    // =========================================================================
    // Result Display Tests
    // =========================================================================

    /**
     * Verify that result section iterates over result array.
     */
    public function testResultSectionIteratesOverResults(): void
    {
        $this->assertStringContainsString(
            '{foreach item=row from=$result|default:null}',
            $this->templateContent,
            'Should iterate over result array with default'
        );
    }

    /**
     * Verify that result section displays each result row.
     */
    public function testResultSectionDisplaysRows(): void
    {
        $this->assertStringContainsString(
            '<div class="spacer">{$row}</div>',
            $this->templateContent,
            'Should display each result row in spacer div'
        );
    }

    /**
     * Verify that result section has logger wrapper.
     */
    public function testResultSectionHasLoggerWrapper(): void
    {
        $this->assertStringContainsString(
            '<div class="logger">',
            $this->templateContent,
            'Should wrap results in logger div'
        );
    }

    /**
     * Verify that result section has back link.
     */
    public function testResultSectionHasBackLink(): void
    {
        $this->assertStringContainsString(
            '<a href="admin.php?fct=modulesadmin">',
            $this->templateContent,
            'Should have link back to module admin'
        );

        $this->assertStringContainsString(
            '{$smarty.const._AM_SYSTEM_MODULES_BTOMADMIN}',
            $this->templateContent,
            'Should use back to admin constant for link text'
        );
    }

    // =========================================================================
    // Table Structure Tests
    // =========================================================================

    /**
     * Verify that table has proper structure with thead, tbody, tfoot.
     */
    public function testTableHasProperStructure(): void
    {
        $this->assertStringContainsString(
            '<thead>',
            $this->templateContent,
            'Table should have thead section'
        );

        $this->assertStringContainsString(
            '</thead>',
            $this->templateContent,
            'Table should close thead section'
        );

        $this->assertStringContainsString(
            '<tbody>',
            $this->templateContent,
            'Table should have tbody section'
        );

        $this->assertStringContainsString(
            '</tbody>',
            $this->templateContent,
            'Table should close tbody section'
        );

        $this->assertStringContainsString(
            '<tfoot>',
            $this->templateContent,
            'Table should have tfoot section'
        );

        $this->assertStringContainsString(
            '</tfoot>',
            $this->templateContent,
            'Table should close tfoot section'
        );
    }

    /**
     * Verify that table uses outer class for styling.
     */
    public function testTableUsesOuterClass(): void
    {
        $this->assertStringContainsString(
            'class="outer"',
            $this->templateContent,
            'Table should use outer class'
        );
    }

    /**
     * Verify that table header displays module label.
     */
    public function testTableHeaderDisplaysModuleLabel(): void
    {
        $this->assertStringContainsString(
            '{$smarty.const._AM_SYSTEM_MODULES_MODULE}',
            $this->templateContent,
            'Table header should display module constant'
        );
    }

    /**
     * Verify that table rows use cycle for alternating styles.
     */
    public function testTableRowsUseCycleForAlternating(): void
    {
        $this->assertStringContainsString(
            "{cycle values='odd, even'}",
            $this->templateContent,
            'Table rows should cycle between odd and even classes'
        );
    }

    /**
     * Verify that table has centered text classes.
     */
    public function testTableHasCenteredTextClasses(): void
    {
        $this->assertStringContainsString(
            'class="txtcenter',
            $this->templateContent,
            'Table should use txtcenter class for centered text'
        );
    }

    // =========================================================================
    // Smarty Constants Tests
    // =========================================================================

    /**
     * Verify that template uses Smarty constants for localization.
     */
    public function testUsesSmartyConstants(): void
    {
        $expectedConstants = [
            '_AM_SYSTEM_MODULES_MODULE',
            '_AM_SYSTEM_MODULES_SUBMIT',
            '_AM_SYSTEM_MODULES_CANCEL',
            '_AM_SYSTEM_MODULES_BTOMADMIN',
        ];

        foreach ($expectedConstants as $constant) {
            $this->assertStringContainsString(
                '{$smarty.const.' . $constant . '}',
                $this->templateContent,
                "Template should use constant: {$constant}"
            );
        }
    }

    // =========================================================================
    // Security Tests
    // =========================================================================

    /**
     * Verify that template does not contain hardcoded sensitive data.
     */
    public function testDoesNotContainHardcodedSensitiveData(): void
    {
        $this->assertStringNotContainsString(
            'password',
            strtolower($this->templateContent),
            'Template should not contain hardcoded passwords'
        );
    }

    /**
     * Verify that form uses POST method for security.
     */
    public function testFormUsesPostMethod(): void
    {
        $this->assertMatchesRegularExpression(
            '/method\s*=\s*["\']post["\']/i',
            $this->templateContent,
            'Form should use POST method'
        );
    }

    // =========================================================================
    // Accessibility Tests
    // =========================================================================

    /**
     * Verify that form buttons have proper classes.
     */
    public function testFormButtonsHaveProperClasses(): void
    {
        $this->assertStringContainsString(
            'class="formButton"',
            $this->templateContent,
            'Form buttons should have formButton class'
        );
    }

    /**
     * Verify that table has cellspacing attribute.
     */
    public function testTableHasCellspacing(): void
    {
        $this->assertStringContainsString(
            'cellspacing="1"',
            $this->templateContent,
            'Table should have cellspacing for visual separation'
        );
    }

    // =========================================================================
    // Edge Case Tests
    // =========================================================================

    /**
     * Verify that template uses default filter for null values.
     */
    public function testUsesDefaultFilterForNullValues(): void
    {
        $this->assertStringContainsString(
            '|default:null',
            $this->templateContent,
            'Template should use default:null filter for safe iteration'
        );
    }

    /**
     * Verify that template properly closes all Smarty blocks.
     */
    public function testProperlyClosesAllSmartyBlocks(): void
    {
        // Count opening and closing foreach tags
        $foreachCount = substr_count($this->templateContent, '{foreach');
        $endForeachCount = substr_count($this->templateContent, '{/foreach}');

        $this->assertEquals(
            $foreachCount,
            $endForeachCount,
            'All {foreach} blocks should be closed with {/foreach}'
        );

        // Count opening and closing if tags
        $ifCount = substr_count($this->templateContent, '{if ');
        $endIfCount = substr_count($this->templateContent, '{/if}');

        $this->assertEquals(
            $ifCount,
            $endIfCount,
            'All {if} blocks should be closed with {/if}'
        );
    }

    /**
     * Verify that template does not have PHP code.
     */
    public function testDoesNotContainPhpCode(): void
    {
        $this->assertStringNotContainsString(
            '<?php',
            $this->templateContent,
            'Template should not contain PHP code'
        );

        $this->assertStringNotContainsString(
            '<?=',
            $this->templateContent,
            'Template should not contain short PHP tags'
        );
    }

    // =========================================================================
    // Regression Tests
    // =========================================================================

    /**
     * Verify that template uses proper colspan in footer.
     */
    public function testFooterUsesProperColspan(): void
    {
        $this->assertStringContainsString(
            'colspan="3"',
            $this->templateContent,
            'Footer should use colspan="3" to span all columns'
        );
    }

    /**
     * Verify that template uses proper CSS classes.
     */
    public function testUsesProperCssClasses(): void
    {
        $expectedClasses = [
            'outer',
            'txtcenter',
            'bold',
            'red',
            'spacer',
            'logger',
            'formButton',
            'foot',
        ];

        foreach ($expectedClasses as $class) {
            $this->assertStringContainsString(
                'class="' . $class,
                $this->templateContent,
                "Template should use class: {$class}"
            );
        }
    }

    /**
     * Verify that JavaScript onclick is properly quoted.
     */
    public function testJavascriptOnclickIsProperlyQuoted(): void
    {
        $this->assertMatchesRegularExpression(
            '/onclick\s*=\s*"[^"]*location\s*=\s*\'[^\']+\'[^"]*"/',
            $this->templateContent,
            'JavaScript onclick should use proper quote nesting'
        );
    }

    // =========================================================================
    // Layout Consistency Tests
    // =========================================================================

    /**
     * Verify that template maintains consistent indentation.
     */
    public function testMaintainsConsistentIndentation(): void
    {
        $lines = explode("\n", $this->templateContent);
        $indentedLines = 0;

        foreach ($lines as $line) {
            if (preg_match('/^\s{4,}/', $line)) {
                $indentedLines++;
            }
        }

        $this->assertGreaterThan(
            0,
            $indentedLines,
            'Template should use indentation for readability'
        );
    }

    /**
     * Verify that template does not have trailing whitespace issues.
     */
    public function testDoesNotHaveTrailingWhitespaceIssues(): void
    {
        // Template should end with a newline
        $this->assertMatchesRegularExpression(
            '/\n$/',
            $this->templateContent,
            'Template should end with a newline'
        );
    }

    // =========================================================================
    // Additional Coverage Tests
    // =========================================================================

    /**
     * Verify that hidden fields use proper value escaping.
     */
    public function testHiddenFieldsUseProperValueEscaping(): void
    {
        $this->assertStringContainsString(
            'value="{$row.oldname}"',
            $this->templateContent,
            'Should escape oldname in hidden field value'
        );

        $this->assertStringContainsString(
            'value="{$row.newname}"',
            $this->templateContent,
            'Should escape newname in hidden field value'
        );
    }

    /**
     * Verify that module log has proper ID for styling.
     */
    public function testModuleLogHasProperId(): void
    {
        $this->assertStringContainsString(
            'id="xo-module-log"',
            $this->templateContent,
            'Module log should have xo-module-log ID'
        );
    }

    /**
     * Verify that result display wraps HTML safely.
     */
    public function testResultDisplayWrapsHtmlSafely(): void
    {
        // Result rows display $row directly, which should contain pre-formatted HTML
        // from the controller
        $this->assertStringContainsString(
            '{$row}',
            $this->templateContent,
            'Should display result row content (HTML from controller)'
        );
    }

    /**
     * Test that template handles empty result arrays gracefully.
     */
    public function testHandlesEmptyResultArrays(): void
    {
        // The {if isset($result)} check ensures empty arrays are handled
        $this->assertStringContainsString(
            '{if isset($result)}',
            $this->templateContent,
            'Should check if result exists before displaying'
        );
    }
}

[mambax7]

Review of PR #1631 — "removed unused code"

Overview

The PR removes dead code from the module admin confirmation workflow. The confirmation page previously had 3 columns (Module, Action, Order), but oldstatus, oldweight, and weight were never populated in the confirm case's PHP code, so the Action column always showed "No changes" and the Order column was always blank. Status toggling and reordering are now handled via JS/AJAX on the list page. The PR correctly removes:

• The newstatus assignment in the confirm case (main.php:243)
• The activate/deactivate logic in the submit case (main.php:298-305)
• The Action & Order columns, and associated hidden form fields from the template

The removal is internally consistent — no dangling references remain. The generic POST extraction at main.php:30-34 doesn't create an issue since nothing reads those variables after the PR.

---

Issues Found

1. Stale colspan="3" (bug — CodeRabbit also flagged this)

system_modules_confirm.tpl line 51 (unchanged by PR):

<td colspan="3">

The table now has only 1 column. This should be changed to just <td>.

2. Indentation regression on <span> tag

The PR changes the <span> from being properly nested inside its <{if}> block to being at the same indentation level:

 <{if $row.oldname != $row.newname}>
-    <span class="bold red">&nbsp;&raquo;&nbsp;<{$row.newname}></span>
+<span class="bold red">&nbsp;&raquo;&nbsp;<{$row.newname}></span>
 <{/if}>

The <span> should remain indented one level deeper than its parent <{if}>.

3. Inconsistent indentation in the logger section

The bottom half of the template has the <div class="logger"> block de-indented from its parent <{if isset($result)}>, and the inner <div class="spacer"> is now at the same level as its <{foreach}> parent. This is a gratuitous formatting change that introduces inconsistency — either keep original indentation or fix it properly:

 <{if isset($result)}>
-        <div class="logger">
-            <{foreach item=row from=$result|default:null}>
-            <div class="spacer"><{$row}></div>
-            <{/foreach}>
-        </div>
+            <div class="logger">
+                <{foreach item=row from=$result|default:null}>
+                    <div class="spacer"><{$row}></div>
+                <{/foreach}>
+            </div>
 <{/if}>

4. Minor: <{/foreach}> indentation change in body

The closing <{/foreach}> tag was moved from 12 to 16 spaces, changing its alignment relative to the opening <{foreach}> which remains at 12. These should match.

---

Verdict

The logic of the PR is correct and the dead code removal is well-justified. The confirm/submit flow now only handles name changes, which matches what the form actually submits. However, the PR needs minor fixes:

1. Fix colspan="3" → remove the attribute
2. Fix the <span> indentation regression
3. Either revert the gratuitous logger indentation changes, or fix them properly
4. Fix the <{/foreach}> alignment

None of these are blockers — they could be addressed in a follow-up fixup commit. The core change (removing dead status/weight code) is sound.

[sonarqubecloud]

Quality Gate passed

Issues
0 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

[mambax7]

@ggoffy Thank you!

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 0.00%. Comparing base (87765a0) to head (3ee60fa).
⚠️ Report is 31 commits behind head on master.

Additional details and impacted files

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

☔ 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.