joomla
[5.4] Fix MySql 8 error "Illegal argument to a regular expression." in banners model
Pull Request for Issue #41183 .
Summary of Changes
The banners module supports using banners by tag search or keywords. When the banners model is building the query for it, there is a reg-expression to search for words. This regex seems not be compatible with MySql version 8.0.4 and newer.
The SQL query, which is created, is something like: Where metakey regexp('[[:<:]]Mo[[:>:]]')
"Mo" is the keyword to search for.
Executing it on MySql 8.0.4 or any newer version, it throws the error "Illegal argument to a regular expression.".
I think the correct reg-expression must be: Where metakey regexp('\\bMo\\b')
In the MySql manual https://dev.mysql.com/doc/refman/8.0/en/regexp.html there is the explanation about the switch from the Henry Spencer's implementation to the ICU compatible implementation:
MySQL implements regular expression support using International Components for Unicode (ICU), which provides full Unicode support and is multibyte safe. (Prior to MySQL 8.0.4, MySQL used Henry Spencer's implementation of regular expressions, which operates in byte-wise fashion and is not multibyte safe.
Further information in the web can for example be found here: https://bugs.mysql.com/bug.php?id=106504 https://stackoverflow.com/questions/59998409/error-code-3685-illegal-argument-to-a-regular-expression
System information: Joomla: The site which had/has those issues had version 5 and is in the meantime migrated to 6 (where the errors still appear). DB type: mysql DB version: 8.0.42 PHP-Version: 8.4.7
Background information:
I use a custom component which calls in a view $this->getDocument()->setMetaData('keywords', $currentKeywords);
With this approach, the banners module gets those keywords in its BannersHelper method getBanners() line 45 with:
$keywords = explode(',', $app->getDocument()->getMetaData('keywords'));
With this Pull Request the reg-expression will be surrounded with a db type check, which makes sure the correct expression is used.
In the same part of the banners model there is
- another fix for a null ref deprecation warning from PHP 8:
Deprecated
: str_starts_with(): Passing null to parameter #2 ($needle) of type string is deprecated in
E:\joomla\github\joomla-cms_5.4\components\com_banners\src\Model\BannersModel.php
on line
219
- a fix for a PHP 8 warning:
Warning
: foreach() argument must be of type array|object, false given in
E:\joomla\github\joomla-cms_5.4\components\com_banners\src\Model\BannersModel.php
on line
275
Testing Instructions
Honestly, I don't know how I can provide the keywords with the Joomla standard components. So the easiest way to reproduce is:
- Joomla should use MySql version 8.0.4 or newer.
- Under System > Global configuration, in Tab "Server" set Error Reporting = Maximum.
- In the banners component configuration, in Tab "Client" make sure the "Keyword Prefix" is empty.
- Under Components > Banners, create a new Client:
- Name = Client 1
- Contact Name = Xy
- Then create a new banner:
- Title = Banner 1
- Type = Image
- Image = Choose an image
- In Tab "Banner Details":
- set Client = Client 1
- In Tab "Publishing":
- set "Keywords" = Mo
- set "Use Own Prefix" = Yes
- set "Keyword Prefix" = Mo
- Create Banner 2, with the same settings as mentioned for Banner 1. This can be done with "Save as Copy" and adapt name, and set status to published.
- Create Banner 3, with the same settings as mentioned for Banner 1, but
- set "Keywords" = Di
- set "Keyword Prefix" = Di
- By default all three banners are assigned to the Category = Uncategorised, which is fine.
- Under System > Site Modules, create new Banners module:
- Title = Banners by keyword
- Client = Client 1
- Category = Uncategorised
- Set "Select by Keyword" = Yes
- Choose a proper position = Below Top
- In Tab "Menu Assignment" it should already be:
- Module Assignment = On all pages
- Now edit the module_banners in BannersHelper ([root]\modules\mod_banners\src\Helper\BannersHelper.php):
In the method getBanners() at line 45, replace
$keywords = explode(',', $app->getDocument()->getMetaData('keywords'));with$keywords = ['Mo']; - Clear cache: System > Clear cache, click "Delete All"
- Open the start page of the site, and check the output.
Actual result BEFORE applying this Pull Request
- No banners are visualized.
- The following three messages are displayed instead:
Deprecated
: str_starts_with(): Passing null to parameter #2 ($needle) of type string is deprecated in
E:\joomla\github\joomla-cms_5.4\components\com_banners\src\Model\BannersModel.php
on line
219
Warning
: foreach() argument must be of type array|object, false given in
E:\joomla\github\joomla-cms_5.4\components\com_banners\src\Model\BannersModel.php
on line
275
Warning: foreach() argument must be of type array|object, false given in E:\joomla\github\joomla-cms_5.4\modules\mod_banners\tmpl\default.php on line 27
Expected result AFTER applying this Pull Request
- Banner 1 and Banner 2 are displayed (banners having "Keywords": Mo), each with its assigned image. Right klick the image and click "inspect", then you can see in the "alt" attribute from which banner it is. (Banner 3 is not displayed because it belongs to another keyword)
- None of the mentioned three messages are displayed.
Link to documentations
Please select:
-
[ ] Documentation link for docs.joomla.org:
-
[x] No documentation changes for docs.joomla.org needed
-
[ ] Pull Request link for manual.joomla.org:
-
[x] No documentation changes for manual.joomla.org needed
Good point, I didn't saw it. Yes, indeed, the word boundary part is the same. Although the related issue 41183 mentioned MariaDb as well and there is a discussion about a more generic/clean implementation. It seems that my PR is more like a quick fix.
@richard67 Since you were involved in the discussion of issue 41183, what do you think: Is my quick fix acceptable? I don't believe I'm capable of implementing the clean approach of extending the database providers.
@richard67 Since you were involved in the discussion of issue 41183, what do you think: Is my quick fix acceptable? I don't believe I'm capable of implementing the clean approach of extending the database providers.
@beni71 If you accept my code change suggestions (can be done with buttons on GitHub), the PR is ok and can be changed from draft to ready for review.
I have now a general question: It seems that this PR is based on an old way to pass keywords to the banners module, throught the document, which was removed from the Joomla components with #25258. Joomla components no longer support it. Does it mean that it is deprecated anyway? If so, I will close the PR.
To be honest: I have no idea if it is deprecated. Let's see if other comments come it.
The reason why I haven't made a PR yet for fixing the issue was because I was not able to find out how and when keywords were used in the banner module.
As far as I know keywords of articles are used for the related articles module.
@beni71 - first off, thank you for your contribution, unfortunately, I am not getting exactly what you are intending: (just a small, perhaps obvious instruction update , you need to set a position for the module ;))
I set mine to Below_Top (and then Main_Bottom to see if that would change things... only difference is that with Main_Bottom I saw the Chrome with the title: Site Module Banner which isn't present in Below_top).
BEFORE CONDITION, I only match 2 out of 4 of your conditions,
-
I get "No banners are visualized." - I do NOT have the error message: The getItems method has catched the error "Illegal argument to a regular expression." but returned
-
The PHP 8 deprecation warning "...Passing null to parameter #2 ($needle) of type string is deprecated..." is displayed.
I get:
Deprecated : str_starts_with(): Passing null to parameter #2 ($needle) of type string is deprecated in /home/lights/public_html/_j540/components/com_banners/src/Model/BannersModel.php on line 219
which seems to be a match!
- The PHP 8 warning "...foreach() argument must be of type array|object, false given..." is NOT displayed.
AFTER CONDITION: no more error message BUT no banner displayed. NO errors in Console in either case ;)
Hope that helps you!
@beni71 There is no need to update your branch when it is shown as not up to date. It only needs to be done when there are conflicts shown. No problem now, but if you PR would already have successful human tests, the test counter would be reset by the branch update, and that could cause us missing it when searching for PRs which have 2 successful tests for setting them RTC (ready to commit).
As said, not a problem right now, I just wanted to explain for future use.
@exlemor Thank you for your test and feedback. You are right, the reproduction steps were not clear enough and some steps were missing. I have now started from a clean Joomla 5.4 (from the branch) and have revised the instructions. It would be great if you could test it again.
I have tested this item :white_check_mark: successfully on 0fc51d182a827d8051d832e542ffb962a10bc5d2
I have tested this successfully - thanks for the updated testing instructions @beni71 - FYI, I only got the 1st error message but the AFTER condition was spot on match, no error messages, correct Banner 1 and Banner 2 in the Alt descriptions etc etc
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/46547.
@beni71 - I'm going to get in trouble I fear, as I was cleaning up my test environment after testing this PR, I realized that that server is running MariaDB 10.11.14-MariaDB (and not technically exactly MySQL 8.0.4) (even though I understand MariaDB is supposed to be drop-in replacement, I'm told there are small differences), that may be why I only got 1 error message and not all 3 of them.
@exlemor Regarding MariaDB see my comment (hidden review comment) https://github.com/joomla/joomla-cms/pull/46547#discussion_r2598853463 :
MySQL changed from the old Henry Spencer syntax to ICU (which uses PCRE) with version 8.0.4.
MariaDB changed from Henry Spencer syntax to PCRE with version 10.0.5. The old syntax is still supported unless the compatibility mode is switched off.
I have tested this item :white_check_mark: successfully on 0fc51d182a827d8051d832e542ffb962a10bc5d2
I followed the instruction and could see banner 1 and " after applying the patch
Database Type: mysql
Database Version: 9.1.0
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/46547.
RTC
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/46547.
✅ Final test before merge with JBT (which is using PHP 8.4.15)
- Started with MySQL 8.1.0 using MySQLi database driver, used the test instructions :+1:, no banners seen, but the one deprecated and the two warnings
- Patch applied with
gh pr checkout 46547, both 'Mo' banners are visible and there are no deprecation notices or warnings. - Switched to MySQL (PDO) and returned before PR with
git switch -, repeated step 1. seen the errors- Applied PR again with
git switch -and see it working
- Applied PR again with
- Switched to MariaDB 10.6.23 with MySQLi database driver, seen only the deprecation and both banners are displayed, but to small
- Applied PR: no more deprecation message and both banners are displayed in the correct size
- Switched to MariaDB 10.6.23 with PDO driver, seen only the deprecation and both banners are displayed, but to small
- Applied PR: no more deprecation message and both banners are displayed in the correct size
- Switched to PostgreSQL 15.8 with PDO driver, only the deprecation message is shown, no banners
- Applied PR: no more deprecation message and both banners are displayed
- Adopted test instructions in using UTF8 characters and German umlaut: 'Client 1 Ä 😃', 'Banner 1 ü ⏰', Keywords 'Mo ö ℹ️' and 'Di ß 🍕'
- Tested MySQL using MySQLi database driver, no errors before applying PR and both banners are displayed, however
- Applied PR: still no errors and both banners are displayed
Thank you @beni71 for your contribution. Thank you @brianteeman and @richard67 for support. Thank you @exlemor and @ThomasFinnern for testing.