[3.0] Avoid the deprecated by-reference aliases in saveBatch() - #9314
Open
albertlast wants to merge 2 commits into
Open
[3.0] Avoid the deprecated by-reference aliases in saveBatch()#9314albertlast wants to merge 2 commits into
albertlast wants to merge 2 commits into
Conversation
User::saveBatch() maps table columns onto property names, but had no
entry for member_name or real_name, so both fell through to the default
and addressed the deprecated backward compatibility aliases instead of
the canonical $username and $name.
Those aliases are by-reference property hooks:
public string $member_name {
&get => $this->username;
}
isset() on an uninitialized typed property normally returns false, but
a by-reference hook has to materialise a reference first, so it throws
instead:
Error: Cannot access uninitialized non-nullable property
SMF\User::$username by reference in Sources/User.php:808
UpdateSpoofDetectorNames loads members with UserDataset::None and sets
only $name, so $username is never initialised. The task therefore threw
on every run, which for a logged in member meant a fatal error on page
load.
Maps both columns to the canonical properties, where isset() behaves as
the surrounding code expects.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: albertlast <mathiaspapealbert@hotmail.com>
email_address and usertitle are members table columns that share their names with by-reference backward compatibility aliases, so they hit the same problem the previous commit fixed for member_name and real_name: Cannot access uninitialized non-nullable property SMF\User::$email by reference in Sources/User.php:850 Reached on an ordinary page load, via User::loadMe() -> setLastVisit() -> save() -> saveBatch(). Of the 24 aliases declared with a &get hook, these are the only two that are member columns and are not already handled, either by their own case in the switch (id_member, id_post_group, id_group, lngfile, website_url, website_title) or by an existing entry in this match. The mapping is now exhaustive for the columns saveBatch() can see. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: albertlast <mathiaspapealbert@hotmail.com>
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
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.
Note
This change was produced by an LLM. The code, the commit message and this
description were all written by Claude (Anthropic), driven by @albertlast. It has
not yet had human code review.
Everything stated below was verified by actually running it against a fresh
PostgreSQL install of this branch, rather than only reasoned about. Even so,
please review it as untrusted work: the diagnosis may be right while the fix is
not what SMF would prefer stylistically or architecturally.
Description
User::saveBatch()maps table columns onto property names, but had no entries formember_name,real_name,email_addressorusertitle, so all four fell throughto
default => $columnand addressed the deprecated backward compatibility aliasesrather than the canonical properties.
Those aliases are by-reference property hooks:
isset()on an uninitialized typed property normally returnsfalse, but aby-reference hook has to materialise a reference first, so it throws instead:
Two call paths reach it:
UpdateSpoofDetectorNamesloads members withUserDataset::Noneand sets only$name, so$usernameis never initialised — the task threw on every run.User::loadMe()→setLastVisit()→save()→saveBatch().Combined with the error handler re-entering on the same object (see the separate PR
for that), the second path produced an uncaught fatal on page load.
Of the 24 aliases declared with a
&gethook, these four are the only ones that aremembers columns and were not already handled — either by their own
casein theswitch (
id_member,id_post_group,id_group,lngfile,website_url,website_title) or by an existing entry in thismatch. The mapping should now beexhaustive for the columns
saveBatch()can see.Why not give the properties default values
Defaulting
$username/$emailwould makeisset()returntrueand causesaveBatch()to write empty strings over every member's username and email on abatch update. Skipping uninitialized properties is the correct behaviour.
How this was verified
A regression test driving the exact failing condition — a member loaded with
UserDataset::None, handed tosaveBatch()— run against both branches:release-3.0saveBatch()The comparison matters: it shows the test genuinely detects the bug rather than
passing vacuously. The second row confirms the concern above — the admin's stored
email survived untouched.
Also confirmed
UpdateSpoofDetectorNamesnow completes and writes a correct skeleton(
admin→adrnin), with zero errors logged.Issues References (Fixes|Related|Closes)