Skip to content

[3.0] Rewrites installer and upgrader #8652

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 39 commits into
base: release-3.0
Choose a base branch
from

Conversation

Sesquipedalian
Copy link
Member

@Sesquipedalian Sesquipedalian commented May 19, 2025

This is the new installer and upgrader code. It is based on the work in #8093.

It works in my tests, but it probably has bugs. Let's find 'em and fix 'em.

Fixes #7954
Fixes #8261
Fixes #8570
Fixes #8599
Fixes #8616
Fixes #8648

Signed-off-by: Jon Stovell <jonstovell@gmail.com>
Signed-off-by: Jon Stovell <jonstovell@gmail.com>
Signed-off-by: Jon Stovell <jonstovell@gmail.com>
Signed-off-by: Jon Stovell <jonstovell@gmail.com>
Signed-off-by: Jon Stovell <jonstovell@gmail.com>
Signed-off-by: Jon Stovell <jonstovell@gmail.com>
Signed-off-by: Jon Stovell <jonstovell@gmail.com>
Signed-off-by: Jon Stovell <jonstovell@gmail.com>
Signed-off-by: Jon Stovell <jonstovell@gmail.com>
Signed-off-by: Jon Stovell <jonstovell@gmail.com>
Signed-off-by: Jon Stovell <jonstovell@gmail.com>
Signed-off-by: Jon Stovell <jonstovell@gmail.com>
Signed-off-by: Jon Stovell <jonstovell@gmail.com>
Signed-off-by: Jon Stovell <jonstovell@gmail.com>
Signed-off-by: Jon Stovell <jonstovell@gmail.com>
Copy link
Member

@jdarwood007 jdarwood007 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is 2.0 and before stuff going up separately?

@Sesquipedalian
Copy link
Member Author

Is 2.0 and before stuff going up separately?

This PR already includes the necessary migration steps to upgrade from 2.0. We had previously discussed the plan to make upgrading from 1.x a separate converter.

Signed-off-by: Jon Stovell <jonstovell@gmail.com>
Signed-off-by: Jon Stovell <jonstovell@gmail.com>
Signed-off-by: Jon Stovell <jonstovell@gmail.com>
Signed-off-by: Jon Stovell <jonstovell@gmail.com>
@jdarwood007
Copy link
Member

Is 2.0 and before stuff going up separately?

This PR already includes the necessary migration steps to upgrade from 2.0. We had previously discussed the plan to make upgrading from 1.x a separate converter.

I had planned to bake the converter logic into the installer/upgrader to make it still easy to convert Yabb, SMF 1.x and 2.0. The reason is that we are still supporting them, but only through a conversion process. Yabb and SMF 1.x cost a lot of technical debt to maintain the upgrader logic for. So by moving into converters, we reduce that to maintaining how to convert data, which becomes easier with a select this and insert that process.

@Sesquipedalian
Copy link
Member Author

Sesquipedalian commented May 21, 2025

Sure. But that would all just be a matter of writing Migration and Cleanup classes, plus a Tools class to call them, right?

@sbulen
Copy link
Contributor

sbulen commented May 21, 2025

I just ran a 2.1 =>3.0 upgrade & it went very well.

EDIT: I had to rerun for... reasons... Different issues reported below...

On completion, I got this error in the Cleanup phase:
Critical Error!
fileinode(): Argument # 1 ($filename) must be of type string, false given
D:\wamp64\www\van2130\Sources\Maintenance\Cleanup\v3_0\TasksDirCase.php:62

@sbulen
Copy link
Contributor

sbulen commented May 28, 2025

Note a corrollary to the above: The upgrade_x-y_MySQL.sql files must be kept in perfect sync across all releases. I.e., any change in the 3.0 branches to the .sql files must be made in the 2.1 branch files & vice-versa.

Ideally, they're separate & we just maintain one set. 2.0 and 2.1 had drifted significantly over time - I spent a lot of time bringing the 1.x & 2.x .sql files back in sync across the 2.0 & 2.1 repos...

It was painful... Different upgrade paths produced different DB structures...

Signed-off-by: Jon Stovell <jonstovell@gmail.com>
Signed-off-by: Jon Stovell <jonstovell@gmail.com>
Signed-off-by: Jon Stovell <jonstovell@gmail.com>
@Sesquipedalian
Copy link
Member Author

Sesquipedalian commented May 28, 2025

I want to come back to this...

On the 2.1 => 3.0 upgrade, it didn't rerun the 2.1 upgrade steps. Are we changing this going forward?
I suspect they did this because there were DB changes introduced in point releases during 1.x and had to clean
things up. I don't think they could assume a given DB state.

A couple cases in point:

If I upgrade from 2.1.4 (or 2.1.1 - 2.1.3...) to 3.0, neither of those DB changes will be applied.

This is the reason that in the past, all steps from the current to the target were applied - they couldn't assume you had all of the current (2.1.x in this example) DB updates applied before moving to the 3.0 step.

So... In my mind, either (A) we rerun the 2.1 step before the 3.0 step - OR - (B) we make ABSOLUTELY NO DB changes, not even indexes, in point releases (and revert the two changes above prior to 2.1.5) - OR - (C) we add a whole new mechanism to the upgrader to handle point release DB changes.

I'd prefer A or B. I think C would be a waste of time - that's what option A does. We just need to stick to the rule that the upgrader must be re-runnable.

Actually, this is trivial to take care of with the new installer and upgrader system. 4305a83 has fixed the issue for the two PRs you mentioned.

@sbulen
Copy link
Contributor

sbulen commented May 28, 2025

Actually, this is trivial to take care of with the new installer and upgrader system. 4305a83 has fixed the issue.

Hmmm... So, any updates made in point releases going forward, must be replicated in SMF\Migration\v2_1\ and also SMF\Migration\v3_0?

What about non-index updates, e.g., field lengths or new columns, etc.?

@Sesquipedalian
Copy link
Member Author

Sesquipedalian commented May 28, 2025

Actually, this is trivial to take care of with the new installer and upgrader system. 4305a83 has fixed the issue.

Hmmm... So, any updates made in point releases going forward, must be replicated in SMF\Migration\v2_1\ and also SMF\Migration\v3_0?

Not necessarily. If a particular migration step (ie. a database change) should be applied in more than one version of SMF, it can be listed in both appropriate sections of SMF\Maintenance\Tools\Upgrade::MIGRATIONS. In practice, though, it won't normally be necessary to list any given migration step more than once.

What about non-index updates, e.g., field lengths or new columns, etc.?

Any changes related to table structure can be applied by using code similar to these examples:

		$table = new Schema\v3_0\LogSearchResults();
		$table->alterIndex($table->indexes['primary']);
		$table = new Schema\v3_0\Calendar();
		$table->alterColumn($table->columns['did']);

The first example will ensure that the primary key index of the log_search_results table conforms to the definition for it in the SMF\Db\Schema\v3_0\LogSearchResults class. The second example will ensure that the uid column of the calendar table conforms to the definition for it in the SMF\Db\Schema\v3_0\Calendar class.

In each case, the complete definition of the relevant column or index is given by an SMF\Db\Schema\Column or SMF\Db\Schema\DbIndex object. Calling the alterColumn or alterIndex method changes the existing column or index in the database so that all of its attributes match the definition given by the defining object.

This sort of thing is the whole point of the SMF\Db\Schema* classes. 🙂

@sbulen
Copy link
Contributor

sbulen commented May 29, 2025

Not necessarily. If a particular migration step (ie. a database change) should be applied in more than one version of SMF, it can be listed in both appropriate sections of SMF\Maintenance\Tools\Upgrade::MIGRATIONS. In practice, though, it won't normally be necessary to list any given migration step more than once.

I don't think I follow. Why was it needed in two places in only this instance? Any tweaks in 2.1 will cause a similar tweak in 3.0, correct?

Most db changes apply to a specific release & all subsequent releases when you make a change...

If the approach is "take me to THIS structure", as it sounds, then shouldn't it check all tables, columns & indexes? Especially for major releases?

That would make more sense to me than having to pick & choose what might need to be checked in subsequent releases.

The old upgrader, clunky & problematic as it was, would deal with any tweaks, at any time, and allow you to migrate forward from any point release. All you had to do was make one update to one .sql file. I'm not yet comfortable we've matched that functionality...

@Sesquipedalian
Copy link
Member Author

