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

More upgrader fixes #8204

Merged

Conversation

Sesquipedalian
Copy link
Member

@Sesquipedalian Sesquipedalian commented May 9, 2024

Fixes #8197
Fixes #8198
Fixes #8199

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>
@Sesquipedalian Sesquipedalian added this to the 3.0 Alpha 2 milestone May 9, 2024
@Sesquipedalian
Copy link
Member Author

@Oldiesmann, please test to confirm that upgrading now works in both the browser and on the command line.

@Oldiesmann
Copy link
Contributor

Upgrade went smoothly with MySQL via the browser. Will test with PostgreSQL on command line shortly.

@Oldiesmann
Copy link
Contributor

Upgrade went fine in the shell as well but I still got a deprecation warning for pg_version() even after making the changes, this time on line 75 of upgrade.php.

@Sesquipedalian
Copy link
Member Author

Latest commit should fix that, @Oldiesmann. Can you please test on PG one more time?

@Oldiesmann
Copy link
Contributor

Oldiesmann commented May 10, 2024

New problem this time.

PHP Fatal error:  Uncaught TypeError: strpos(): Argument #1 ($haystack) must be of type string, false given in /.../Sources/QueryString.php:70
Stack trace:
#0 /,,,/Sources/QueryString.php(70): strpos()
#1 /.../upgrade.php(848): SMF\QueryString::cleanRequest()
#2 /.../upgrade.php(241): loadEssentialData()
#3 {main}
  thrown in /.../Sources/QueryString.php on line 70

The problem is that $_SERVER['QUERY_STRING'] is never set when running via CLI (since there is no query string as we're running the file directly through the PHP interpreter). Best option would be to just check whether we're running from the command line and skip that check if so. I'm not sure why I didn't run into this last time, unless I just wasn't paying attention.

@Tyrsson
Copy link
Collaborator

Tyrsson commented May 10, 2024

Normally, and this is just a general observation. QueryString will pretty much mirror argv. If the browser script is supporting params being passed the same options should "generally" be passable via argv in shell should they not?

@Oldiesmann
Copy link
Contributor

Oldiesmann commented May 10, 2024

The code in question is simply checking whether someone is trying to be funny by sticking a URL in the query string. Completely irrelevant when upgrading via CLI.

		// It seems that sticking a URL after the query string is mighty common, well, it's evil - don't.
		if (strpos($_SERVER['QUERY_STRING'], 'http') === 0) {
			Utils::sendHttpStatus(400);

			die;
		}

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>
@Sesquipedalian
Copy link
Member Author

New problem this time.

PHP Fatal error:  Uncaught TypeError: strpos(): Argument #1 ($haystack) must be of type string, false given in /.../Sources/QueryString.php:70
Stack trace:
#0 /,,,/Sources/QueryString.php(70): strpos()
#1 /.../upgrade.php(848): SMF\QueryString::cleanRequest()
#2 /.../upgrade.php(241): loadEssentialData()
#3 {main}
  thrown in /.../Sources/QueryString.php on line 70

The problem is that $_SERVER['QUERY_STRING'] is never set when running via CLI (since there is no query string as we're running the file directly through the PHP interpreter). Best option would be to just check whether we're running from the command line and skip that check if so. I'm not sure why I didn't run into this last time, unless I just wasn't paying attention.

It's strange that I don't receive any such errors in my tests. But at any rate, try now.

@Oldiesmann
Copy link
Contributor

Oldiesmann commented May 11, 2024

Latest attempt...

PHP Deprecated:  pg_version(): Automatic fetching of PostgreSQL connection is deprecated in /.../Sources/Db/APIs/PostgreSQL.php on line 1113
 * Updating Settings.php... Successful.
 *  done.
 Successful.
 *  +++  Adding a new column "version" to messages table done.
 +++  Adding a new column "version" to personal_messages table done.
 Successful.
 *  +++  Add duration, rrule, rdates, and exdates columns to calendar table done.
 +++  Set duration and rrule values and change end_datePHP Fatal error:  Uncaught TypeError: SMF\Db\APIs\PostgreSQL::fetch_assoc(): Argument #1 ($result) must be of type object, false given, called in /.../upgrade.php(2467) : eval()'d code on line 9 and defined in /.../Sources/Db/APIs/PostgreSQL.php:350
Stack trace:
#0 /.../upgrade.php(2467) : eval()'d code(9): SMF\Db\APIs\PostgreSQL->fetch_assoc()
#1 /.../upgrade.php(2467): eval()
#2 /.../upgrade.php(1884): parse_sql()
#3 /.../upgrade.php(371): DatabaseChanges()
#4 {main}
  thrown in /.../Sources/Db/APIs/PostgreSQL.php on line 350

@Sesquipedalian
Copy link
Member Author

Er, are you sure you tested with the correct commit checked out? The connection is provided on line 1113.

@Oldiesmann
Copy link
Contributor

I'll double check. I may have forgotten to update that file.

@Oldiesmann
Copy link
Contributor

That got rid of the deprecation error. It still dies with the other error though.

@Oldiesmann
Copy link
Contributor

Further digging reveals that the query beginning on line 129 of upgrade_3-0_PostgreSQL.sql is failing because there is no longer an end_time column in the calendar table (since that was dropped during a previous run of the upgrader). We either need to check whether that column exists first or add a setting indicating we've already completed that part previously and then skip the event conversion stuff if so.

@Sesquipedalian
Copy link
Member Author

Hopefully the latest commit will finish it.

@Oldiesmann
Copy link
Contributor

Hopefully the latest commit will finish it.

Unfortunately not.

 +++  Set duration and rrule values and change end_datePHP Parse error:  Unclosed '{' on line 2 in /.../upgrade.php(2467) : eval()'d code on line 46

@Oldiesmann
Copy link
Contributor

It's because the "{" in the if statement on line 128 of upgrade_3-0_PostgreSQL.sql is never closed. I'm just not sure where to add it.

@Sesquipedalian
Copy link
Member Author

Bleh. I must have made the same mistake there that I made earlier in the MySQL file. Not sure why I didn't catch it when I checked after finding the one in the MySQL file.

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

Everything completed successfully this time, though it did show Clean up "Cleanup" twice for some reason.

@Sesquipedalian Sesquipedalian force-pushed the more_upgrader_fixes branch 4 times, most recently from d277cdc to b5cee38 Compare May 13, 2024 07:06
@Sesquipedalian Sesquipedalian merged commit 9b2a6b0 into SimpleMachines:release-3.0 May 13, 2024
6 checks passed
@Sesquipedalian Sesquipedalian deleted the more_upgrader_fixes branch May 13, 2024 16:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants