pkp / pkp-lib

The library used by PKP's applications OJS, OMP and OPS, open source software for scholarly publishing.
https://pkp.sfu.ca
GNU General Public License v3.0
304 stars 444 forks source link

Localize issue and article cover images #1934

Closed bozana closed 7 years ago

bozana commented 7 years ago

The issue cover images are not localized any more in OJS 3.0, which is a problem for journals migrating from 2.4.x, s. for example http://forum.pkp.sfu.ca/t/update-to-ojs-3-failed/21564. Thus the issue cover images should be saved localized again. The same guilt for article cover images. The monograph cover images are not localized. If necessary, this can be changed later, as a separate issue.

bozana commented 7 years ago

PRs: pkp-lib master: https://github.com/pkp/pkp-lib/pull/1952 ojs master: https://github.com/pkp/ojs/pull/1083 omp master: https://github.com/pkp/omp/pull/346

pkp-lib ojs-stable-3_0_0: https://github.com/pkp/pkp-lib/pull/1972 ojs ojs-stable-3_0_0: https://github.com/pkp/ojs/pull/1092

bozana commented 7 years ago

@NateWr, could you take a look at this PR -- because you have already worked on that? This is just issue cover image localization, I will do the article cover image localization tomorrow...

bozana commented 7 years ago

Ah, sorry @NateWr, could you please wait with this -- I forgot to consider import/export :-(

bozana commented 7 years ago

@NateWr, the PRs above are ready for code review :-) Thanks a lot!

NateWr commented 7 years ago

Looks good @bozana! I had a couple of very minor code style questions, but feel free to merge if you disagree.

I do think that we'll probably get some UX/UI feedback at some point from people who don't want to have to switch languages to upload alternate cover images. But I'm happy with this as a good-enough UX for now.

bozana commented 7 years ago

Thanks a lot @NateWr, I'll make the suggested changes and let you know. Then I will cherry-pick it to ojs-stable-3_0_0 as well...

NateWr commented 7 years ago

@bozana I'm having some trouble with localized article images. I'm trying to update the bootstrap theme to support hte localized images: https://github.com/NateWr/bootstrap3/issues/43

However, I'm getting a Fatal Error when $article->getLocalizedCoverImage() is called on an article with an image that was uploaded before this change was in place.

[Mon Nov 28 18:59:19.233344 2016] [:error] [pid 22812] [client 192.168.0.8:59348] PHP Warning:  Declaration of SubmissionFileDAO::fromRow($row) should be compatible with PKPSubmissionFileDAO::fromRow($row, $fileImplementation) in /var/www/html/ojs/classes/article/SubmissionFileDAO.inc.php on line 23, referer: http://192.168.0.8/ojs/index.php/publicknowledge/article/view/10
[Mon Nov 28 18:59:19.235788 2016] [:error] [pid 22812] [client 192.168.0.8:59348] PHP Warning:  Cannot use a scalar value as an array in /var/www/html/ojs/lib/pkp/classes/core/DataObject.inc.php on line 133, referer: http://192.168.0.8/ojs/index.php/publicknowledge/article/view/10
[Mon Nov 28 18:59:19.235799 2016] [:error] [pid 22812] [client 192.168.0.8:59348] PHP Warning:  Cannot use a scalar value as an array in /var/www/html/ojs/lib/pkp/classes/core/DataObject.inc.php on line 133, referer: http://192.168.0.8/ojs/index.php/publicknowledge/article/view/10
[Mon Nov 28 18:59:19.235805 2016] [:error] [pid 22812] [client 192.168.0.8:59348] PHP Warning:  Cannot use a scalar value as an array in /var/www/html/ojs/lib/pkp/classes/core/DataObject.inc.php on line 133, referer: http://192.168.0.8/ojs/index.php/publicknowledge/article/view/10
[Mon Nov 28 18:59:19.235813 2016] [:error] [pid 22812] [client 192.168.0.8:59348] PHP Warning:  Cannot use a scalar value as an array in /var/www/html/ojs/lib/pkp/classes/core/DataObject.inc.php on line 133, referer: http://192.168.0.8/ojs/index.php/publicknowledge/article/view/10
[Mon Nov 28 18:59:19.235818 2016] [:error] [pid 22812] [client 192.168.0.8:59348] PHP Warning:  Cannot use a scalar value as an array in /var/www/html/ojs/lib/pkp/classes/core/DataObject.inc.php on line 133, referer: http://192.168.0.8/ojs/index.php/publicknowledge/article/view/10
[Mon Nov 28 18:59:19.235822 2016] [:error] [pid 22812] [client 192.168.0.8:59348] PHP Warning:  Cannot use a scalar value as an array in /var/www/html/ojs/lib/pkp/classes/core/DataObject.inc.php on line 133, referer: http://192.168.0.8/ojs/index.php/publicknowledge/article/view/10
[Mon Nov 28 18:59:19.260929 2016] [:error] [pid 22812] [client 192.168.0.8:59348] PHP Warning:  array_keys() expects parameter 1 to be array, string given in /var/www/html/ojs/lib/pkp/classes/submission/Submission.inc.php on line 153, referer: http://192.168.0.8/ojs/index.php/publicknowledge/article/view/10
[Mon Nov 28 18:59:19.260946 2016] [:error] [pid 22812] [client 192.168.0.8:59348] PHP Warning:  array_shift() expects parameter 1 to be array, null given in /var/www/html/ojs/lib/pkp/classes/submission/Submission.inc.php on line 154, referer: http://192.168.0.8/ojs/index.php/publicknowledge/article/view/10
[Mon Nov 28 18:59:19.260992 2016] [:error] [pid 22812] [client 192.168.0.8:59348] PHP Fatal error:  Uncaught Error: Cannot return string offsets by reference in /var/www/html/ojs/lib/pkp/classes/submission/Submission.inc.php:154\nStack trace:\n#0 /var/www/html/ojs/classes/article/Article.inc.php(253): Submission->getLocalizedData('coverImage')\n#1 /var/www/html/ojs/cache/t_compile/e26d2de610226743d9feff244fb14ef53700227f^%%A0^A0A^A0AA4E57%%article_summary.tpl.php(11): Article->getLocalizedCoverImage()\n#2 /var/www/html/ojs/lib/pkp/lib/vendor/smarty/smarty/libs/Smarty.class.php(1870): include('/var/www/html/o...')\n#3 /var/www/html/ojs/lib/pkp/classes/template/PKPTemplateManager.inc.php(359): Smarty->_smarty_include(Array)\n#4 /var/www/html/ojs/cache/t_compile/e26d2de610226743d9feff244fb14ef53700227f^%%DD^DDF^DDF03D78%%issue_toc.tpl.php(122): PKPTemplateManager->_smarty_include(Array)\n#5 /var/www/html/ojs/lib/pkp/lib/vendor/smarty/smarty/libs/Smarty.class.php(1870): include('/var/www/html/o...')\n#6 /var/www/html/ojs/lib/pkp/classes/template/PKPTemplateManager.inc.php(359): Smarty->_smarty_include(Array)\n#7 /var/ww in /var/www/html/ojs/lib/pkp/classes/submission/Submission.inc.php on line 154, referer: http://192.168.0.8/ojs/index.php/publicknowledge/article/view/10

I also found that I don't see the existing image in the article metadata. And if I try to upload a new image it doesn't work.

The cover images work fine on an article that's never had an image uploaded before. Do I need to run a database upgrade somewhere?

NateWr commented 7 years ago

I was able to upgrade the database, though I had to manually set the core version to 3.0.0 and then upgrade to 3.0.1 (php tools/upgrade.php upgrade). However, the old cover image data doesn't seem to have been upgraded.

Here's a database entry for two submissions. ID 9 has a cover image uploaded before the changes and ID 10 has one uploaded after the changes:

+---------------+--------+-------------------+----------------------------------------------------+--------------+
| submission_id | locale | setting_name      | SUBSTRING(setting_value,1,50)                      | setting_type |
+---------------+--------+-------------------+----------------------------------------------------+--------------+
|             9 |        | coverImage        | article_9_cover.jpg                                | string       |
|             9 |        | coverImageAltText | Alt text here                                      | string       |
|            10 |        | coverImage        | a:2:{s:5:"en_US";s:26:"article_10_cover_en_US.jpg" | object       |
|            10 |        | coverImageAltText |                                                    | string       |
|            10 | en_US  | coverImage        | article_10_cover_en_US.jpg                         | string       |
|            10 | fr_CA  | coverImage        | article_10_cover_fr_CA.jpg                         | string       |
+---------------+--------+-------------------+----------------------------------------------------+--------------+
6 rows in set (0.00 sec)

Did I miss an upgrade routine somewhere?

asmecher commented 7 years ago

@bozana is probably not going to be available for a few days -- reopening the issue for a bit.

bozana commented 7 years ago

@NateWr, this upgrade script change should actually consider the old and unlocalized cover images: https://github.com/bozana/ojs/commit/493d18dc2cece5c78cf39dd6caf1a6e63db8a221#diff-694ad44b2764303882d41942f9516d5e and https://github.com/pkp/ojs/blob/master/dbscripts/xml/upgrade.xml#L114. Thus, I wonder why this do not change the submission 9 :-O And I wonder since when is the other entry with the array value for the coverImage for submission 10 there -- because also in the OJS 2.4.8 it is different i.e. the same as in 3.0 just that the setting name is different :-O Could you maybe also send me your DB dump? -- I could then take a look... THANKS!!!

bozana commented 7 years ago

These issues should be considered too: -- remove strange old cover images with array values in the DB - from 3.alpha or 3.beta? -- remove empty 3.0 cover images -- remove the old 2.4.x cover images, for which a new cover image exists

PR: https://github.com/pkp/ojs/pull/1128

bozana commented 7 years ago

@NateWr, could you code review and test the PR above? Just change the version to 3.0.0.0 before doing the DB update... THANKS!

NateWr commented 7 years ago

@bozana I ran into an error when running the upgrade.

[data: dbscripts/xml/upgrade/3.0.1_update.xml]
ERROR: Upgrade failed: DB: Duplicate entry 'googlescholarplugin-6-enabled' for key 'plugin_settings_pkey'

I set the version to 3.0.0.0 in the database, then went to /tools and ran php upgrade.php upgrade. Is it a problem since I maybe already upgrade this database in teh past?

bozana commented 7 years ago

Ah, yes, I also always have to comment out this line: https://github.com/pkp/ojs/blob/master/dbscripts/xml/upgrade.xml#L110 But that is not the right solution :-P @asmecher, is there a way to say: "If not exist, insert..." here: https://github.com/pkp/ojs/blob/master/dbscripts/xml/upgrade/3.0.1_update.xml#L15-L16, to make it idempotent? Something like MySQL INSERT IGNORE, REPLACE, or INSERT … ON DUPLICATE KEY UPDATE. Hmmm... I'll take a look...

NateWr commented 7 years ago

Great! The upgrade worked this time and the cover image issue is fixed. :+1:

bozana commented 7 years ago

Great, thanks @NateWr! I will then merge it... @asmecher, are those SQLs fine with PostgreSQL?

@asmecher, how about one of the following SQL statements, for solving the idempotent issue above:

INSERT INTO plugin_settings (plugin_name, setting_name, setting_value, setting_type, context_id) SELECT 'googlescholarplugin', 'enabled', '1', 'bool', j.journal_id FROM journals j
WHERE NOT EXISTS (SELECT ps.plugin_name FROM plugin_settings ps WHERE ps.plugin_name = 'googlescholarplugin' AND ps.context_id = j.journal_id)
INSERT INTO plugin_settings (plugin_name, setting_name, setting_value, setting_type, context_id) SELECT 'googlescholarplugin', 'enabled', '1', 'bool', j.journal_id FROM journals j
WHERE 'googlescholarplugin' NOT IN (SELECT ps.plugin_name FROM plugin_settings ps WHERE ps.plugin_name = 'googlescholarplugin' AND ps.context_id = j.journal_id)

?

asmecher commented 7 years ago

@bozana, yes, both those queries run OK for me.

bozana commented 7 years ago

PR for the plugin settings insert statement: master: https://github.com/pkp/ojs/pull/1129 ojs-stable-3_0_1: https://github.com/pkp/ojs/pull/1130

bozana commented 7 years ago

Everything merged, thus closing...