[3.0] Stop the current board being rebuilt into an empty object - #9433
Open
albertlast wants to merge 1 commit into
Open
[3.0] Stop the current board being rebuilt into an empty object#9433albertlast wants to merge 1 commit into
albertlast wants to merge 1 commit into
Conversation
Any page that has a board and then walks the board tree is a 500: ?action=search;board=1.0 ?action=search;topic=1 ?action=admin;area=manageboards;board=1.0 Typed property SMF\Board::$id must not be accessed before initialization Category::getTree() rebuilds each board from the row it has just read, and to do that it drops the old object first: unset(Board::$loaded[(int) $row['id_board']]); Board::init((int) $row['id_board'], $row); For every board but one that works. For the board the member is actually on, Board::init() finds nothing loaded and constructs a new object - and the constructor sees an id equal to Board::$board_id, takes its "load the current board" path, finds Board::$info already set, and returns having assigned nothing. What comes back is an object with no id, no name and no anything, and the tail of the constructor reads $this->id straight away. The board is not really gone, though: Board::$info is still holding it. So put that one back instead of building a second object claiming to be the same board, and let the existing branch below apply the fresh row to it. getTree() then finds Board::$loaded[$id] where it expects it, and the tree it builds refers to the same instance as the rest of the request rather than a copy. Nothing else changes: the new branch only fires when Board::$info is set, the id is the current board, and that board is missing from the loaded list, which is the combination that used to throw. Verified by sweeping 22 pages as an admin, with the current board set to a top level board and to a child board in turn: the three URLs above now load, every page that already worked still does, and the board trees on Manage Boards, the search picker and Manage Permissions come out byte for byte the same as on release-3.0. The error log is clean apart from the known calendar TimeInterval fault. Signed-off-by: Mathias Albert <mathiaspapealbert@hotmail.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.
Description
Noticed while testing #9432. Any page that has a current board and then walks the
board tree is a 500:
Category::getTree()rebuilds each board from the row it has just read, and todo that it drops the old object first:
For every board but one that is fine. For the board the member is actually on,
Board::init()finds nothing loaded and constructs a new object — and theconstructor sees an id equal to
Board::$board_id, takes its "load the currentboard" path, finds
Board::$infoalready set, and returns having assignednothing at all:
What comes back is an object with no id, no name and no anything, and the tail of
the constructor reads
$this->idimmediately.The fix
The board is not really gone —
Board::$infois still holding it. So put that oneback rather than building a second object claiming to be the same board, and let
the existing branch below apply the fresh row to it.
getTree()then findsBoard::$loaded[$id]where it expects it, and the tree it builds refers to thesame instance as the rest of the request rather than to a copy.
The new branch only fires when
Board::$infois set, the id is the current board,and that board is missing from the loaded list — the exact combination that
used to throw. Every other path through
init()is untouched.Testing
Swept 22 pages as an admin, with the current board set to a top level board and
then to a child board (so the parent/child branch of the constructor is
exercised too):
Permissions come out identical to
release-3.0— compared by fingerprintingevery board input, board link and category name on each page;
TimeIntervalfault ([3.0] Calendar 500s on any forum with an event: SMF\TimeInterval is not a usable \DateInterval #9384, fixed by [3.0] Let SMF\Time do arithmetic with an SMF\TimeInterval #9405).Issues References (Fixes|Related|Closes)
Related to #7933