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

Fix site (frontend) menus of menu type "main" or "menu" and admin (backend) main menu being destroyed by 3.7.x update #16577

Merged
merged 4 commits into from Jun 21, 2017

Conversation

Projects
None yet
8 participants
@richard67
Contributor

richard67 commented Jun 7, 2017

Pull Request for Issues #15671 , #15719 , #16643 and #16732 .

Replaces PR #15695 .

Summary of Changes

With PR #13619 by @rdeutz a schema update was introduced which tried to fix incorrect client_id of the main menu for the admin client and change menu type "menu" to "main" to fix some inconsistency remaining on some installations which have been sequentially updated since very old Joomla versions.

Starting with version 2.5, the menu types "menu" and "main" have been reserved so users were not allowed to create such menus and menu items. Since 3.7.0 only menutype "main" is reserved for the main admin menu. But in some cases it is possible to have menus and menu items with those reserved types, e.g. if having sequentially updated the installation since very old version.

PR #13619 did not care about that, and so it caused site (frontend) menus to disappear, which has caused rumors in the support forums.

PR #15695 by @alikon tried to fix that, but the discussions in that PR have shown that it was not as easy as it looked like in the first moment.

After I proposed a solution in the discussion, @alikon encouraged me to make a PR from my proposal, and here it is now.

It fixes the problem by renaming the menu type "main" for site menus before setting the correct client ID for the main menu, and by renaming a present admin menu type "menu" to "main" only if it is not a used-defined menu type.

The fix is done in the original schema update from PR #13619 and not in a new schema update, because these had to be updated anyway in order to be correct for updating 3.6.5 and earlier versions, and for later versions nothign can be done. If anyone already has run into the trouble by updating to 3.7.0, he/she had to fix the menus manually with database tools or fall back to the pre-update version, otherwise he/she will still be lost, and there is no easy way to fix the messed up menus after the problem has come. Therefore there are no new schema updates provided by this PR here.

More details about the issue and how it could be fixed can be found here in German language: http://www.joomlaportal.de/joomla-3-x-installation/328656-nach-upgrade-auf-3-7-menu-verschwunden-2.html#post1640779. This was also the inspiration for how to do it in my PR.

Testing Instructions

Preconditions:

Either have been hit by issue #15671 or #15719 and have a backup of the Joomla! from before it was updated to 3.7.0 or later and therefore hit by the issue, and have test system where you have restored that backup.

Or prepare as described in @alikon 's comment below.

Then execute as described in the same comment by @alikon .

Expected result

The update is performed without any errors. No menu has disappeared in on site (frontend) or admin (backend).

Actual result

The update is performed without any errors, but the site (frontend) menus with menu type "main" and "menu" have disappeared from site, and backend menus were sometimes messed up, too. Details see in @alikon 's comment.

Documentation Changes Required

None.

richard67 added some commits Jun 5, 2017

Fix PR 13619 sync menutype for admin menu
Fix PR #13619 "Sync menutype for admin menu and set client_id correct"
so it does not make user-created menus disappear.
Fix typo in comment
"con't" -> "don't"
@andrepereiradasilva

This comment has been minimized.

Contributor

andrepereiradasilva commented Jun 7, 2017

richard, didn't test, just noticed something: shouldn't the updates be new sql files for 3.7.3 with todays date,
instead of changing existing sql update files that already run in 3.x to 3.7.0 site updates?

@richard67

This comment has been minimized.

Contributor

richard67 commented Jun 7, 2017

Added explanation why this PR fixes the existing schema updates and does not provide new ones, see end of section "Summary of Changes".


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

@andrepereiradasilva

This comment has been minimized.

Contributor

andrepereiradasilva commented Jun 7, 2017

ok right sorry

@richard67

This comment has been minimized.

Contributor

richard67 commented Jun 7, 2017

@andrepereiradasilva Just when I was typing to add an explanation about that, I received the notification about your comment above. Seems I am getting old and so too slow ;-)


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

@richard67

This comment has been minimized.

Contributor

richard67 commented Jun 7, 2017

@andrepereiradasilva P.S.: The SQL in the old schema updates had to be corrected anyway or commented out, otherwise people again would run into a mess during the update.


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

@richard67

This comment has been minimized.

Contributor

richard67 commented Jun 7, 2017

Please be patient with testing instructions. I need time to build an update container with update package to current staging + this PR and a custom update URL for that.


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

@richard67

This comment has been minimized.

Contributor

richard67 commented Jun 7, 2017

Added testing instructions, updated description by link to German forum with manual method to fix menus before updating to 3.7.x without my patch.


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

@richard67 richard67 changed the title from Fix menu type main to Fix menu types main and menu for frontend being destroyed by 3.7.x update Jun 7, 2017

@richard67 richard67 changed the title from Fix menu types main and menu for frontend being destroyed by 3.7.x update to Fix site (frontend) menus of menu type "main" or "menu" being destroyed by 3.7.x update Jun 7, 2017

@richard67

This comment has been minimized.

Contributor

richard67 commented Jun 7, 2017

PR is ready for test now. Don't be scared of the long description - I only want to be informing and precise.


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

@rdeutz rdeutz self-assigned this Jun 7, 2017

@franz-wohlkoenig

This comment has been minimized.

franz-wohlkoenig commented Jun 9, 2017

@richard67 will test this Weekend.

@franz-wohlkoenig

This comment has been minimized.

franz-wohlkoenig commented Jun 9, 2017

@richard67 did you mean https://update.joomla.org/core/nightlies/next_patch_list.xml in "Step 2" of your Instructions?

Given URL gave Warning Update: :Collection: Could not parse.

@richard67

This comment has been minimized.

Contributor

richard67 commented Jun 9, 2017

@franz-wohlkoenig Yes, I meant the description for 3.7 on https://developer.joomla.org/nightly-builds.html, which tells to use https://update.joomla.org/core/nightlies/next_patch_list.xml. If that does not work for some reason, we have to find someone who can help with that.


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

@richard67

This comment has been minimized.

Contributor

richard67 commented Jun 10, 2017

@franz-wohlkoenig I just tested using custom URL https://update.joomla.org/core/nightlies/next_patch_list.xml for updating a clean 3.7.2 to 3.7.3-dev nightly build. I did not get any warning, but I have error loggging on in php so if there was some I should have seen it in the log file. Same with my custom URL from step 4, works without warning. Maybe there is something wrong with your testing system?


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

@franz-wohlkoenig

This comment has been minimized.

franz-wohlkoenig commented Jun 10, 2017

@richard67 i installed a clean 3.6.5 but haven't tested more. Will do it this Weekend.

@richard67

This comment has been minimized.

Contributor

richard67 commented Jun 10, 2017

Maybe there are problems with updating a 3.6.5 but not with updating a 3.7.2? I haven't tested it with 3.6.5 yet, to be honest.

@franz-wohlkoenig

This comment has been minimized.

franz-wohlkoenig commented Jun 10, 2017

I use 3.6.5 as wrote in Test Instructions. If another Version should be tested, please write and correct Instructions.

@richard67

This comment has been minimized.

Contributor

richard67 commented Jun 10, 2017

@franz-wohlkoenig
The test instructions are correct, but because I did not have a 3.6.5 and was lazy, I patched the database to fulfill the starting conditions regarding the menu types and moved the sql statements handled with this PR into some schema update starting with 3.7.3, so I could see my SQL has no syntax errors. But it is not a real life test and so maybe not acceptable for those who had the issues which this PR tries to correct. And so I did not propose this method for testing.

If I find time I will test also with 3.6.5 today, but if you did before then let me know about if there are problems with the update xmls because in this case it will also affect my custom URL for test step 4.

@franz-wohlkoenig

This comment has been minimized.

franz-wohlkoenig commented Jun 10, 2017

@richard67 will test on 3.6.5.

@richard67

This comment has been minimized.

Contributor

richard67 commented Jun 10, 2017

@mydutchtouch @jasonscottmontoya @bolando @schultz-it-solutions @shanenefdt @isherwood @inspry @affectit Since you all had the issue which this PR here wants to fix: Does anybody having a backup of the situation (Joomla files and database) before update to 3.7 which could be used for testing this PR here, and a testing site? If so, please test this PR.


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

@franz-wohlkoenig

This comment has been minimized.

franz-wohlkoenig commented Jun 10, 2017

good Idea @richard67 to let People test who reported Issue thie PR is for.

@alikon

This comment has been minimized.

Contributor

alikon commented Jun 11, 2017

i've tested upgrading from 3.6.5 patchting the zip
with 2 menutype (main,menu) with 2 menus of menutype(main,menu)
after upgrading the component list is composed by on two component (com_postinstall,com_associations)

the step 3 don't do the right work in my scenario
AND (SELECT COUNT("id") FROM "#__menu_types" WHERE "menutype" = 'menu') = 0;
this is not true i have a menu_types item menu

i've used this one instead and it worked
UPDATE '#___menu' SET 'menutype' = 'main' WHERE 'menutype' = 'menu' AND 'client_id' = 1

@richard67

This comment has been minimized.

Contributor

richard67 commented Jun 11, 2017

@alikon Are your 2 menu types for the site (client_id 0) or the admin (client id 1)? As my testing instructions say they shall be for the site.

This PR shall not rename any existing, user-created menu ot menutype "menu", because that is not reserved. What @rdeutz wanted to do is to rename the menutype for the standard joomla admin menu from "menu" to "main" if it is for some reason "menu". The standard joomla admin menu does not have any record in the menu type table. So the 3rd step will only apply in that case, which is not cour case.
In your case, the "main" menu type shall be renamed and the "menu" menu type shall be untouched.
If you do not have a user-defined menu type and menu items and modules for a menu type "menu" but accidently some or all of the standard admin menu items have value for menutype = "menu" instead of "main", then the 3rd statement will have an effect. If you have both, i.e. a user-defined menu type "menu" with user-defined menu items but also the standard ones beloning to it, you are already in a mess which cannot be cleaned up by this PR.

For testing all these possibilities it would need to add a 2nd test with different starting conditions to this PR. But the focus was to avoid that the original statements kill user-defined menu types "main" and "menu" for the site, and this is achieved.

@richard67

This comment has been minimized.

Contributor

richard67 commented Jun 16, 2017

@andrepereiradasilva I don't know whom else to ask. Do you think you can find time to test this PR here? There is a good test instruction here: Alikon's comment.

The guys who reported the issues which this PR solves do not react.

And it has to be fixed as soon as possible, otherwise update to 3.7.3 will again cause issues and rumors in support forums.

And you are a reliable tester according to my experience.


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

@wojsmol

This comment has been minimized.

Contributor

wojsmol commented Jun 18, 2017

I have tested this item successfully on 4451e20


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

@wojsmol

This comment has been minimized.

Contributor

wojsmol commented Jun 18, 2017

@franz-wohlkoenig Plese set RTC after 2 successfull test.
@rdeutz Plese merge for 3.7.3

@franz-wohlkoenig

This comment has been minimized.

franz-wohlkoenig commented Jun 18, 2017

RTC after two successful tests.

@joomla-cms-bot joomla-cms-bot added the RTC label Jun 18, 2017

@richard67

This comment has been minimized.

Contributor

richard67 commented Jun 20, 2017

What is that PR waiting for? It has 2 good tests and so is RTC, it fixes an ugly bug which caused much rumor in support forums, and it does not change any language strings ... so why is it not merged???


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

@richard67

This comment has been minimized.

Contributor

richard67 commented Jun 20, 2017

For some reasons the issue tracker shows 2 successful tests but it counts only 1 of them.


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

@rdeutz

This comment has been minimized.

Contributor

rdeutz commented Jun 20, 2017

I need time to check the PR before I press the merge button

@richard67

This comment has been minimized.

Contributor

richard67 commented Jun 20, 2017

Well, if check is ok it should definitely go into 3.7.3.

@rdeutz rdeutz added this to the Joomla 3.7.3 milestone Jun 21, 2017

@rdeutz rdeutz merged commit 206f38b into joomla:staging Jun 21, 2017

5 checks passed

JTracker/HumanTestResults Human Test Results: 2 Successful 0 Failed.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/drone/pr the build was successful
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
hound No violations found. Woof!

@joomla-cms-bot joomla-cms-bot removed the RTC label Jun 21, 2017

@richard67

This comment has been minimized.

Contributor

richard67 commented Jun 21, 2017

Finally. Well, thanks to everybody who was involved in testing and discussing and improving.

@richard67 richard67 deleted the richard67:fix-menu-type-main branch Jun 21, 2017

@rdeutz

This comment has been minimized.

Contributor

rdeutz commented Jun 21, 2017

Just one comment, it might be a non issue but what is when someone has menus named "main_to_be_deleted" or "main_is_reserved"? Shouldn't we add some hash at the end to be 100000% save?

@richard67

This comment has been minimized.

Contributor

richard67 commented Jun 21, 2017

theoretically yes. the "hash" could be a to_char of the result from "select count(id) + 1 from #__menu_type where menutype like 'main_is_reserved%'" (or so). But it is very unlikely that there is such problem.

@rdeutz

This comment has been minimized.

Contributor

rdeutz commented Jun 21, 2017

I would simply add a "LKJDSF982342"

@richard67

This comment has been minimized.

Contributor

richard67 commented Jun 21, 2017

That does not keep someone to have a menu type with such name!
And the max length of the menutype column is as far as i remember 25 chars.

@richard67

This comment has been minimized.

Contributor

richard67 commented Jun 21, 2017

something dynamic has to be added to be sure, and the easiest things is to use the count of the existing recods which have the value like the common part in the value.

@rdeutz

This comment has been minimized.

Contributor

rdeutz commented Jun 21, 2017

We can do this after RC, it is a bit paranoid :-)

@richard67

This comment has been minimized.

Contributor

richard67 commented Jun 21, 2017

well i have to check manuals of all 3 db types to do it the right way.
but as i say, "main_to_be_deleted_" concatenated with the count + 1 of the records in the menu type table where menutype like "main_to_be_deleted%" will be easy and safe.

@richard67

This comment has been minimized.

Contributor

richard67 commented Jun 21, 2017

ah, and the "main_to_be_deleted" we can avoid by deleting instead of renaming. i just was too scared of that. but it should be no problem to delete that menuy type record.

@richard67

This comment has been minimized.

Contributor

richard67 commented Jun 21, 2017

on weekend i will find time do do that improvement

@rdeutz

This comment has been minimized.

Contributor

rdeutz commented Jun 21, 2017

Thanks!

@richard67 richard67 referenced this pull request Jun 24, 2017

Merged

Improve menu main fix #16852

@richard67

This comment has been minimized.

Contributor

richard67 commented Jun 24, 2017

@rdeutz I implemented the improvements which we discussed. See PR #16852 . Please check.

@VCardWEb

This comment has been minimized.

VCardWEb commented Jul 28, 2017

Hello everyone, I had a call from a client to do an update on their Joomla! 3.2 to the most recent version Joomla! 3.7.4, so when I did the menu under Components when down, to fix it I followed @alikon instructions at https://github.com/joomla/joomla-cms/pull/16577#issuecomment-307611339 and it worked.

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