[3.0][Testing] Make the code style check runnable locally - #9346
Open
albertlast wants to merge 15 commits into
Open
[3.0][Testing] Make the code style check runnable locally#9346albertlast wants to merge 15 commits into
albertlast wants to merge 15 commits into
Conversation
Provides a reproducible local stack so contributors can work on SMF without installing PHP, Composer or PostgreSQL on the host: - PHP 8.4 on Apache, with every extension other/requirements.md lists as required (mbstring, fileinfo, pgsql, mysqli) or recommended (gd, intl, curl, exif, ftp, xsl, zip). - PostgreSQL 17, with standard_conforming_strings forced on at database level as SMF requires. - Mailpit, so mail() is captured locally and nothing can be sent out. - Adminer, for browsing the database. The entrypoint runs composer install, waits for the database, generates a Settings.php pointed at the db service and drops install.php into place, so a fresh checkout is ready to install on first boot. Everything lives under .docker/ because check-smf-index.php and check-smf-license.php skip dot directories, so the environment cannot break the file integrity checks. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: albertlast <mathiaspapealbert@hotmail.com>
Install::finalize() called Time::strftime() to build the log_activity date, and later Logging::updateStats(), which does the same thing. Both end up in Time::__construct(), which reads User::$me to resolve the time zone. But User::setMe()/User::loadMe() were not called until much later in the same method, so installation died with: Error: Typed static property SMF\User::$me must not be accessed before initialization in Sources/Time.php:191 The installer therefore aborted on its last step, leaving the forum without the member, topic and message stats that finalize() is responsible for writing, including latestMember and latestRealName. Moves the user initialisation up to just after the settings are reloaded, which is the first point at which it can run, and leaves the rest of the "we've just installed" block where it was. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: albertlast <mathiaspapealbert@hotmail.com>
SMF supports MySQL and PostgreSQL, and until now this environment only offered one of them. Both database services now start, and SMF_DB_TYPE decides which one the generated Settings.php points at. It defaults to mysql, since that is what the great majority of installs run on. The two engines keep separate volumes, so a forum can be installed on each and switched between by deleting Settings.php and restarting. Settings.php wins over SMF_DB_TYPE once it exists, and the entrypoint says so rather than silently ignoring the variable. The postgres service is renamed from `db` to say what it is, and keeps `db` as a network alias so Settings.php files written by the previous version still resolve. Engine settings are pinned the same way the postgres side already pinned standard_conforming_strings: utf8mb4 and InnoDB, matching SMF's own table DDL. The collation is deliberately left at the charset default, because SMF sets CHARSET without COLLATE, and forcing one here would diverge from the tables it creates. Also corrects the everyday-use notes: php.ini, the vhost and the entrypoint are copied into the image, so editing them needs a rebuild rather than a restart. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
PostgreSQL logs every statement that errors together with the SQL that caused it, with no configuration needed, and the log is only on the container stderr. That makes `docker compose logs postgres` the most useful debugging tool in the stack, and nothing said so. MySQL logs server errors only, never the client statement that failed, so the note points out the asymmetry: now that mysql is the default engine, a suspected SQL problem is worth reproducing on postgres. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Maintenance::exit() renders the tool's templates, and those are the only place errors are ever shown. On the command line it takes the fallthrough path instead and goes straight to die(), so nothing was reported and the exit status was always 0: a scripted install that died on step three looked exactly like one that had finished. ToolsBase::updateSettingsFile() made the same assumption more directly, calling die() outright when Settings.php could not be written rather than recording the error the way the web path does. Writes the warnings and errors to stderr and exits non-zero when the tool actually failed. A step that merely wants input it was not given sets neither, so pausing part way through is still a success - the installer is meant to be called more than once - and that case now says which step it stopped on instead of nothing at all. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: albertlast <mathiaspapealbert@hotmail.com>
Two things in the installer only hold when a browser is on the other end,
and both are reached before the forum exists, so neither could be worked
around from outside.
defaultHost() reads $_SERVER['SERVER_NAME'] and ['SERVER_PORT'] whenever
HTTP_HOST is absent. On the command line none of the three is set, so
every run began with an undefined index warning. Falls back to localhost:
the value only seeds the suggested board URL on the form, and a scripted
install passes its own boardurl in.
forumSettings() then built the same suggestion with
substr($self, 0, strrpos($self, '/')). getSelf() is $_SERVER['PHP_SELF'],
which in a request is a rooted path but on the command line is whatever
was typed - usually a bare 'install.php' with no directory in it. strrpos()
returns false, and substr() with a false length is fatal on PHP 8, so the
installer died here on every CLI run.
While in there: an unrecognised database type reported
Lang::getTxt('upgrade_unknown_error'), which is not a string that exists.
The fatal error was therefore blank in the browser too. Names the type
that was rejected and the ones that would have been accepted, which
matters most on the command line where the type is typed by hand rather
than picked from a list of exactly those keys.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: albertlast <mathiaspapealbert@hotmail.com>
finalize() ends by signing the new administrator in, so the browser that just ran the installer lands on an admin session instead of a login form. It sets a login cookie, then records the session against the user agent that asked for it. None of that has any meaning on the command line. There is no browser to hold the cookie and no user agent to key the session on, so every CLI install ended with four warnings - headers sent after output had already started, a session that could not be started, and an id that could not be regenerated - and then wrote a sessions row built from an undefined HTTP_USER_AGENT. Runs the whole block only when there is a request behind it. The stats that follow it are untouched, so an install still records latestMember, totalMessages and totalTopics either way. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: albertlast <mathiaspapealbert@hotmail.com>
Two things were wrong with the note the command line prints when a tool stops part way. It indexed the step list to get the number, which counts from zero, while every other line of output uses the step's own id, which counts from one - so it disagreed with the "Step 3: Database Settings" lines immediately above it. It also fired on a successful run. Tools deliberately return false from their last step so the web flow stops and renders its "all done" template, which means reaching that step is success rather than a pause, and a completed install claimed to have stopped at it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: albertlast <mathiaspapealbert@hotmail.com>
The dev environment stopped at a Settings.php and a staged install.php, leaving the actual install to a human clicking through a browser. That is the one step between a fresh clone and a running forum that could not be scripted, and everything that wants to test against a real install has to start by doing it. Adds four scripts under .docker/: install-forum.sh installs a forum, no browser involved use-engine.sh switches which installed forum is live reset.sh empties one engine's database and restages lib.sh shared settings and engine name normalisation The installer is already CLI-native - parseCliArguments() turns --name=value into $_POST and execute() runs every step in one process - so this is two passes rather than 2.1's five curl requests. The second pass carries pop_done, which is the short-circuit past the population report; passing it on the first pass would skip building the schema. --engine both installs MySQL and then PostgreSQL. It has to be sequential: Settings.php pins a single db_type and Db::load() hands back the connection it already made, so only one engine is ever live in a process. Both installs are kept, and use-engine.sh swaps between them by putting the saved Settings.php back - no restart, because the entrypoint only writes one when there is not one already. --pin-secrets fixes auth_secret and image_proxy_secret, which are generated with random_bytes() and stored nowhere but Settings.php. Without it the two installs differ by more than their database and a login cookie does not survive the switch. The cookie name needs no such help: createCookieName() is a crc32 of the database name and prefix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: albertlast <mathiaspapealbert@hotmail.com>
The README invokes them as .docker/install-forum.sh rather than through bash, which only works with the bit set. Windows checkouts do not carry it, so it has to be recorded in the index. lib.sh is left alone: it is sourced, never run. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: albertlast <mathiaspapealbert@hotmail.com>
SectionComments.php called array_first(), which arrived in PHP 8.5. The platform in composer.json is 8.4.1, and AGENTS.md tells contributors to run composer lint before every commit - so on the lowest PHP the project claims to support, the documented pre-commit check could not run at all. It died with "Call to undefined function array_first()" on every file, changed or not. CI never noticed because php-cs-fixer.yml runs in a container that happens to ship 8.5. array_key_first() has been available since 7.3 and says the same thing here, since what the call wants is the first value of an array keyed by token type. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: albertlast <mathiaspapealbert@hotmail.com>
Reproducing a CI failure meant reading the workflow YAML and reconstructing the invocation by hand, which is how a code style problem got pushed twice in a row on the branch this came from. .docker/ci.sh runs sign-off, the four file integrity checks, phplint and php-cs-fixer, plus the test suite when the branch has one. It keeps going after a failure and lists everything that failed at the end, rather than stopping at the first and hiding the rest. --full asks for the whole-tree style check. That is not a nicety: the style workflow only looks at the files a pull request changed, except when composer.lock or the fixer config is in the diff, in which case it checks all seventeen hundred. A branch touching a dependency therefore inherits anything already non-compliant on release-3.0, and finding that out before pushing is worth the extra half minute. It cannot cover the other half of the matrix: CI runs on PHP 8.4 and 8.5 and the container is whichever PHP_VERSION built it. The header and the README say so, along with how to rebuild against the other one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: albertlast <mathiaspapealbert@hotmail.com>
albertlast
changed the title
This was referenced Aug 2, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Two related things, both about being able to reproduce a CI result before
pushing rather than after.
composer lintcannot run on the PHP this project supports.github/phpcs/SectionComments.phpcalledarray_first(), which arrived inPHP 8.5.
composer.jsonpins the platform to 8.4.1, and AGENTS.md tellscontributors to run
composer lintbefore every commit.So on the lowest PHP the project claims to support, the documented pre-commit
check could not run at all — it died with
Call to undefined function array_first()on every file, changed or not, so it was not obvious that theproblem was the tool rather than your branch. CI never noticed, because
php-cs-fixer.ymlruns in a container that happens to ship 8.5.array_key_first()has been available since 7.3 and says the same thing here:what the call wants is the first value of an array keyed by token type.
Verified the whole tree is clean on both — 8.4 in the dev container (which could
not get past the first file before), and 8.5 in the same image CI uses.
.docker/ci.shReproducing a CI failure otherwise means reading the workflow YAML and
reconstructing the invocation by hand. This runs the lot:
Mirrors
php.yml(sign-off, the four integrity checks, phplint) andphp-cs-fixer.yml, plus the test suite when the branch has one. Every check runseven after one fails and the failures are listed at the end, because finding out
about the second problem on the next push is the thing this exists to stop.
--fullis the interesting flag. The style workflow only looks at the files a PRchanged — except when
composer.lockor the fixer config is in the diff, inwhich case it checks all seventeen hundred. So a branch that touches a dependency
silently inherits anything already non-compliant on
release-3.0. Running--fullon this branch reports exactly one file,Sources/PersonalMessage/Search.php,which nothing here touches and which #9345 fixes.
It deliberately does not pretend to cover the whole matrix: CI lints and tests on
8.4 and 8.5, and the container is whichever
PHP_VERSIONbuilt it. The scriptheader and the README both say so, and give the one-line rebuild for the other.
Verified
.docker/ci.shexits 0 on a clean tree and 1 with a violation present —checked by planting one, including an untracked file, which an earlier
draft of the changed-file detection missed.
shellcheckclean on every script in.docker/.Merge order
Merge #9317 and #9344 before this one. Both are contained in this branch, so the
diff shown here is theirs as well as its own; once they land and this is rebased on
release-3.0, what is left is.docker/ci.shand thearray_first()fix.Issues References (Fixes|Related|Closes)
ci.shlives in.docker/.ci.shsources.docker/lib.sh.--fullreports.