Skip to content

in updates XML always resulted in an error#38121

Merged
HLeithner merged 3 commits intojoomla:4.2-devfrom
nikosdion:fix/38117-dbsupport-updates
Jun 27, 2022
Merged

in updates XML always resulted in an error#38121
HLeithner merged 3 commits intojoomla:4.2-devfrom
nikosdion:fix/38117-dbsupport-updates

Conversation

@nikosdion
Copy link
Copy Markdown
Contributor

Pull Request for Issue #38117 .

Summary of Changes

Updates \Joomla\CMS\Updater\Adapter\ExtensionAdapter to support the <supported_databases> tag in XML update files correctly.

Moreover, fixes the message when there is no supported database to display the DB server technology type, not the DB connector type (therefore mysqli and pdomysql / mysql will all be referred to as “MySQL”, whereas the same connectors on a MariaDB server will be referred to as “MariaDB”).

Testing Instructions

Actual result BEFORE applying this Pull Request

A bunch of misleading warnings:

  • For the extension Social Magick version 1.0.2 is available, but your current database MySQL (PDO) is not supported anymore.
  • For the extension Social Magick version 1.0.1 is available, but your current database MySQL (PDO) is not supported anymore.
  • For the extension Social Magick version 1.0.0 is available, but your current database MySQL (PDO) is not supported anymore.

Expected result AFTER applying this Pull Request

Update found, no warnings.

Documentation Changes Required

None

Translation impact

Added six new translation strings: JLIB_DB_SERVER_TYPE_MARIADB, JLIB_DB_SERVER_TYPE_MYSQL, JLIB_DB_SERVER_TYPE_POSTGRESQL, JLIB_DB_SERVER_TYPE_ORACLE, JLIB_DB_SERVER_TYPE_SQLITE, JLIB_DB_SERVER_TYPE_MSSQL

However, these strings are product names and do not need to be translated; they just need to be copied verbatim into language files by the translation teams.

@joomla-cms-bot joomla-cms-bot added Language Change This is for Translators PR-4.2-dev labels Jun 22, 2022
@richard67
Copy link
Copy Markdown
Member

However, these strings are product names and do not need to be translated; they just need to be copied verbatim into language files by the translation teams.

In most cases this might be right, but in some languages like e.g. Chinese there might be transcripts of product names in their writing system, so I think it's good to have them translatable.

@nikosdion
Copy link
Copy Markdown
Contributor Author

@richard67 They are language strings already, as I said. If a language needs to transliterate the product names, they can.

@richard67
Copy link
Copy Markdown
Member

@richard67 They are language strings already, as I said. If a language needs to transliterate the product names, they can.

@nikosdion Yes, I understood that. I only wanted to say why I think this PR is really useful. Am preparing for tests now. Will create some own updatesite for that extension and modify it to use that so I can play around a bit with the supported_databases values.

@nikosdion
Copy link
Copy Markdown
Contributor Author

@richard67 Oh! Sorry, I misunderstood what you wrote before. All clear now :)

@richard67
Copy link
Copy Markdown
Member

Unfortunately the extension linked in the description doesn't have 4.2 yet in the targetplatform tag of the update site https://raw.githubusercontent.com/lucid-fox/social-magick/main/update/socialmagick.xml , so on a 4.2-dev the test will not work as described with that extension. So I have to create an own update site anyway.

@brianteeman
Copy link
Copy Markdown
Contributor

So I have to create an own update site anyway.

or modify the version number of your install. One file to edit. Very quick

@richard67
Copy link
Copy Markdown
Member

So I have to create an own update site anyway.

or modify the version number of your install. One file to edit. Very quick

@brianteeman For that purpose only yes, but I also want to play with the supported_databases values of the update site.

@nikosdion
Copy link
Copy Markdown
Contributor Author

@richard67 You can download the update XML and host it locally, amending the supported platforms. Then edit your DB to point its update site to your local file and Bob's your uncle.

@richard67
Copy link
Copy Markdown
Member

richard67 commented Jun 26, 2022

@richard67 You can download the update XML and host it locally, amending the supported platforms. Then edit your DB to point its update site to your local file and Bob's your uncle.

@nikosdion Yes, I'm doing it that way, change the URL of the update site in the db and also in the manifest XML of the plugin.

What I found out up to now is that your PR works as described, but I did not get the version check work up to now here

$dbMatch = version_compare($dbVersion, $minimumVersion, '>=');
.

I had a mysql="9.0.0" in the supported_databases for the 1.0.2 version of the plugin, but I was offered the update.

When I removed the "mysql" completely with <supported_databases oracle="10.1" /> for plugin version 1.0.2, then I got the expected message telling that my database type "MySQL" is not supported anymore.

Am investigating what the reason could be. Will let you know soon.

@richard67
Copy link
Copy Markdown
Member

richard67 commented Jun 26, 2022

@nikosdion The problem is that the array keys of $supportedDbs are in uppercase for whatever reason, while $dbType is lowercase. You catch that well in line 159, but in line 161 the $minimumVersion = $supportedDbs[$dbType]; fails.

If we can rely on the keys always being uppercase we could just change that to $minimumVersion = $supportedDbs[strtoupper($dbType)];, but maybe it would be better just to map or copy the $supportedDbs so it has lowercase keys?

P.S. What we should not to is to change $dbType to uppercase.

@richard67
Copy link
Copy Markdown
Member

Hmm, it is an XML element attribute ... if we can rely on it always being made uppercase by the XML parser, we can just use $supportedDbs[strtoupper($dbType)]; in line 161.

@richard67
Copy link
Copy Markdown
Member

richard67 commented Jun 26, 2022

@nikosdion In addition to my 2 previous review comments, I found that it needs to change the error message also here

from Text::_($db->name), to Text::_('JLIB_DB_SERVER_TYPE_' . $dbTypeUcase), if you follow my suggestions, or to Text::_('JLIB_DB_SERVER_TYPE_' . $dbType), otherwise.

@richard67
Copy link
Copy Markdown
Member

For testing I have created a few update sites:

Just change the location column of the record with name 'Social Magick Updates' in the #__update_sites table to the mentioned URL, and to be sure it survives an update site rebuild change it also in line 637 of file "plugins/system/socialmagick/socialmagick.xml" of your testing site.

@richard67
Copy link
Copy Markdown
Member

Another thing I found is that there is very similar functionality here https://github.com/joomla/joomla-cms/blob/4.2-dev/libraries/src/Updater/Update.php#L357-L388 . I'm not sure, but I think that's used for the core update. It would make sense to use the new language strings also there when this PR here has been merged. I could make a follow up or for that in this case.

Finally, as the language strings are only used for reporting the current database type on which Joomla is running, and not the supported one from the update site, and Joomla 4 runs only on MySQL or PostgreSQL, we currently do not really need the language strings for the other database types (Oracle, MS SQL, SQLite). But as the drivers support to connect with them, I think it is good to have them for future use at other places. But that's just my opinion, others might disagree.

@richard67
Copy link
Copy Markdown
Member

@nikosdion If you want I can make a PR for you in your repo to apply my above suggestions.

@nikosdion
Copy link
Copy Markdown
Contributor Author

Yup, please do a PR.

As for the lang strings, best to have all keys than ending up with an untranslated string 5 years down the line if, say, we decide that a site can have multiple DB connections and people write integration plugins which talk to the secondary database which might be of a different technology than what is supported for the primary database.

@richard67
Copy link
Copy Markdown
Member

Yup, please do a PR.

@nikosdion Done, see https://github.com/nikosdion/joomla-cms/pull/10 .

Regarding the language strings I fully agree with you. I just mentioned it in case others may disagree.

…upport-updates-mod1

[CMS PR 38121] Use uppercase for array key and language string key
@nikosdion
Copy link
Copy Markdown
Contributor Author

Thank you! I merged your changes.

@richard67
Copy link
Copy Markdown
Member

I have tested this item ✅ successfully on a4be380

I have tested as follows.

I have created 3 update sites for 3 tests:

  1. https://test5.richard-fath.de/test_pr-38121.xml for testing the PR as currently described in the instructions on a 4.2-dev.
  2. https://test5.richard-fath.de/test_pr-38121_test-2.xml for having no supported database type.
  3. https://test5.richard-fath.de/test_pr-38121_test-3.xml for having a supported database type but with unsupported version.

Then I have installed the plugin linked in the description.

Then I have updated the location column of the record with name 'Social Magick Updates' in the #__update_sites table to the URL of the particular update site and then have used the "Check For Updates" button.

I've done this for all 3 sites without and with the PR applied.

Results will come with screenshot in my next comment.

Other testers please follow my inscrutions above.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/38121.

@richard67
Copy link
Copy Markdown
Member

richard67 commented Jun 26, 2022

Here the details to my test results for the 3 update sites I've mentioned in the comment to my test.

Without the PR applied, I get with all 3 update sites the same result as described in the issue:
test-pr-38121_before

With the PR applied, everything is as expected:

When using the first update site, an update is found:
test-pr-38121_after-1

When using the second update site, I get a message that the database is not supported anymore:
test-pr-38121_after-2

When using the third update site, I get a message that the database version is not supported anymore:
test-pr-38121_after-3

@Quy
Copy link
Copy Markdown
Contributor

Quy commented Jun 27, 2022

I have tested this item ✅ successfully on a4be380


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/38121.

@Quy Quy changed the title <supported_databases> in updates XML always resulted in an error in updates XML always resulted in an error Jun 27, 2022
@Quy
Copy link
Copy Markdown
Contributor

Quy commented Jun 27, 2022

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/38121.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jun 27, 2022
@HLeithner HLeithner merged commit 0eeae2d into joomla:4.2-dev Jun 27, 2022
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jun 27, 2022
@HLeithner
Copy link
Copy Markdown
Member

thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Language Change This is for Translators

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants