Ojs upgrade fails on php8?

OyvindLGjesdal · September 15, 2021, 4:53pm

Description of issue or problem I’m having:
We are upgrading to version 3.3.0.9 from 3.1.1 and I’m getting an error message which I think is related to php 8. I’ve continued from a first bug which has already been resolved by pkp Can't upgrade to 3.2.x on PHP 8 due to use of get_magic_quotes_runtime · Issue #7275 · pkp/pkp-lib · GitHub

Steps I took leading up to the issue:

ran php tools/upgrade.php upgrade after merging changes in git. (php 8)

What I tried to resolve the issue:
I think the issue might be because of some of the changes listed in php in version 8:

now causes an error: …
Attempting to write to an array index of a scalar value.

If I understand the code correctly $data[$key] is sometimes a string, and if so, it causes an error when $data[$key][$locale] is attempted written to.

I’ve tested

diff --git a/classes/core/DataObject.inc.php b/classes/core/DataObject.inc.php

+                                unset($this->_data[$key]);                              
                                $this->_data[$key][$locale] = $value;

This makes the update script continue further, all the way to [migration: PKPv3_3_0UpgradeMigration]

The next error message and followup-questions will be in a reply due to character limit.

Application Version - e.g., OJS 3.1.2:

Additional information, such as screenshots and error log messages if applicable:
Relevant section with context

[data: dbscripts/xml/upgrade/3.2.0_url_path.xml]
[code: Installer Installer::setStatsEmailSettings]
PHP Fatal error:  Uncaught TypeError: Cannot access offset of type string on string in /var/www/ojs/lib/pkp/classes/core/DataObject.inc.php:133
Stack trace:
#0 /var/www/ojs/lib/pkp/classes/db/DAO.inc.php(494): DataObject->setData()
#1 /var/www/ojs/lib/pkp/classes/user/UserDAO.inc.php(239): DAO->getDataObjectSettings()
#2 /var/www/ojs/lib/pkp/classes/db/DAOResultFactory.inc.php(94): UserDAO->_returnUserFromRowWithData()
#3 /var/www/ojs/lib/pkp/classes/install/Installer.inc.php(991): DAOResultFactory->next()
#4 /var/www/ojs/lib/pkp/classes/install/Installer.inc.php(444): Installer->setStatsEmailSettings()
#5 /var/www/ojs/lib/pkp/classes/install/Installer.inc.php(251): Installer->executeAction()
#6 /var/www/ojs/lib/pkp/classes/install/Installer.inc.php(174): Installer->executeInstaller()
#7 /var/www/ojs/lib/pkp/classes/cliTool/UpgradeTool.inc.php(89): Installer->execute()
#8 /var/www/ojs/lib/pkp/classes/cliTool/UpgradeTool.inc.php(65): UpgradeTool->upgrade()
#9 /var/www/ojs/tools/upgrade.php(22): UpgradeTool->execute()
#10 {main}
  thrown in /var/www/ojs/lib/pkp/classes/core/DataObject.inc.php on line 133
PHP Fatal error:  Uncaught TypeError: pg_free_result(): Argument #1 ($result) must be of type resource, bool given in /var/www/ojs/lib/pkp/lib/vendor/adodb/adodb-php/drivers/adodb-postgres64.inc.php:1068
Stack trace:
#0 /var/www/ojs/lib/pkp/lib/vendor/adodb/adodb-php/drivers/adodb-postgres64.inc.php(1068): pg_free_result()
#1 /var/www/ojs/lib/pkp/lib/vendor/adodb/adodb-php/adodb.inc.php(4037): ADORecordSet_postgres64->_close()
#2 /var/www/ojs/lib/pkp/lib/vendor/adodb/adodb-php/adodb.inc.php(3353): ADORecordSet->Close()
#3 [internal function]: ADORecordSet->__destruct()
#4 {main}
  thrown in /var/www/ojs/lib/pkp/lib/vendor/adodb/adodb-php/drivers/adodb-postgres64.inc.php on line 1068

asmecher · September 15, 2021, 9:24pm

Hi @OyvindLGjesdal,

Continuing from #7275:

I suspect this is due to entries in the user_settings table that are expected to be multilingual (i.e. the locale column should be set for each entry) but for which there is missing locale data.

You should be able to identify the problem setting(s) by looking at which settings are localized:

SELECT DISTINCT setting_name FROM user_settings WHERE locale IS NOT NULL;

Then check which settings aren’t localized:

SELECT DISTINCT setting_name FROM user_settings WHERE locale IS NULL;

Check for a setting name that exists in both lists.

Regards,
Alec Smecher
Public Knowledge Project Team

OyvindLGjesdal · September 16, 2021, 7:13am

The queries return:

SELECT DISTINCT setting_name FROM user_settings WHERE locale IS NOT NULL;
        setting_name        
----------------------------
 givenName
 filterSection
 affiliation
 orcid
 profileImage
 signature
 apiKey
 filterEditor
 familyName
 citation-editor-hide-intro
 preferredPublicName
 apiKeyEnabled
 biography
(13 rows)

SELECT DISTINCT setting_name FROM user_settings WHERE locale IS NULL;
 setting_name 
--------------
(0 rows)

It looks like we have all the settings localized?

I also run this solution since the issue described in Error when updating from 3.1.2-4 to 3.2.0-1 with postgres - #4 by asmecher appears

ALTER TABLE journal_settings ALTER COLUMN setting_type DROP NOT NULL

Edit:

We’ve downgraded to php 7.4 and this error (and the next one) went away. Maybe a section could be added in the upgrade, that php 7.4 should be used? Updated to fix the typo in the postgres queries. After the update there seems to be three more rows in the first result.

asmecher · September 16, 2021, 6:10pm

Hi @OyvindLGjesdal,

I think there’s a typo in one of those queries above? They both appear to be the same. Could you edit your post to correct?

Thanks,
Alec Smecher
Public Knowledge Project Team

OyvindLGjesdal · September 21, 2021, 7:28am

Thanks for helping out @asmecher! I’ve edited the queries.

As I mentioned, I think we will use php 7.4 instead of 8 for the upgrade, since we get through the upgrade script without errors on php 7.4.

We can help out if it is a goal to get it running with php 8, but as I understood the old upgrades are deprecated, since there is a new library inbetween?

We will probably meet other obstacles, so we’ll be back soon with more questions.

Thanks again for helping out.
Øyvind

asmecher · September 21, 2021, 4:06pm

Hi @OyvindLGjesdal,

I think we’re close to identifying a data problem; could you try this query (in your pre-upgrade database)?

SELECT DISTINCT setting_name FROM user_settings WHERE locale IS NULL OR locale = '';

Thanks,
Alec Smecher
Public Knowledge Project Team

OyvindLGjesdal · September 21, 2021, 4:46pm

Output is:

SELECT DISTINCT setting_name FROM user_settings WHERE locale IS NULL OR locale = '';
        setting_name        
----------------------------
 apiKey
 apiKeyEnabled
 biography
 citation-editor-hide-intro
 filterEditor
 filterSection
 orcid
 profileImage
 signature
(9 rows)

And then

SELECT DISTINCT setting_name FROM user_settings WHERE locale IS NOT NULL and locale != '';
 setting_name 
--------------
 signature
 biography
 affiliation
(3 rows)

does that mean that signature and biography are duplicates?

asmecher · September 21, 2021, 5:09pm

Hi @OyvindLGjesdal,

The user_settings entries with setting_name of biography, signature, and affiliation should always have a valid entry in the locale column. So the following query…

SELECT * FROM user_settings WHERE setting_name IN ('biography', 'signature', 'affiliation') AND (locale IS NULL OR locale = '');

…should list all problematic entries.

If there are just a few, I’d recommend removing or reconciling them manually. If there are a lot, you might need to look into how to set the locale field, expecting that there will probably be index collisions that’ll prevent you from just doing a simple UPDATE.

Regards,
Alec Smecher
Public Knowledge Project Team

OyvindLGjesdal · September 22, 2021, 1:40pm

I’ve removed them with sql

DELETE FROM user_settings WHERE setting_name IN ('biography', 'signature', 'affiliation') AND (locale IS NULL OR locale = ''))

which deleted 20 rows (of which 18 had the empty string as value), but there still remains output with warnings in the php (but a bit shorter)

See output below.

We’ve tried to run this update script before the upgrade.

update user_settings SET setting_value = null WHERE user_settings.setting_value = '';
UPDATE 9144

The upgrade then didn’t show any warnings after this update. Does this change make sense?

Best regards, Øyvind

Warnings after deleting rows with locale:

[code: Installer Installer::setStatsEmailSettings]
PHP Warning:  Illegal string offset 'it_IT' in /var/www/boap-ojs/lib/pkp/classes/core/DataObject.inc.php on line 133
....

asmecher · September 22, 2021, 4:20pm

Hi @OyvindLGjesdal,

You can’t just set the setting_value to null – that’ll kind-of-but-not-really remove all biographies, signatures, and affiliations, but still leave some problematic placeholder entries where they used to be.

Have you customized the code, especially with respect to user settings? Do you have any exotic plugins installed?

Regards,
Alec Smecher
Public Knowledge Project Team

OyvindLGjesdal · September 23, 2021, 7:59am

I don’t think we have made any custom changes to the code, outside of patches. Where is a good path to look for changes to user settings?

We’ve reviewed the difference of plugins in ojs-stable-3_1_0 and our current production branch, and these are present:

extra plugins (github user/group):

addThis (pkp)
jatsTemplate (asmescher)
lucene (not in submodule, not present in ojs-stable-3_1_0)
markup (kaschioudi/ojs3-markup) Not maintained, removing in upgrade
oaijats (pkp)
shariff (ojsde)
texture (asmescher)

We noticed when comparing that we also miss these plugins from ojs-stable-3_1_0 in our production branch:

citationLookup
citationParser

Additonally we notice we don’t have any values with null in the settings_value field.

When the biography is not filled in by the user, will there be a biography user_settings row in the database (with the value null?), or should it be absent?

boap=# select count(*) FROM user_settings WHERE user_settings.setting_value = '';
 count
-------
  9151
(1 row)

boap=# select count(*) FROM user_settings WHERE user_settings.setting_value = null;
 count
-------
     0
(1 row)

Best regards,
Øyvind