I don't think I follow. Why was it needed in two places in only this instance?

It wasn't. SMF\Migration\v3_0\IndexUpdates is only applied in the 2.1 → 3.0 phase.

If the approach is "take me to THIS structure", as it sounds, then shouldn't it check all tables, columns & indexes? Especially for major releases?

Well, we could add a final migration step for each point release that simply walks through all tables in that version's schema and applies alterColumn or alterIndex for each column and index in each table. That would ensure that we were always dealing with a known table structure. However, it would also increase the likelihood of blocking errors during upgrade—specifically, in situations where a column has gotten out of whack and now has data in it that can't be coerced into the expected form.

@sbulen
Copy link
Contributor

sbulen commented May 29, 2025

I don't think I follow. Why was it needed in two places in only this instance?

It wasn't. SMF\Migration\v3_0\IndexUpdates is only applied in the 2.1 → 3.0 phase.

But it is needed in 2.1 also - its a 2.1 change.

I'm thinking about 3.0 & 3.1 as well, when both are being worked on. The change should really only live in one release.

@sbulen
Copy link
Contributor

sbulen commented May 29, 2025

Well, we could add a final migration step for each point release that simply walks through all tables in that version's schema and applies alterColumn or alterIndex for each column and index in each table. That would ensure that we were always dealing with a known table structure. However, it would also increase the likelihood of blocking errors during upgrade—specifically, in situations where a column has gotten out of whack and now has data in it that can't be coerced into the expected form.

TBH, I like this idea. Though it suggests a need for 'report only' changes vs 'required structural' changes.

And it would also need to factor in the DB changes SMF makes due to user setting changes, e.g., text to mediumtext, custom search index, reports, etc.

Anyway, food for thought.

I'll run a few more tests tonight. Then I'm out of town for 8 days, coming back next weekend.

@sbulen
Copy link
Contributor

sbulen commented May 29, 2025

No luck with the installer...

I copied down 3 files from /other: install.php, Settings.php & Settings_bak.php. If I should be doing something different than that, let me know.

PHP 8.4.5, MySQL 8.4.4.

Also tried with PHP 8.4.5 & Postgresql 14, & got the same result.

image

@sbulen
Copy link
Contributor

sbulen commented May 29, 2025

Just tried a 2.1 => 3.0 upgrade via CLI. It completed OK, but several issues:

  • After upgrade, it took me a few attempts to login. At times I saw the same symptoms as the 'no session' error ([3.0]: Guests cannot see boards when cookies are disabled #8624): no categories or boards. But I had no restrictions on cookies.
  • I'm missing the id_board index on smf_log_notify
  • I'm missing smf_version on smf_log_packages
  • I'm getting cron errors on entity decode, see pic below

image

@sbulen
Copy link
Contributor

sbulen commented May 29, 2025

Just ran the 2.1 => 3.0 upgrade via browser... All the same comments as above, but a minor nit about the graphics - the weird two dark grey lines below the 'please be patient' line. Note I did not ask to have details displayed. I think this is the box for the details:
image

@sbulen
Copy link
Contributor

sbulen commented May 29, 2025

Just ran a 3.0 => 3.0 upgrade via CLI and this is the bottom part of the log:

...
...
+++ Converting "smf_topics" to utf8mb4... skipped.
+++ Converting "smf_user_alerts" to utf8mb4... skipped.
+++ Converting "smf_user_alerts_prefs" to utf8mb4... skipped.
+++ Converting "smf_user_drafts" to utf8mb4... skipped.
+++ Converting "smf_user_likes" to utf8mb4... skipped.

Warning: Undefined array key 72 in D:\wamp64\www\van2130\Sources\Maintenance\Utf8ConverterStep.php on line 667

Call Stack:
0.0005 394264 1. {main}() D:\wamp64\www\van2130\upgrade.php:0
0.0219 1417704 2. SMF\Maintenance\Maintenance->execute($type = 2) D:\wamp64\www\van2130\upgrade.php:24
1.3207 6775704 3. call_user_func:{D:\wamp64\www\van2130\Sources\Maintenance\Maintenance.php:294($callback = [0 => class SMF\Maintenance\Utf8ConverterStep private int $SMF\Maintenance\Step}id = 5; private string $SMF\Maintenance\Step}name = 'Convert to UTF-8'; private string $SMF\Maintenance\Step}title = 'Convert to UTF-8'; private array|string $SMF\Maintenance\Step function = ...; private string ${SMF\Maintenance\Steptemplate = 'convertUtf8'; private int $SMF\Maintenance\Step}progress = 30; public array $supported_charsets = [...]; public string $lang_charset = 'UTF-8'; public array $charset_maps = [...] , 1 => 'convertDatabase']) D:\wamp64\www\van2130\Sources\Maintenance\Maintenance.php:294
1.3207 6775888 4. SMF\Maintenance\Utf8ConverterStep->convertDatabase() D:\wamp64\www\van2130\Sources\Maintenance\Maintenance.php:294

Warning: Attempt to read property "name" on null in D:\wamp64\www\van2130\Sources\Maintenance\Utf8ConverterStep.php on line 667

Call Stack:
0.0005 394264 1. {main}() D:\wamp64\www\van2130\upgrade.php:0
0.0219 1417704 2. SMF\Maintenance\Maintenance->execute($type = 2) D:\wamp64\www\van2130\upgrade.php:24
1.3207 6775704 3. call_user_func:D:\wamp64\www\van2130\Sources\Maintenance\Maintenance.php:294($callback = [0 =&gt; class SMF\Maintenance\Utf8ConverterStep private int ${SMF\Maintenance\Stepid = 5; private string $SMF\Maintenance\Step}name = 'Convert to UTF-8'; private string $SMF\Maintenance\Steptitle = 'Convert to UTF-8'; private array|string $SMF\Maintenance\Step}function = ...; private string $SMF\Maintenance\Steptemplate = 'convertUtf8'; private int $SMF\Maintenance\Stepprogress = 30; public array $supported_charsets = [...]; public string $lang_charset = 'UTF-8'; public array $charset_maps = [...] }, 1 => 'convertDatabase']) D:\wamp64\www\van2130\Sources\Maintenance\Maintenance.php:294
1.3207 6775888 4. SMF\Maintenance\Utf8ConverterStep->convertDatabase() D:\wamp64\www\van2130\Sources\Maintenance\Maintenance.php:294

Step 7: Finalize Upgrade
Upgrade complete!

@sbulen
Copy link
Contributor

sbulen commented May 29, 2025

That's enough for tonight... See you in a week.

Your meditation exercise for the week: contemplate a 2.1.2 => 3.1.3 upgrade. Will the id_board index be there?

And when working on 3.1, do the 3.0 & 3.1 migration release branches for 3.0 need to be kept in sync? How?

@sbulen
Copy link
Contributor

sbulen commented May 29, 2025

One last question: Where's the log?

Ah... Saw your note above, it's only around when it fails...

Hmmm... I'm thinking support board questions... Not sure I'd want it deleted. Might want to consult @Kindred-999 ???

@sbulen
Copy link
Contributor

sbulen commented May 29, 2025

A thought...

If you had migration folders for the point releases, you're probably golden... Just step through each.

In 2.1, you started saving the version in the settings table that way - e.g., "2.1.4". So you know the precise starting point.

We'd need a 2.1.5 one for the two changes in question.

And that way, the exact same process could be used in point releases as in the upgrader. All locked in step. The only difference is the starting point.

@Sesquipedalian
Copy link
Member Author

Sesquipedalian commented May 30, 2025

But it is needed in 2.1 also - its a 2.1 change.

I'm thinking about 3.0 & 3.1 as well, when both are being worked on. The change should really only live in one release.

Your meditation exercise for the week: contemplate a 2.1.2 => 3.1.3 upgrade. Will the id_board index be there?

And when working on 3.1, do the 3.0 & 3.1 migration release branches for 3.0 need to be kept in sync? How?

I have thought about these things. I respectfully suggest that you are thinking about them wrongly, my friend, and in two key ways.

First, you seem to be thinking as though the upgrader needs to replay the evolution of our database structure with a patch release level of granularity, as though we need to model 2.1.0 → 2.1.1 → 2.1.2 → 2.1.3 → 2.1.4 → 2.1.5 → ... → 3.0.0 in the upgrader. Because of this way of thinking about it, you think that 2.1.5's fix to the log_notify table's primary index needs to be handled in an upgrade step that happens before the 2.1 → 3.0 upgrade.

However, the upgrader actually replays the evolution of the database structure with a point release level of granularity, e.g. 2.0.x → 2.1.x, or 2.1.x → 3.0.x. That means a database change introduced in a patch release needs to be reflected in two places: in a 2.1.x → 2.1.x upgrade (i.e. rerunning the upgrader), and in a 2.1.x → 3.0.x upgrade.

That brings us to the second point. You seem to be thinking of the upgrader as though it were an un-versioned, universal thing that applies to all SMF versions equally—we just tack on new sets of changes with each new version of SMF.

But that's not the case. Each version of SMF has a corresponding version of the upgrader, and these versions are indeed distinct. Specifically, only the 2.1 version of the upgrader will ever perform a 2.1.x → 2.1.x upgrade, while only the 3.0 version will ever perform a 3.0.x → 3.0.x upgrade, etc. This difference is why the change to the database in 2.1.5 needs to be reflected in two different places. In the 2.1 upgrader, it needs to be reflected in the 2.1.x → 2.1.x steps. But in the 3.0 upgrader (and beyond), it needs to be reflected in the 2.1.x → 3.0.x steps, because no 2.1.x → 2.1.x steps are or could ever be performed by the 3.0 upgrader.

@Sesquipedalian
Copy link
Member Author

Sesquipedalian commented May 30, 2025

And when working on 3.1, do the 3.0 & 3.1 migration release branches for 3.0 need to be kept in sync? How?

This, like everything else, is managed by Git. As I said in my previous reply, the upgrader is, in fact, versioned along with SMF as a whole and as part of SMF as a whole. When 3.1 is forked from 3.0, it will inherit the migration steps from the 3.0 branch. As new database changes are introduced in 3.1, migration steps will be added for them in the 3.1 branch. If we ever find ourselves needing to introduce a database change in a patch to 3.0, a migration step for that will end up being added in both the 3.0 branch as part of the 3.0x → 3.0.x steps and, separately, in the 3.1 branch as part of the 3.0.x → 3.1.x steps. That's all there is to it.

@Sesquipedalian
Copy link
Member Author

  • After upgrade, it took me a few attempts to login.

That's actually not the upgrader's fault. It's due to the changes introduced in #8464 (specifically, in fed16cc). At the time when I was working on that PR, I was flipping back and forth so much between the old and new password hash values that I didn't notice that it would take two tries to log in successfully once the change was made. We need to make some more changes— probably in SMF\Actions\Login2—in order to handle the change seamlessly.

@Sesquipedalian
Copy link
Member Author

  • I'm missing the id_board index on smf_log_notify
  • I'm missing smf_version on smf_log_packages

Both should be fixed with the latest commits.

Signed-off-by: Jon Stovell <jonstovell@gmail.com>
@Sesquipedalian
Copy link
Member Author

Sesquipedalian commented May 30, 2025

One last question: Where's the log?

Ah... Saw your note above, it's only around when it fails...

Hmmm... I'm thinking support board questions... Not sure I'd want it deleted. Might want to consult @Kindred-999 ???

Log files are now saved in a secured ./logs directory. If that directory can't be created or can't be secured, then the log file is deleted after the tool completes its process.

Signed-off-by: Jon Stovell <jonstovell@gmail.com>
Signed-off-by: Jon Stovell <jonstovell@gmail.com>
@Sesquipedalian
Copy link
Member Author

  • I'm getting cron errors on entity decode, see pic below

b2a365f should fix that.

Just ran a 3.0 => 3.0 upgrade via CLI and this is the bottom part of the log:

Warning: Undefined array key 72 in D:\wamp64\www\van2130\Sources\Maintenance\Utf8ConverterStep.php on line 667

54c268a should fix that.

@Sesquipedalian
Copy link
Member Author

No luck with the installer...

I copied down 3 files from /other: install.php, Settings.php & Settings_bak.php. If I should be doing something different than that, let me know.

PHP 8.4.5, MySQL 8.4.4.

Also tried with PHP 8.4.5 & Postgresql 14, & got the same result.

image

Hm. Weird. I'll need to experiment to see if I can reproduce that and figure out a cause.

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