Skip to content

fix(installer): make "remove the install folder" work (3.6) - #2552

Open
elcreator wants to merge 2 commits into
evolution-cms:3.6.xfrom
elcreator:fix-installer-removal-3.6
Open

elcreator wants to merge 2 commits into
evolution-cms:3.6.xfrom
elcreator:fix-installer-removal-3.6

Conversation

@elcreator

@elcreator elcreator commented Nov 4, 2026 •

Copy link
Copy Markdown

Problem

The box "Remove the install folder" on the last page of the web installer does not work, seen on 3.5.9, 3.5.x and 3.6.x: the folder stays and the browser gets Not found. If you were installing Evolution CMS, the installer session has expired. It happens on a new site and on an update, on every web server.

install/src/controllers/install.php requires core/includes/define.inc.php, which ends with unset($base_path, $base_url) at the global scope. The controller then looks for $base_path . 'install.session.php', with $base_path now null, finds nothing, and skips writeLock(). The page still prints a token, but the lock never holds it, so remove_installer.processor.php refuses it. Seen on a real site by logging the values at that line: {"base":null,"exists":false}.

InstallerWebPathTest did not notice because it writes the lock by hand and runs only the processor.

On nginx the box could not have worked anyway: ng.inx runs PHP only from an allowlist, and the processor is not on it.

Change

  • install.php uses EVO_BASE_PATH, which define.inc.php leaves alone.
  • ng.inx allows manager/processors/remove_installer.processor.php. It stays guarded by the one-time token.
  • New core/tests/Feature/InstallerRemovalFlowTest.php: a real site on SQLite behind php -S, driven through the real forms like a browser. On a new site and on an update (an administrator with the installer grant from the Manager) it checks that the ticked box removes the folder and the lock, and that an unticked box keeps the folder, shut to other browsers. It also checks that a forged token removes nothing.
  • The test cannot reach a real database, whatever installation it runs from. Local configuration stays out of the copy (.env files, core/custom, connection files, the env cache), the server starts with an allowlisted environment, the site gets its own core/custom/.env, and before the first write each test asks the site what it resolves and stops on anything else. Every flow runs next to conflicting settings that lead to a separate SQLite file, whose hash must not change.
  • ApacheConfigHardeningTest pins the ng.inx entry, and that other processors stay refused.

Verification

In evo-ci-php:8.4, PHP 8.4.26:

  • The new test passes with the fix, and without it all 5 removal cases fail, on both 3.5.x and 3.6.x.
  • Isolation, broken on purpose: with the copy filter off, all 10 flow cases stop before writing. With the filter, the check and the fixture's own config all off, the installer's migrations did go to the conflicting database (Connection: default, Database: .../original.sqlite), so the risk is real.
  • composer test -- --compact --fail-on-warning --fail-on-risky --fail-on-deprecation: on 3.6.x 2447 passed, 9 skipped, 0 failed.

Notes

  • The new test copies the tree with its vendor once and runs seven installs, which adds around 30 seconds to composer test.
  • /update.php, which the classic updater of the manager runs, is not on the ng.inx allowlist either. Not changed here: opening it is a separate decision.
  • The same change for 3.5.x is in fix(installer): make "remove the install folder" work #2551.

🤖 Generated with Claude Code

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Seiger commented Nov 4, 2026

Copy link
Copy Markdown

Could we isolate the database configuration of InstallerRemovalFlowTest.php before merging?

installerFlowCopy() copies the working tree, including .env files and custom configuration. The exclusion list removes the generated default connection, but does not exclude these other configuration sources. Clearing inherited DB_*, EVO_*, and APP_* variables before starting PHP does not prevent the copied application from loading them again from its configuration files.

If the test runs from an existing configured installation, those settings can override the intended temporary SQLite connection. Because the test drives real install/update flows, migrations or seeding could then reach a real database.

Please create a clean, test-owned configuration, exclude local environment/custom configuration from the copy, and verify that the effective database connection points to the fixture’s temporary SQLite file before any database writes. A regression test with deliberately conflicting local configuration would help ensure that neither the fresh-install nor update scenario can touch the original database.

SokilAI

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@elcreator

Copy link
Copy Markdown
Author

Thanks, the risk was real. With the isolation removed on purpose, the installer's migrations did reach the conflicting database (Connection: default, Database: .../original.sqlite).

Fixed in the latest commit:

  • Local configuration stays out of the copy: .env files, core/custom apart from the samples, connection files, the env cache and config.inc.php.
  • The server starts with an allowlisted environment, because Symfony Process also merges $_ENV.
  • The fixture writes its own core/custom/.env.
  • Before the first write each test asks the site what it resolves and stops on anything else.
  • Every fresh and update flow runs next to conflicting settings that lead to a separate SQLite file, and its hash is checked after each test. A dataset test plants settings in five places and checks that the flow is refused.

With the copy filter disabled, all 10 flow cases stop before writing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants