Skip to content
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

[4.0] Don't use the "id" column as criterion in WHERE clauses for updating menu items in update SQL scripts #30701

Conversation

richard67
Copy link
Member

@richard67 richard67 commented Sep 20, 2020

Pull Request for Issue #30696 (comment).

Summary of Changes

Depending on the update history which an installation has gone through in past, menu items added by the core to the database may have different ids in the #__menu table.

E.g. on an installation which has gone through an update to 2.5 in past, when Smart Search was added to the core, the id of the smart search menu item in the admin menu (menutype='main') is 21, see update SQL 2.5.0-2011-12-22.sql in the Joomla_2.5.28-Stable-Full_Package.zip, and it will still be 21 now after the years and all updates. This is e.g. the case on my private website, which is on 3.9.21 right now.

But on a new installation of 3.9.x the id of the smart search menu item in the admin menu is 18, and on a new installation of 4.0-dev it is 13.

This is just one example. In principle it can apply to any menu item added by the core with some update after Joomla 1.0.

Therefore the golden rule is never to use the id as the criterion in the WHERE clause when updating some menu item with an update SQL script.

Unfortunately there was added an update SQL script 4.0.0-2019-05-05.sql which breaks this rule with PR #24801 in May last year, and for some reason I haven't noticed that PR and so haven't protested.

E.g. on my private website, the com_tags menu item has id=301, and the menu item for com_associations has id=346. Both of these menu items have a different link than the one it has on a new 3.9 installation. The link should be normalized by the update SQL script handled with this PR, but it isn't because of the hard-coded id not matching.

This PR here fixes that by using the combination of menutype='main' and path as criterion in the update SQL script 4.0.0-2019-05-05.sql.

With menutype='main' we can be sure it only affects the admin main menu because this name is granted to be unique and reserved for that purpose. If we had that problem with hard-coded id in update SQL scripts for other menu types (which we don't have), we would have to include also the client_id into the criterion.

We are lucky we don't have other update SQL scripts with that problem, using id as criterion in a WHERE clause..

In addition, this PR adds a new update SQL script 4.0.0-2020-09-22.sql for updating previous 4.0 Beta versions to a version which includes this PR, to fix those admin menu items which have not been fixed when updating a 3.10 with a long update history to that previous Beta version.

If this PR will be merged before the next Beta, it might make sense to combine that new update SQL with the recently added 4.0.0-2020-09-19.sql, i.e. remove the new script and put its content into the existing one which already fixes the com_findermenu item, so we don't again add a new script. We already have so many.

Testing Instructions

This PR should be tested with both kinds of databases, MySQL (or MariaDB) and PostgreSQL.

Testers please report back which kind of database you've used for the test. If you can test with both, MySQL/MariaDB and PostgreSQL, please do so. Thanks in advance.

Test 1: Verify that nothing is broken by this PR when updating a fresh installed 3.10

  1. Make a new installation of 3.10-dev or latest 3.10 nightly build or 3.10.0-alpha2.
  2. Update to the latest 4.0 nightly build by using the custom update URL https://update.joomla.org/core/nightlies/next_major_list.xml, or if you can't use Live Update for some reason use Upload & Update and the update package downloaded from https://developer.joomla.org/nightlies/Joomla_4.0.0-beta5-dev-Development-Update_Package.zip.
  3. After successful update, use an SQL client like e.g. phpMyAdmin to export the content of table #__menu (replace #__ by your table prefix) in an SQL or a CSV file.
  4. Repeat steps 1 to 3, having exactly the same starting conditions for step 1, but this time in step 2 use the custom URL or download package built for this pull request, and in step 3 exporting in the same way as before but into another file.
  1. Compare the exported data from step 4 of both tests.
    Verify that for menu items with menutype='main' there is no difference except the generated titles and aliases of links of type 'separator'.

Result: There is no difference, i.e. updating a 3.10 with the patch of this PR doesn't break anything compared to updating without the patch.

Test 2: Verify that this PR really fixes an issue and is not just cosmetics

  1. If you have a copy of a live site which is now on 3.9.21 and has gone through updates from 2.5 or earlier so that the id of the menu item for com_associations is larger than 101 (which is the site menu home item), update this site to the latest 3.10 nightly build.
    Otherwise, if you don't have such a site, make a new installation of a latest 3.10 nightly which has been patched in a way so it has menu item ids like my private website. You can donwload such an installation package from here: https://test5.richard-fath.de/Joomla_3.10.0-alpha3-dev-Development-Full_Package_2020-09-22_pr-30701.zip.
  2. Using an SQL client like e.g. phpMyAdmin, check the link value of the menu item for com_associations.
    Result: It is index.php?option=com_associations.
  3. Update to the latest 4.0 nightly build by using the custom update URL https://update.joomla.org/core/nightlies/next_major_list.xml, or if you can't use Live Update for some reason use Upload & Update and the update package downloaded from https://developer.joomla.org/nightlies/Joomla_4.0.0-beta5-dev-Development-Update_Package.zip.
  4. Check again the link value of the menu item for com_associations.
    Result: It is still the same as in step 2 before the update.
  5. Update the installation to the update package built for this pull request.
  1. Check again the link value of the menu item for com_associations.
    Result: It is index.php?option=com_associations&view=associations, i.e. the &view=associations has been appended by the update SQL script.

Result: Without the patch of this PR, some admin menu item links are not normalized with the update of 3.10 to a current 4.0 nightly. This will then be fixed by a later update to a future 4.0 Beta or RC version which will include this PR.

Actual result BEFORE applying this Pull Request

When updating a 3.10 site with a longer update history back to 2.5 or before, some admin menu item links for new components added to the core during the update history are not normalized with the update SQL script 4.0.0-2019-05-05.sql.

Expected result AFTER applying this Pull Request

When updating a 3.10 site with a longer update history back to 2.5 or before, all admin menu item links for new components added to the core during the update history are normalized with the update SQL script 4.0.0-2019-05-05.sql.

Updating a 3.10 with a shorter update history, e.g. newly installed or updated from staging/3.9.x, works as well as before.

Documentation Changes Required

None.

@richard67 richard67 changed the title [4.0] [WiP] Don't use the "id" column as criterion in WHERE clauses for updating menu items in update SQL scripts [4.0] Don't use the "id" column as criterion in WHERE clauses for updating menu items in update SQL scripts Sep 22, 2020
@richard67 richard67 marked this pull request as ready for review September 22, 2020 11:06
@infograf768
Copy link
Member

Are'nt the new update sql files redundant?

administrator/components/com_admin/sql/updates/mysql/4.0.0-2020-09-22.sql

@richard67
Copy link
Member Author

@infograf768 The new scripts are needed because the corrected old scripts will not run again when updating e.g. a Beta 4 to the future version where this PR is included, because only those update SQL scripts will be extecuted which have a version and date in their name being larger and younger than the schema version stored in database.

The few redundant statements which are the same in the old, modified file and the new file don't do harm.

But as I said, the new ones are needed to update a previous 4.0 version where the link e.g. of com_associations has not been corrected, like my test 2 does it.

@infograf768
Copy link
Member

I have tested this item ✅ successfully on 7f69470

OK on review.


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

@alikon
Copy link
Contributor

alikon commented Sep 23, 2020

I have tested this item ✅ successfully on 7f69470

tested scenario 2 with postgresql 12.1


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

@richard67
Copy link
Member Author

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Sep 23, 2020
@rdeutz rdeutz merged commit ba83c28 into joomla:4.0-dev Oct 2, 2020
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Oct 2, 2020
@rdeutz rdeutz added this to the Joomla 4.0 milestone Oct 2, 2020
@richard67
Copy link
Member Author

Thanks!

@richard67 richard67 deleted the 4.0-dev-fix-bad-update-sql-with-fixed-menu-item-ids branch October 2, 2020 10:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants