joomla / joomla-cms

Home of the Joomla! Content Management System
https://www.joomla.org
GNU General Public License v2.0
4.71k stars 3.64k forks source link

[4.0] Upgrade from j3 to J4 sql updates not run #30913

Closed brianteeman closed 3 years ago

brianteeman commented 3 years ago

Steps to reproduce the issue

I followed the upgrade procedure as documented here https://www.joomla.org/announcements/release-news/5822-joomla-4-0-0-beta4-and-joomla-3-10-alpha2.html

Expected result

Site updated with perhaps a few errors to resolve

Actual result

500 error Table 'brian_db1.ayle_workflow_associations' doesn't exist Table 'brian_db1.ayle_workflow_associations' doesn't exist

Checked the database and NONE of the workflows tables are present

Further checks show that none of the administrator\components\com_admin\sql\updates\mysql\ sql scripts appear to have run

Database Version | 5.5.5-10.5.5-MariaDB PHP Version | 7.3.23

-- | --

richard67 commented 3 years ago

@brianteeman I can't reproduce that here, starting with current staging (and the version patched from "3.9.22-rc" to "3.9.22-dev", because there seems to be another issue with version comparison when it contains "-rc", so the update to 3.10-alpha2 wasn't found, but that's another issue).

So I need more info:

brianteeman commented 3 years ago

@richard67 I think I have worked out the cause. Give me 10 minutes to confirm

richard67 commented 3 years ago

I give you as many minutes as you need ;-)

brianteeman commented 3 years ago

OK so I managed to get further this time and I can see what is happening and what we need to fix.

The problem is with isSite and isAdmin which have been REMOVED in favour of isClient('site') and isClient('admin')

If you have a system plugin that uses those then it kills your site BEFORE the final stages of the update.

Normally you would do the update and then the site logs you out and you have to log back in and then enter your details again to complete the process.

The problem is that if you have a system plugin using either of those methods then the upgrade process dies at that point and there is no way to continue and you have a completely bricked web site

The isSite and isAdmin are VERY widely used and if we dont do something about this then its bye bye joomla.

You couldnt replicate it because you were testing with just core extensions.

It is a known issue https://docs.joomla.org/Potential_backward_compatibility_issues_in_Joomla_4#CMSApplication

It has been reported many many times https://github.com/joomla/joomla-cms/search?q=isSite&type=issues but I dont think anyone has reported before that it breaks upgrades and kills the sites. Just that a specific extension or template needed updating.

richard67 commented 3 years ago

Unassigned me because it's not a database issue. But I agree it has to be solved.

brianteeman commented 3 years ago

Can it please be marked as a release blocker

wilsonge commented 3 years ago

The problem is that these functions specifically will not function as expected with the new console and api applications because they were largely used in the context of

if ($app->isAdmin()) { // stuff } else { // site stuff }

Or vice versa. I saw that pattern used lots (and even used it myself once upon a time)

brianteeman commented 3 years ago

We've ignored these reports since the change was made. Always blaming the developer for using a deprecated function and telling people just to a search and replace.

We can say its been deprecated for a few years as much as we want (I've said it myself) but when your site is dead you're not interested in assigning blame - you just want your site to work.

When it is in a system plugin then it kills the update and leaves the site in an unusable state that can only be recovered by starting again or manually running each sql script in the com_admin/ directory to even get to a state where the search and replace will be useful. That is assuming that you even know that you have to run those 72 sql scripts manually and know how to do it.

I suggested it before "why cant the old code be aliased to the new code" if the removal of the code really cant be reverted.

Finally - we can't even always blame the developer. Until recently the user profile plugin used this code and users were actively encourage to fork that plugin and use it as an example. I doubt that anyone who did that realised that the code needed to be changed. I know I didnt!

Fedik commented 3 years ago

@wilsonge suggestion: Create isSite()/isAdmin() only for SiteApplication and AdministratorApplication (and mark them depreciated)

In this way an old extensions will continue to work, and new extensions (which use Api/Cli Application) must use new method isClient() from CMSApplication

zero-24 commented 3 years ago

@brianteeman just a question not to blame anyone but what did the pre upgrade checker said for that extension? Has the Developer declared that extension to be 4.x compatible or not?

zero-24 commented 3 years ago

@wilsonge suggestion: Create isSite()/isAdmin() only for SiteApplication and AdministratorApplication (and mark them depreciated)

In this way an old extensions will continue to work, and new extensions (which use Api/Cli Application) must use new method isClient() from CMSApplication

Hmm they are deprecated for a longer time already. I'm not sure whether another new method to deprecated them helps here.

The problem is that when that plugin is triggerd by an cli app and it uses isAdmin or isSite that would fail the cli app right as this would try to call an not defined method. Or i'm missing something?

brianteeman commented 3 years ago

No the pre-update check didnt say anything.

Just to remind you this is NOT about the offending code spitting out an error message or not working. That is easy to resolve. The problem is that this problem kills the update process leaving you with a partially updated site that is completely unusable and can only be recovered by completing deleting and reinstalling a backup.

zero-24 commented 3 years ago

No the pre-update check didnt say anything.

Just to remind you this is NOT about the offending code spitting out an error message or not working. That is easy to resolve. The problem is that this problem kills the update process leaving you with a partially updated site that is completely unusable and can only be recovered by completing deleting and reinstalling a backup.

Hmm can you send me the plugin in question? It should atleast say something.

I get the thing with the broken update. But that is how joomla works right now. We update the CMS with the same application that also runs other parts so all system events are triggered too. In an ideal world we would have an dedicated update app that would trigger dedicated plugin events only used in the updater.

I'm not sure how we could solve that as all other breaking changes introduced in 4.x could result into similiar issues when used in plugins.

Even an 'disable broken extensions' thing like Wordpress did would not help here i guess as it first need to fail in order to act and at this point the update is already broken.

And I'm not sure whether it is wise to make an conditional to all system plugin events that they should not run in the update process.

So i'm a bit out of ideas how to solve that other than in the extension itself.

brianteeman commented 3 years ago

sorry it did says something - the "no information available" message

This is not a case of a broken extension - we can live with that. This is a completely destroyed and unrecoverable web site

It is easy to test and see what happens. Just create a system plugin with code like this and do the update

    function onAfterRoute()
    {

        $app = JFactory::getApplication();

        if ( $app->isAdmin() )
        {
            return;
        }
    }

So i'm a bit out of ideas how to solve that other than in the extension itself.

Revert the removal of the code

HLeithner commented 3 years ago

if the extension is not ready for j4 you should not update your site.

zero-24 commented 3 years ago

sorry it did says something - the "no information available" message

Yes and that means that extension should be carefully checked upfront the Upgrade right?

This is not a case of a broken extension - we can live with that. This is a completely destroyed and unrecoverable web site

It is easy to test and see what happens. Just create a system plugin with code like this and do the update

  function onAfterRoute()
  {

      $app = JFactory::getApplication();

      if ( $app->isAdmin() )
      {
          return;
      }
  }

I know i just wanted the plugin to check why it did not show up in the pre Upgrade checker.

Btw you can break your site by calling any removed or broken code at that point even when you Upgrade from 3.9.21 to 3.9.22 (well there we have no sql stuff so you might be save here).

So i'm a bit out of ideas how to solve that other than in the extension itself.

Revert the removal of the code

I'm not sure whether that is a good solution too as mention by george. By that argument we have to revert all breaking changes out of 4.x as that might break the site on upgrade.

We might have to add just another more and bigger warning when system plugins are not compatible or no info is there?

Or we might disable extensions not compatible? That could break the site to but 'maybe' just the frontend and hopefully the backend still works?

We could also add just another warning and confirmation once you are upgrading when incompatible extensions are detected?

brianteeman commented 3 years ago

if the extension is not ready for j4 you should not update your site.

Of course not. That was NOT the case here. This was one of the million extensions that provided no update information

zero-24 commented 3 years ago

if the extension is not ready for j4 you should not update your site.

Of course not. That was NOT the case here. This was one of the million extensions that provided no update information

Any suggestion how to handle that? (other than reverting all breaking changes out of 4.x) Maybe one of my suggestions above can help with that? So special extensions like system plugins get a dedicated notice once found incompatibel or no info?

Fedik commented 3 years ago

The problem is that when that plugin is triggerd by an cli app and it uses isAdmin or isSite that would fail the cli app right as this would try to call an not defined method.

That makes sense :)

Another idea, more fancy, but should keep us on safe side. After compatibility check and before actually the upgrade: The script store a map of enabled core/non-core extensions => turn off all non core extensions => make upgrade => restore disabled extensions.

With this we can be sure that 3d extension will not crash the upgrade (even if it say "it is compatible")

HLeithner commented 3 years ago

if the extension is not ready for j4 you should not update your site.

Of course not. That was NOT the case here. This was one of the million extensions that provided no update information

ok then this screen is missleading, if we hvae no information to a component then this component has to be marked as incompatible.

I tested one of my own components and ask my self which version the pre-update checker shows: image

whats version 1.3.2 and why is it compatible?

after switching to https://update.joomla.org/core/test/310to4_list.xml it shows a big red block saying no update information available....

image

I would suggest to make a more explicit warning for major versions with known b/c breaks. If any extension is not marked as compatible don't let the user update without explicit confirmation.

HLeithner commented 3 years ago

The problem is that when that plugin is triggerd by an cli app and it uses isAdmin or isSite that would fail the cli app right as this would try to call an not defined method.

That makes sense :)

Another idea, more fancy, but should keep us on safe side. After compatibility check and before actually the upgrade: The script store a map of enabled core/non-core extensions => turn off all non core extensions => make upgrade => restore disabled extensions.

With this we can be sure that 3d extension will not crash the upgrade (even if it say "it is compatible")

sadly that will not work, because there are badly written plugins that thinks that the __construct function is a good place to execute code and joomla can't disabled this...

Fedik commented 3 years ago

sadly that will not work, because there are badly written plugins that thinks that the __construct function...

hm, why? if extension is "disabled" it should not be even booted

Or can go even further, and turn off all extensions, leave only bare minimum, and then re-enable

brianteeman commented 3 years ago

sorry it did says something - the "no information available" message

Yes and that means that extension should be carefully checked upfront the Upgrade right?

If it does mean that then it needs to say it - which it doesn't

This is the output of my test site with 50% extensions providing no information including several very well known ones

updatereport.pdf

This is just being plain stubborn to prove a point that joomla extensions are not made by real developers. We know for a fact that the isSite/isAdmin change on a system plugin (at least) will break the update. We have no choice but to fix it. We can not bury our head in the sand and blame others. We've had reports of this since the beginning.

brianteeman commented 3 years ago

The problem is that when that plugin is triggerd by an cli app and it uses isAdmin or isSite that would fail the cli app right as this would try to call an not defined method.

That makes sense :)

That's a problem but it is not fatal. (ignoring the fact that a very small % will even use a cli app). If you get a failure with a message then you can react to it. This is a fatal error and would be the nail in the coffin for many joomla sites

zero-24 commented 3 years ago

sorry it did says something - the "no information available" message

Yes and that means that extension should be carefully checked upfront the Upgrade right?

If it does mean that then it needs to say it - which it doesn't

Can you suggest a better text for that message. I'm always happy to improve that upgrade checker.

This is the output of my test site with 50% extensions providing no information including several very well known ones

updatereport.pdf

This is just being plain stubborn to prove a point that joomla extensions are not made by real developers.

I dont think this has much todo with who did the plugin and whether there are "real developers" or not. But that set of extensions seems to be not marked compatible might just for the case that this developer did not tested it on 4.x

We know for a fact that the isSite/isAdmin change on a system plugin (at least) will break the update.

Yes and the same is true again for all b/c breaks we did.

We have no choice but to fix it.

I agree and I'm happy to discuss and improve the pre upgrade checker but i disagree to "just" revert that PR back as that would mean we have to revert any b/c break back.

We can not bury our head in the sand and blame others. We've had reports of this since the beginning.

No I do not I have suggested workarounds that works generally not only for isAdmin/isSite.

Fedik commented 3 years ago

Additionally to my last comment.

Main goal is to make a core upgrade without crash. Literally execute update #_extension set state = 0 where <non core>, and load page with upgrade process (it important for unload disabled extensions).

After upgrade succeed set state = 1 back. It may lead to crash, but at this point we sure that the core is updated and can handle fatal errors.

brianteeman commented 3 years ago

We know for a fact that the isSite/isAdmin change on a system plugin (at least) will break the update.

Yes and the same is true again for all b/c breaks we did.

No it is not. That is the point.

It is perfectly acceptable for an extension not to work because of b/c breaks. That is expected (at least by me). I was actually preparing a tutorial video to show people what to do after an update when an extension doesn't work.

What is not acceptable is for the update to fail and leave you with a site in a state that can only be recovered by completing erasing it and reinstalling a backup.

zero-24 commented 3 years ago

Main goal is to make a core upgrade without crash. Literally execute update #_extension set state = 0 where , and load page with upgrade process (it important for unload disabled extensions).

What happens with extensions that are expected to execute code before or after the update. similiar to the "backup before update" plugin from akeeba?

zero-24 commented 3 years ago

We know for a fact that the isSite/isAdmin change on a system plugin (at least) will break the update.

Yes and the same is true again for all b/c breaks we did.

No it is not. That is the point.

well the same happens when you have a plugin that uses JMail::sendAdminMail (https://docs.joomla.org/Potential_backward_compatibility_issues_in_Joomla_4#Mail) or use JString. (https://docs.joomla.org/Potential_backward_compatibility_issues_in_Joomla_4#String)

isAdmin and isSite is more common for sure.

What is not acceptable is for the update to fail and leave you with a site in a state that can only be recovered by completing erasing it and reinstalling a backup.

I agree that is the reason the pre upgrade checker lists all extensions to be checked upfront within 3.10 not after the upgrade. What about the suggestions I did above for that special plugins that can acctually break the upgrade so we catch that issues before the update happens?

Fedik commented 3 years ago

What happens with extensions that are expected to execute code before or after the update

It depend "when" we disable them, and when the Upgrade event will be triggered. Could be like this:

zero-24 commented 3 years ago

What happens with extensions that are expected to execute code before or after the update

It depend "when" we disable them, and when the Upgrade event will be triggered. Could be like this:

  • check extensions compatibility
  • trigger onBeforeUpgrade
  • execute #_extension set state = 0
  • reload page and run upgrade
  • execute #_extension set state = 1
  • reload the page to boot disabled extensions
  • trigger onAfterUpgrade

Ok but this has to be implemented in 3.10 and by that change we break existing plugins using the old way right? That would be a B/C break or I'm missing something? That could be a workaround we could implement into 4.x for the next updates. And in this specific case of a system plugin calling an undefined method at onAfterRoute would still break the site and the backend.

brianteeman commented 3 years ago

I agree that is the reason the pre upgrade checker lists all extensions to be checked upfront within 3.10 not after the upgrade.

Did you look at the list I shared - no way anyone (even a hard core dev like yourself) is going to check that

/me off to debug a different update issue with sql

Fedik commented 3 years ago

by that change we break existing plugins using the old way right? That would be a B/C break or I'm missing something?

we do not break anything we just turn them off, at 1 step before actually replacing files and run sql update. upd: or maybe I missing something

And in this specific case of a system plugin calling an undefined method at onAfterRoute would still break the site and the backend.

While upgrade: the plugin is disabled, so no break. After upgrade: yes it will break, but at this point the core upgrade is finished, and the exception handler will show what is going on, so User can guess where to look.

zero-24 commented 3 years ago

Did you look at the list I shared - no way anyone (even a hard core dev like yourself) is going to check that

Well i would check them as i know the consequences. But yes thats the reason i suggested the message for the system plugins that could break the upgrade any feedback on that idea is welcome.

we do not break anything we just turn them off, at 1 step before actually replacing files and run sql update. upd: or maybe I missing something

Well theoretical some extension could listen on a specific update step to execute some code like the backup on update plugin or like set the site in some kind of update mode or whatever bussines logic makes sense at this point. That usecases would be broken and to me this would be a B/C break that can not go into 3.x but has to be in 4.x where we also have to solve the thing that we finish the update with a different codebase as we started from so for the first version we add such thing we have to take care that this update to enable the extensions again is not breaking ;)

While upgrade: the plugin is disabled, so no break. After upgrade: yes it will break, but at this point the core upgrade is finished, and the exception handler will show what is going on, so User can guess where to look.

Agree but i dont think someone should update to 4.x when there are known issues with such central plugins.

brianteeman commented 3 years ago

You would check all the extensions in that list? Really?

The pre-update check does not report what type of plugin it is

Agree but i dont think someone should update to 4.x when there are known issues with such central plugins.

They do not know there are such issues

zero-24 commented 3 years ago

You would check all the extensions in that list? Really?

Well I personally would not install that many extensions in the first place but yes i would check why they fail or atleast disable them manually upfront. That would also work even without B/C.

We might mention that workaround disable the extensions listed here in the pre upgrade checker can you suggest a good wording for that?

The pre-update check does not report what type of plugin it is

That is one of the simplest thing to add when that helps but a dedicated message for system plugins too.

They do not know there are such issues

Well there are 40 extensions that joomla can not check / say whether it is compatible, that is the issue right? I would asume that some of them fail.

Fedik commented 3 years ago

theoretical some extension could listen on a specific update step to execute some code like the backup on update plugin or like set the site in some kind of update mode or whatever bussines logic makes sense at this point

that why I have separate it to a different steps ;)

btw, do we have a loot of events while core update?

brianteeman commented 3 years ago

I'm giving up - I will not participate in any discussion where you deliberately misquote me. And you clearly did not actually read the extension list I posted.

zero-24 commented 3 years ago

that why I have separate it to a different steps ;)

Yes but that new plugin event has to be implemented by the plugin first right? as the old way using onAfterRoute is not working anymore. -> B/C break ;)

I'm giving up - I will not participate in any discussion where you deliberately misquote me.

I'm sorry for that. It was not my intention to miss quote you where did i did that? So we can clear things up.

And you clearly did not actually read the extension list I posted.

I checked the list you posted but what is the point? That there are old fof stuff listed? That dpcalendar is listed? I seem to miss something in that context?

zero-24 commented 3 years ago

btw, do we have a loot of events while core update?

well that list here for sure: https://docs.joomla.org/Plugin/Events/System as a full page is loaded a few times probertly a few more too.

Fedik commented 3 years ago

well that list here for sure: https://docs.joomla.org/Plugin/Events/System

I meant "update" specific, well I just found onJoomlaAfterUpdate, and nothing for "Before"

Yes but that new plugin event has to be implemented by the plugin first right?

I did not meant new events ;) I thought there already exists Before and After update, but seems only After exist, that make thing a bit complicated :smile:

Okay, may need to split &task=update.install to 3 (or more) tasks, and modify &task=update.finalise: 1 &task=update.install - at this point we doing nothing, just redirect to next task, this allows to extensions hookup as usual. 2 &task=update.step1 - here the script store what extensions was enabled, and disable all of them, then redirect to next task 3 &task=update.step2 - here is where the files update happens, and redirect to next task 4 &task=update.step3 - this is existing update.finalise, it execute update script, update SQL queries, and redirect to next task 5 &task=update.step4 - restore previously disabled extensions, redirect to next task 6 &task=update.done - here run onJoomlaAfterUpdate and show "Hello world!" message

Important that between step1 ... stepX should not be allowed to run any extension.

There may be more or less steps (example can split to 2 update script and update SQL if need), or they may be ordered differently.

In general it just an idea.

brianteeman commented 3 years ago

Please just do what I asked. Create a system plugin with the removed code and see what happens. It is NOT that extensions may not work on an upgraded site. Everyone accepts that. The problem is that this "pointless" code change results in a failed update. It leaves the site in a state that can neither be fixed or reverted. the only option is to completely wipe the site.

infograf768 commented 3 years ago

Reopening issue as not solved.

Fedik commented 3 years ago

@brianteeman would be nice to have more solid fix that prevent :

It leaves the site in a state that can neither be fixed or reverted. the only option is to completely wipe the site.

It happen because how Update process works: &task=update.install - replace new files, then new request to &task=update.finalise - here should be executed update script and SQL update, but it never happen because buggy extension is booted and doing a dirty stuff onAfterRoute and before actual update.finalise executed. That break update process and leaves the site broken, because outdated database schema.

It may happen with any other method used by extension that was removed in j4, but existed previously.

Yeah we can just restore isSite/isAdmin, but this bug may come up in another form, later.

zero-24 commented 3 years ago

well that list here for sure: https://docs.joomla.org/Plugin/Events/System

I meant "update" specific, well I just found onJoomlaAfterUpdate, and nothing for "Before"

Yes but that new plugin event has to be implemented by the plugin first right?

I did not meant new events ;) I thought there already exists Before and After update, but seems only After exist, that make thing a bit complicated :smile:

Okay, may need to split &task=update.install to 3 (or more) tasks, and modify &task=update.finalise: 1 &task=update.install - at this point we doing nothing, just redirect to next task, this allows to extensions hookup as usual. 2 &task=update.step1 - here the script store what extensions was enabled, and disable all of them, then redirect to next task 3 &task=update.step2 - here is where the files update happens, and redirect to next task 4 &task=update.step3 - this is existing update.finalise, it execute update script, update SQL queries, and redirect to next task 5 &task=update.step4 - restore previously disabled extensions, redirect to next task 6 &task=update.done - here run onJoomlaAfterUpdate and show "Hello world!" message

Important that between step1 ... stepX should not be allowed to run any extension.

There may be more or less steps (example can split to 2 update script and update SQL if need), or they may be ordered differently.

In general it just an idea.

Good idea for sure. Do you think you can create a PR for that? Some of the update logic is a mix of JS and PHP and tbh I'm not that familiar with JS ;-)

As this would be a B/C break i think this has to go aganinst 4.0.

Fedik commented 3 years ago

I can try, only it will be not fast :)

As this would be a B/C break i think this has to go aganinst 4.0

I think we need it in 3.10, to prevent crash while transition 3.10 => 4.0 It could be done with keeping b/c, all we need is to keep first task name &task=update.install and last task name &task=update.finalise (instead of &task=update.done as I wrote) and trigger onJoomlaAfterUpdate as it currently.

I do not see other b/c issues.

zero-24 commented 3 years ago

I can try, only it will be not fast :)

No problem. Feel free to contact me when you have a question so we can move that forward together. :+1:

As this would be a B/C break i think this has to go aganinst 4.0

I think we need it in 3.10, to prevent crash while transition 3.10 => 4.0 It could be done with keeping b/c, all we need is to keep first task name &task=update.install and last task name &task=update.finalise (instead of &task=update.done as I wrote) and trigger onJoomlaAfterUpdate as it currently.

I do not see other b/c issues.

Great idea! Yes that could work in a B/C way.

At that step we can also add a dedicated onJoomlaBeforeUpdate so noone is required to do it that hacky way anymore. ;-)

wilsonge commented 3 years ago

I dunno how @nikosdion is doing backups but I'm pretty sure disabling all system plugins is going to cause issues with things like the backup before update plugins (and other such extensions too).

How about we just catch the exceptions in system plugins and continue? Even better would be from the stacktrace trying to disabling the plugin that caused the issue (Like what wordpress announced at some point)

brianteeman commented 3 years ago

How about just accepting that the removal of this code was a mistake

zero-24 commented 3 years ago

I dunno how @nikosdion is doing backups but I'm pretty sure disabling all system plugins is going to cause issues with things like the backup before update plugins (and other such extensions too).

Yes thats the reason we want to keep the old processes to work. So they are still fired and can run the thing they need but they can not break the upgrade.

How about we just catch the exceptions in system plugins and continue? Even better would be from the stacktrace trying to disabling the plugin that caused the issue (Like what wordpress announced at some point)

Yes i thourgt about that too the problem is at this point the upgrade is already broken right? As even for wp you need a site reload to restart without that plugin.