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

Fixes some upgrader issues #7077

Merged
merged 3 commits into from
Sep 17, 2021

Conversation

jdarwood007
Copy link
Member

@jdarwood007 jdarwood007 commented Sep 13, 2021

Fixes #7075 and #7066 by adding the needed variable into the headers
Fixes #7070 by using the select method instead. If this introduces issues because we no longer check the client we can reintroduce that.
Fixes #7060 by addressing that we need to use the function rather than string for the db version.
Fixes #7078 by making the changes the same in the installer
This will conflict with #7072 if it merges first. I changed the eval call to a closure instead. Easier to read and Syntax highlighting works in editors.
I also added a sanity check to ensure the function exists.

Fixes SimpleMachines#7075 by adding the needed variable into the headers
Fixes SimpleMachines#7070 by using the select method instead.  If this introduces issues because we no longer check the client we can reintroduce that.
This will conflict with SimpleMachines#7072 if it merges first.  I changed the eval call to a closure instead.  Easier to read and Syntax highlighting works in editors.
I also added a sanity check to ensure the function exists.
Copy link
Contributor

@sbulen sbulen left a comment

Choose a reason for hiding this comment

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

Good point made over on #7078 - we should apply the same version check changes to the installer.

Code looks good, though I could not test #7070 without a MariaDB.

I was able to confirm #7066 and #7075 are both addressed. I was able to run a variety of upgrades, via CLI & via browser, with & without backups, from 1.1 & 2.0, on small DBs and a larger forum (a copy of my prod forum). All went perfectly.

I needed to test this alongside #7076 since some of my test DBs have no PMs. So they work well together.

@sbulen
Copy link
Contributor

sbulen commented Sep 14, 2021

I have confirmed that this PR addresses #7060 ...

Thought to be honest, I'm not sure why yet... 🙄

That broken logic highlighted by #7060 is clearly not invoked under normal circumstances - it has never worked.

@@ -2541,7 +2548,7 @@ function checkChange(&$change)
// Attempt to find a database_version.
if (empty($database_version))
{
$database_version = $databases[$db_type]['version_check'];
$database_version = $databases[$db_type]['version_check']();
Copy link
Member Author

Choose a reason for hiding this comment

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

@sbulen This change fixes #7060

@jdarwood007
Copy link
Member Author

@sbulen Per #7078 i've synced up the installer. I don't have a working PG instance to test on right now to confirm I didn't break it.
I did some cleanup and removed the UTF-8 code in the installer, since we are requiring UTF-8 in 2.1 anyways, there isn't an option to not use it. Left the sanity checks though.

@jdarwood007
Copy link
Member Author

FYI, I have MariaDB so I can confirm the MariaDB specific changes are working as expected.

@sbulen
Copy link
Contributor

sbulen commented Sep 14, 2021

I hope to do another set of tests tonight.

I'd like to get this & #7076 in sooner rather than later.

I have a pg test environment & will do a 2.0=>2.1 upgrade test there, also.

@illori
Copy link
Contributor

illori commented Sep 15, 2021

this does fix the MariaDB issue, but i dont know if related to this or something else i am getting this error when running upgrade.php

Notice: Undefined index: normalize in C:\wamp64\www\github2.1new\Sources\Subs.php on line 7598

Fatal error: Uncaught Error: Function name must be a string in C:\wamp64\www\github2.1new\Sources\Subs.php:7598
Stack trace:
#0 C:\wamp64\www\github2.1new\Sources\Subs.php(5769): iri_to_url('https://www.sim...')
#1 C:\wamp64\www\github2.1new\upgrade.php(1840): fetch_web_data('https://www.sim...')
#2 C:\wamp64\www\github2.1new\upgrade.php(1757): cli_scheduled_fetchSMfiles()
#3 C:\wamp64\www\github2.1new\upgrade.php(2869): DeleteUpgrade()
#4 C:\wamp64\www\github2.1new\upgrade.php(3429): ConvertUtf8()
#5 C:\wamp64\www\github2.1new\upgrade.php(357): serialize_to_json()
#6 {main}
thrown in C:\wamp64\www\github2.1new\Sources\Subs.php on line 7598

@jdarwood007
Copy link
Member Author

jdarwood007 commented Sep 15, 2021

As it relates to normalize and the iri_to_url, I would say that would be @Sesquipedalian who recently merged a fix for that.

@Sesquipedalian
Copy link
Member

As it relates to normalize and the iri_to_url, I would say that would be @Sesquipedalian who recently merged a fix for that.

Correct. Just do a quick merge of the latest from release-2.1 into this PR, @jdarwood007, and that error will go away.

@illori
Copy link
Contributor

illori commented Sep 16, 2021

i had done an upgrade of my files to latest before i applied the changes here to upgrade.php, i still think there is an issue that needs to be fixed.

anyone else able to duplicate the error i got?

@sbulen
Copy link
Contributor

sbulen commented Sep 16, 2021

I've been testing this alongside #7076 for the last couple of days and it looks good. Lots of variants, including migrations from yabbse, smf 1.0, 1.1, 2.0 & 2.1. Some CLI, some browser. Some php7.4, some php8. I've even done PG installs & a PG 2.0=>2.1 upgrade. All good.

I had a couple random quirky issues that I don't think were related to this PR, so they were logged separately.

I did not encounter the issues Illori encountered above. Not sure why, but I haven't...

I think this looks good & that #7076 & #7077 should go in asap so folks can test upgrades.

@illori
Copy link
Contributor

illori commented Sep 16, 2021

i ran my test via cli on windows if that makes any difference.

@jdarwood007
Copy link
Member Author

@Sesquipedalian Can you merge this? Its confirmed working, but I do try to avoid merging my own PRs.

@Sesquipedalian Sesquipedalian merged commit 186e2cc into SimpleMachines:release-2.1 Sep 17, 2021
@pr-triage pr-triage bot added the PR: merged label Sep 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants