-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
[3.x] [RFC] Fix SQL error 1093 in com_finder's removeOrphanNodes function with particular MySQL server versions #34818
Conversation
Thanks Richard, I just saw your PM from yesterday and I had been monitoring this on GitHub. I am afraid I have long since moved on from MySQL 5.7.9, so I no longer have any way of testing this for you. Thanks for your effort though and I hope it will benefit others and the product... |
@He-Man321 Thanks for reporting back. I'll try to find other testers but it could be hard. |
is there a reason why we don't use the samecode as in 4.0? |
@HLeithner In J3, the map table class derives from a normal table, in J4 from a nested table. Therefore in J4 the table class is used in a loop over the results from the select statement for deleting record by record. I have no idea if a backport of these changes to J3 would make sense. Maybe @Hackwar can enlighten us. |
Ok, in this case we can keep this solution here. if we get 2 tests (doesn't need to be on 5.7 but on earlier versions) we can merge this in to staging. Thanks |
@HLeithner Why earlier versions? Could be hard to find testers who still have such, e.g. 5.6, and it can be (I haven't tested yet) that this issue doesn't even happen on earlier versions. The other one I had recently fixed in J4 with the same root cause did not happen on 5.6. I think it should be sufficient to get tests with any version of MySQL AND MariaDB AND PostgreSQL if things work like described in the test "Test 2". |
Because Joomla 3.x supports mysql 5.1+ |
@HLeithner I have no idea where to get testers for that. Do you know if the CMS Release team can test that? |
I have tested this item ✅ successfully on 5cb07f4 This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/34818. |
this kind of tests should be automated |
I have tested this item ✅ successfully on 5cb07f4 This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/34818. |
RTC This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/34818. |
Thanks |
Thanks. |
Pull Request for Issue #22231 .
Important for maintainers: This PR shall not be merged up into 4.0-dev (or its changes shall not be applied when merging up) because there the issue is already solved with #26392 in the same way as here for the first part with the
SELECT
but then handled differently for theDELETE
due to different data structures of com_finder in J4.Summary of Changes
This pull request (PR) splits the
DELETE
statement on the#__finder_taxonomy
table in theremoveOrphanNodes
function, which has a subquery on the same target table, into 2 separate SQL statements, theSELECT
to get the orphan nodes' ID's and theDELETE
statement using the result of thatSELECT
in itsWHERE
clause.This solves the SQL error reported with issue #22231 , which happens only with particular versions of MySQL 5.7, for sure with 5.7.9.
Normally such buggy versions are updated by webspace providers or package providers for Linux with their regular patch cycle.
But on self managed hosts this may not happen, and so these buggy versions are still in use. So the number of installations using these versions seems to be small, but not zero.
In Joomla 4 the same issue has already been fixed with #26392 in the same way as here, i.e. the inner subquery with the join has been separated into a
SELECT
, and then the result is used to delete the orphan records. But due to other J4 changes deleting is then done in a different way.To be Done
It has to be investigated if we have such SQL statements (
DELETE
orUPDATE
statements with a subquery which contains the same target table) elsewhere, and if so, provide a similar fix as here.Request for Comment
The alternative to this PR and the investigation mentioned above would be to exclude the buggy MySQL versions from our supported versions, similar as we already do in a footnote on PHP versions for certain buggy PHP versions.
But it would take some investigation effort to find out which versions exactly have that problem, and that effort could not be small.
@wilsonge @HLeithner @zero-24 I'd like to know your opinion on this. Opinions of others are of course welcome, too.
Testing Instructions
Test 1: Reproduce the issue
Requirements: For this test it needs a MySQL database server version 5.7.9. If you have something older, try if you can reproduce the issue, too, and report back the result. It might be that the issue affects also other, older versions than 5.7.9.
You very likely won't have such old versions available. And I think you don't want to mess up your existing testing environment with replacing any present MySQL or MariaDB database server by that MySQL version.
But if you have a real or virtual computer available which can be connected to a real or virtual network so it can be reached by the webserver of your testing environment, or if you can use docker for that, you can install a MySQL database server 5.7.9 on that computer and use that computer as database server for a Joomla installation using the webserver of your testing environment.
If the computer which you can use runs on Windows OS, you can find the downloads here: https://test5.richard-fath.de/mysql-installer-community-5.7.9.1.msi .
I know it is a not little work to do that, but if you can, please spend that time.
Otherwise, if you can't, skip this test and go to section "Test 2".
Step 1: If you don't have that already, make a new installation of current staging or latest 3.9 nightly build or 3.9.28 using a MySQL database server 5.7.9, and at the end of the installation chose to install some kind of sample data, e.g. "Blog".
Step 2: Log in to backend and go to "Components - Smart Search - Indexed Content". On a new installation you might see an alert about the smart search content plugin not being enabled. You can ignore that for now.
Step 3: Use the "Index" button to start indexing.
Result: The indexer modal is shown with a progress bar working, but after a while it end with an SQL error "You can't specify target table '#__finder_taxonomy' for update in FROM clause". See the first screenshot in section "Actual result BEFORE applying this Pull Request" below.
Step 4: Go to "Extensions - Plugins" and filter for Type = "finder".
Step 5: Disable all "Smart Search" plugins.
Result: An error alert is shown with the same SQL error as in step 3. See the second screenshot in section "Actual result BEFORE applying this Pull Request" below.
Step 6: Continue with the next test "Test 2".
Test 2: Verify that this PR fixes the issue on MySQL 5.7.9 and nothing is broken on other MySQL versions or other databases
Requirements:
Step 1: In backend, go to "Extensions - Plugins" and filter for Type = "finder".
Step 2: Enable all "Smart Search" plugins.
Step 3: Go to "Components - Smart Search - Indexed Content" and use the "Index" button.
Result: The indexer modal is shown with a progress bar working, and at the end it finishes with success and the modal is closed.
Step 4: Check which indexed content is shown.
Result: There is indexed content shown not only for the root categories of indexable extensions but also for content, e.g. articles.
E.g. with "Blog" sample data:
Step 5: Go to "Extensions - Plugins" and disable all "Smart Search" plugins.
Result: The plugins are disabled, no error alert is shown.
Step 6: Go to "Components - Smart Search - Indexed Content" and check which indexed content is shown compared to step 4.
Result: Only the root categories of indexable extensions are shown but no content like e.g. articles.
Actual result BEFORE applying this Pull Request
When using the "Index" button on "Components - Smart Search - Indexed Content":
When disabling enabled Smart Search plugins on "Extensions - Plugins":
Expected result AFTER applying this Pull Request
Documentation Changes Required
None.