Skip to content

[3.0] Checks that every class name written as a string names something real - #9661

Open
albertlast wants to merge 1 commit into
SimpleMachines:release-3.0from
albertlast:tests/class-name-literals
Open

[3.0] Checks that every class name written as a string names something real#9661
albertlast wants to merge 1 commit into
SimpleMachines:release-3.0from
albertlast:tests/class-name-literals

Conversation

@albertlast

Copy link
Copy Markdown
Collaborator

Description

This is red until #9660 merges, and that is the point of it. #9660 fixes a background
task queued as 'SMF\Tasks\FetchSMfiles' when the class is FetchSMFiles. This adds the
test that catches that class of typo, so the check fails on release-3.0 as it stands and
goes green the moment that one character lands. Nothing else here touches production code.

A class name written as a string is not a name PHP ever checks. It reaches the autoloader
only at the moment it is used, which for a background task is a cron run on somebody else's
forum: TaskRunner::execute() logs the class it could not find, drops the row from the
queue and carries on. The work simply never happens, and the only trace is a line in
log_errors. Nothing in CI could see it either — phplint sees a valid string,
php-cs-fixer does not read inside strings, Upgrade::finalize() needs a database so the
unit suite cannot reach it, and the integration suite installs a fresh forum, so the
upgrader never runs.

The test walks Sources/ and Themes/ to build the set of names the forum ships, tokenises
every shipped file to pull out the SMF\... strings written down in it, and asserts each one
names a class, a namespace or a method that exists. Run against release-3.0 it reports
exactly one thing:

Sources/Maintenance/Tools/Upgrade.php:1229 names SMF\Tasks\FetchSMfiles

That is 85 distinct class-name literals across 1,685 files, one true positive and no false
positives.

Why the set is built from the file tree instead of asking class_exists()

Composer's PSR-4 loader ends at file_exists(), which is case-insensitive on Windows and
macOS, so class_exists('SMF\Tasks\FetchSMfiles') returns true there. A test written the
obvious way would have passed on the two Windows legs of the unit matrix and failed on the
two Linux ones, which reads as a flaky test rather than a real bug.

A directory entry stores the case it was created with on every platform — only lookup is
insensitive — so a set built from the file names is exact everywhere, and a plain string
comparison against it is case-sensitive even where the filesystem is not.

Only the class half is treated that way. Method names are checked with method_exists(),
because PHP method names really are case-insensitive and resolve through traits and parents:
Actions\Groups::call comes from ActionTrait and is not declared in the class at all.

Two things it deliberately ignores
  • Existence probes. A name inside class_exists(), function_exists() and their
    relatives is a question, not a claim. This covers the bootstrap-order checks in Config
    and IntegrationHook, the two Unicode helper probes in Punycode, and the one proxy.php
    keeps for the benefit of old mods.
  • Namespace prefixes. ActionRouter asks whether its own class sits under 'SMF\Actions',
    which is a namespace and never a class. Treating every namespace along the way as a valid
    name absorbs that without needing an allowlist — it is the only bare prefix in the tree.

It reads tokens rather than lines because a docblock saying @var SMF\Utils is not a
reference and there are thousands of those.

Cost

0.151s. No CI change is needed; phpunit.yml already runs the unit suite on ubuntu and
windows across PHP 8.4 and 8.5, and this is exactly the sort of bug that wants all four.

Issues References (Fixes|Related|Closes)

  1. Related to Fix a minor capitalization error #9660 — this PR is red until that one merges.

🤖 Generated with Claude Code

A class name in a string is not a name PHP ever checks. It reaches the
autoloader only at the moment it is used, which for a background task is
a cron run on somebody else's forum: TaskRunner::execute() logs the class
it could not find, drops the row from the queue and carries on, so the
work never happens and the only trace is a line in log_errors.

The test walks Sources/ and Themes/ to build the set of names the forum
ships, tokenises every file to pull out the SMF\... strings it writes
down, and asserts each one names a class, a namespace or a method that
exists. Existence probes are skipped, since naming something that may not
be there is the entire point of class_exists() and its relatives.

The set is built from the file tree rather than asked of class_exists()
because Composer's PSR-4 loader ends at file_exists(), which is
case-insensitive on Windows and macOS. A misspelling that only differs in
case would pass on two of the four legs of the unit matrix and fail on
the other two. A directory entry stores the case it was created with
everywhere, so a set built from the file names is exact on all of them.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: albertlast <mathiaspapealbert@hotmail.com>
@Sesquipedalian

Copy link
Copy Markdown
Member

Checking for this is a good idea, yes.

It would also be helpful to get a separate PR that found all such strings in the current code and replaced them with ::class resolvers (e.g. \SMF\Tasks\FetchSMFiles::class in place of 'SMF\\Tasks\\FetchSMFiles').

@albertlast

Copy link
Copy Markdown
Collaborator Author

So we would like to have \SMF\Tasks\FetchSMFiles::class instead of the other way in the complet code base?

@jdarwood007

Copy link
Copy Markdown
Member

Correct.

This PR is useful in cases where we can't use ::class and serves to ensure that it should be a valid class we are referencing. I would still merge this to ensure we don't make a mistake should we need to adjust or add to the hard-coded instances

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants