Skip to content
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

Installer and upgrader rewrite #8093

Draft
wants to merge 66 commits into
base: release-3.0
Choose a base branch
from

Conversation

Sesquipedalian
Copy link
Member

@Sesquipedalian Sesquipedalian commented Feb 8, 2024

@jdarwood007, let's use this to work collaboratively on the installer/upgrader rewrite.

  • Add Schema for 3.0
  • Add Initial framework for Maintenance logic
  • Build Installer logic
  • Test Installer logic
  • Build Upgrader logic
  • Test Upgrader logic
  • Setup Migration logic for upgrades from SMF 2.1 to 3.0
  • Setup Migration logic for upgrades from SMF 2.0 to 2.1
  • Build Converter Logic
  • Setup Converter logic for YabbSE
  • Setup Converter logic for SMF 1.0
  • Setup Converter logic for SMF 1.1
  • Build Repair Settings tool
  • Build logic for self-contained tools to utilize the maintenance logic.
  • Add logic back for installer to work with compression (?obgz)
  • Allow upgrader to support login or entering in the database password.

@Sesquipedalian Sesquipedalian added this to the 3.0 Alpha 2 milestone Feb 8, 2024
@Sesquipedalian Sesquipedalian changed the title Creates SMF\Db\Schema and descendents Installer and upgrader rewrite Feb 8, 2024
@Sesquipedalian Sesquipedalian reopened this Feb 8, 2024
@jdarwood007
Copy link
Member

jdarwood007 commented Feb 9, 2024

A lot is incoming, it's my initial work I started to write the installer logic. You should be able to get through an install. But the base logic should get it going for an upgrader and converter logic.

@jdarwood007
Copy link
Member

I've also added some task notes based on what I was thinking, please amend or adjust.

@Sesquipedalian
Copy link
Member Author

Sesquipedalian commented Feb 13, 2024

Please remember not to force-push to a shared branch, @jdarwood007. I was able to reconcile the differences in my local repository fairly easily this time, but another time I might not be so lucky.

public static function isInstalled(): bool
{
foreach (['image_proxy_secret', 'db_passwd', 'boardurl'] as $var) {
if (Config::${$var} === Config::$settings_defs[$var]['default']) {
Copy link
Member

Choose a reason for hiding this comment

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

@Sesquipedalian $settings_defs is protected and is not accessible.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, right.

use SMF\Db\DatabaseApi as Db;
use SMF\Maintenance\Migration;

class Migration1022 extends Migration
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be Migration1023

@Sesquipedalian
Copy link
Member Author

I made some changes, @jdarwood007.

First, I moved some stuff around and adjusted some names to make naming and organization more consistent.

Second, instead of using numbered migration sub-steps, I gave them all names and created an array constant (SMF\Maintenance\Tools\Upgrade::MIGRATIONS) that lists them all in the intended order. I did this because (1) it is more reliable to use an order that is explicitly defined in code rather than an implicit order that depends on number file names, and (2) this will make it easier to adjust the order whenever necessary. The latter consideration will become important once we start introducing more database changes as 3.0 develops.

Signed-off-by: Jon Stovell <[email protected]>

# Conflicts:
#	Languages/en_US/Maintenance.php
@jdarwood007
Copy link
Member

jdarwood007 commented Mar 28, 2024

That works as well. My intention for the numbering was to just glob the directory, which then with them named in an order, would return a list for us to use to iterate through with an iterator seek (https://www.php.net/manual/en/arrayiterator.seek.php) when we restart the script.

I have had no time for coding lately. It spring time, so house work to be done and I'm releasing a new app for work. Trying to take time off, but its going to be for more house work.

@jdarwood007
Copy link
Member

I lost the last half hour of work I did to custom fields. Will try to work on this more as I get time. I am currently in the 'Messenger fields' section in the SQL files. You have to compare both MySQL and Postgresql because we appear to sometimes do different things or skip steps in some. The SQL files are still about 2500 lines of code to review. We still need to build converter logic as well to handle YabbSE, SMF 1.x, and SMF 2.0 (if we don't want to rewrite its upgrade). As well we will have to completely revalidate the entire 2.0 to 2.1 upgrade logic here.

@Oldiesmann
Copy link
Contributor

Found an install-related bug that should be fixed in this PR assuming it still applies (I haven't looked at the code yet). The MessageFormatter stuff relies on $txt['lang_locale'], which is defined in the index language file. However, this file isn't loaded in install.php, which will cause errors.
Normally I'd just submit a PR for the fix but since you're in the process of rewriting the installer anyway I didn't think that was necessary.

# Conflicts:
#	other/install.php
#	other/install_3-0_MySQL.sql
#	other/install_3-0_PostgreSQL.sql
#	other/upgrade.php
Copy link
Collaborator

@Tyrsson Tyrsson left a comment

Choose a reason for hiding this comment

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

In reference to the add method in the Column class.

Imho this should not be allowed from a column class. Table classes should consume column classes.

Providing means for a column to add itself to a table is kinda like the tail wagging the dog.

* If 'update', column is updated.
* @return bool Whether or not the operation was successful.
*/
public function add(string $table_name, string $if_exists = 'update'): bool
Copy link
Collaborator

@Tyrsson Tyrsson May 18, 2024

Choose a reason for hiding this comment

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

This method is the one I reference in my other comment.

Why should columns be concerned with tables? Tables should be concerned about columns. Not vice-versa.

The Table class should expose an addColumn method. The Table class already knows which db table its tied to by its $name property. Well, I say it should expose an addColumn. Honestly there should be a group of DDL objects that expose this, and Column should be the parent.

Question:
How, in the future, do you propose to test that the column this instance represents is representative of a Binary, DateTime or TimeStamp for example? We cant test based on type as that is apparent because we do not have objects that represent those "types". So the only choice is to test the instances state since this Column instance can possibly represent all "types". It handles all responsibilities. It should handle only the common responsibilities shared for all column types. Int and text are very different creatures. SRP in OO is a thing for a reason.

The behavioral contract here is that tables contain columns. You do not write queries of the nature (sudo code):
COULMN ALTER TABLE
its ALTER TABLE ADD, DROP, MODIFY
You can not get there from a column. Any code / API modeling it should not be able to get there either. It's a breach of contract in respect to the objects responsibility. What it represents in the context of the system should not be able to perform the action this method provides.

# Conflicts:
#	Languages/en_US/Maintenance.php
#	other/upgrade-helper.php
#	other/upgrade_2-1_MySQL.sql
#	other/upgrade_2-1_PostgreSQL.sql
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.

None yet

6 participants