Moodle
  1. Moodle
  2. MDL-30929

detect incorrect major version upgrades by looking for files that are supposed to be removed

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.2
    • Fix Version/s: 2.3
    • Component/s: General
    • Labels:
    • Rank:
      33937

      Description

      Admins often forget to remove old PHP files when doing major upgrades - http://docs.moodle.org/22/en/Upgrading
      Solution is add a new upgrade test for files that should not be there any more...

        Issue Links

          Activity

          Petr Škoda created issue -
          Petr Škoda made changes -
          Field Original Value New Value
          Assignee moodle.com [ moodle.com ] Petr Škoda (skodak) [ skodak ]
          Petr Škoda made changes -
          Status Open [ 1 ] Development in progress [ 3 ]
          Petr Škoda made changes -
          Description People constantly forget to remove older files when doing major upgrades - http://docs.moodle.org/22/en/Upgrading

          Solution is add a new upgrade test for files that should not be there any more...
          Admins often forget to remove old PHP files when doing major upgrades - http://docs.moodle.org/22/en/Upgrading
          Solution is add a new upgrade test for files that should not be there any more...
          Hide
          Petr Škoda added a comment -

          to integrators:
          1/ please cherry pick to 2.2.x
          2/ feel free to amend the new lang pack strings

          We could also improve the Docs page.

          Show
          Petr Škoda added a comment - to integrators: 1/ please cherry pick to 2.2.x 2/ feel free to amend the new lang pack strings We could also improve the Docs page.
          Petr Škoda made changes -
          Status Development in progress [ 3 ] Waiting for integration review [ 10010 ]
          Pull Master Diff URL https://github.com/skodak/moodle/compare/master...w52_MDL-30929_m23_stalefiles
          Pull Master Branch w52_MDL-30929_m23_stalefiles
          Pull from Repository git://github.com/skodak/moodle.git
          Fix Version/s 2.2.1 [ 11456 ]
          Fix Version/s 2.3 [ 10657 ]
          Testing Instructions 1/ install Moodle 2.1
          2/ unpack Moodle 2.2 over it
          3/ observe the new error message
          Eloy Lafuente (stronk7) made changes -
          Currently in integration Yes [ 10041 ]
          Aparup Banerjee made changes -
          Status Waiting for integration review [ 10010 ] Integration review in progress [ 10004 ]
          Integrator nebgor
          Hide
          Aparup Banerjee added a comment -

          Hi Petr,
          i've looked at the patch, it does seem to be helpful. However i wonder what this would do for installations that do have custom hacks that are files. Say that some folks really wanted the old '/admin/generator.php' for example, then upgrades would always require hacking this just to get on with upgrades. It would get my +1 if this wasn't a strict rule but a sort of helpful warning/notification that could be optionally skipped.

          Also this list of files seems really ugly to me (and also a pain to maintain, yet another tedious step in release procedures).
          I'm just throwing ideas here, would it be better to just fingerprint the moodle files set and simply throw out a warning when a different hash is detected? This would also make it easier to simply update this hash in future.

          Show
          Aparup Banerjee added a comment - Hi Petr, i've looked at the patch, it does seem to be helpful. However i wonder what this would do for installations that do have custom hacks that are files. Say that some folks really wanted the old '/admin/generator.php' for example, then upgrades would always require hacking this just to get on with upgrades. It would get my +1 if this wasn't a strict rule but a sort of helpful warning/notification that could be optionally skipped. Also this list of files seems really ugly to me (and also a pain to maintain, yet another tedious step in release procedures). I'm just throwing ideas here, would it be better to just fingerprint the moodle files set and simply throw out a warning when a different hash is detected? This would also make it easier to simply update this hash in future.
          Hide
          Petr Škoda added a comment -

          1/ nobody should be adding hacks to /admin/ now, they can use /admin/tool/

          2/ if you let people just click continue they will click continue and ignore everything, for example if you keep the course reports there it is not easy to
          completely recover because the upgrade gets confused a bit

          3/ I do not care if the list of files is ugly or not, it was very carefully selected - either the files cause real problems or detect skipped delete of any previous major version; we should not definitely guess here, my list does not throw any false alarms and should catche majority of problems

          4/ the list is trivial to update in 2.3dev - we have to do it very close to the release and add something that was recently removed in 2.3dev


          Eloy could you please have a look at the internals? Sam, could you please review the new lang strings?

          This is urgent imo because several ppl reported this problem during upgrade to 2.2.0, we shouldget something like this into the next point release which may be installed by even more ppl. (People that upgrade from 2.2.0 will not be affected at all because minor upgrades do not require this full replacement of PHP files)

          Show
          Petr Škoda added a comment - 1/ nobody should be adding hacks to /admin/ now, they can use /admin/tool/ 2/ if you let people just click continue they will click continue and ignore everything, for example if you keep the course reports there it is not easy to completely recover because the upgrade gets confused a bit 3/ I do not care if the list of files is ugly or not, it was very carefully selected - either the files cause real problems or detect skipped delete of any previous major version; we should not definitely guess here, my list does not throw any false alarms and should catche majority of problems 4/ the list is trivial to update in 2.3dev - we have to do it very close to the release and add something that was recently removed in 2.3dev — Eloy could you please have a look at the internals? Sam, could you please review the new lang strings? This is urgent imo because several ppl reported this problem during upgrade to 2.2.0, we shouldget something like this into the next point release which may be installed by even more ppl. (People that upgrade from 2.2.0 will not be affected at all because minor upgrades do not require this full replacement of PHP files)
          Petr Škoda made changes -
          Priority Minor [ 4 ] Major [ 3 ]
          Aparup Banerjee made changes -
          Labels docs_required
          Aparup Banerjee made changes -
          Status Integration review in progress [ 10004 ] Waiting for integration review [ 10010 ]
          Hide
          Eloy Lafuente (stronk7) added a comment -

          I'm a bit concerned about this failing "suddenly" in the middle of minor stable (weekly) updates. So surely only will add it to cherry-pick for 2.2.x if it's clearly stated, both in the Docs, in the security spam (somehow related) and in the moodle.org news.

          In any case ideally we should decide this before THIS Saturday, if we want it added in 2.2.1. No problem for master. So let's decide in the next 48h about stable. I can add it after weeklies (2.2) if finally agreed.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - I'm a bit concerned about this failing "suddenly" in the middle of minor stable (weekly) updates. So surely only will add it to cherry-pick for 2.2.x if it's clearly stated, both in the Docs, in the security spam (somehow related) and in the moodle.org news. In any case ideally we should decide this before THIS Saturday, if we want it added in 2.2.1. No problem for master. So let's decide in the next 48h about stable. I can add it after weeklies (2.2) if finally agreed. Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday.

          This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody.

          This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P

          Apologizes for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday. This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody. This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P Apologizes for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao
          Eloy Lafuente (stronk7) made changes -
          Currently in integration Yes [ 10041 ]
          Hide
          Martin Dougiamas added a comment -

          Hmm, I think this is OK as long as the list of files is carefully targeted as being things that can really cause problems, and continues to be (more instructions in the phpdocs for upgrade_stale_php_files_present().

          Suggestion for a more helpful string:

          "Warning: Some old PHP scripts have been detected which may indicate that you installed this version over an older one. Please fix the installation directory by removing all old scripts before installing the new version and then try the upgrade again."

          Show
          Martin Dougiamas added a comment - Hmm, I think this is OK as long as the list of files is carefully targeted as being things that can really cause problems, and continues to be (more instructions in the phpdocs for upgrade_stale_php_files_present(). Suggestion for a more helpful string: "Warning: Some old PHP scripts have been detected which may indicate that you installed this version over an older one. Please fix the installation directory by removing all old scripts before installing the new version and then try the upgrade again."
          Hide
          Petr Škoda added a comment -

          "by removing all old scripts before installing the new version" that might seem to encourage them to delete config.php too

          Show
          Petr Škoda added a comment - "by removing all old scripts before installing the new version" that might seem to encourage them to delete config.php too
          Michael de Raadt made changes -
          Labels docs_required docs_required triaged
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Eloy Lafuente (stronk7) made changes -
          Fix Version/s 2.2.2 [ 11552 ]
          Fix Version/s 2.2.1 [ 11456 ]
          Hide
          Petr Škoda added a comment -

          Please keep in mind that the list of files is not meant to be used for manual cleanup, I did not include all files that may cause problems...

          In any case this was supposed to help mainly ppl upgrading to 2.2.1, now that we missed it this can go to master only.

          Show
          Petr Škoda added a comment - Please keep in mind that the list of files is not meant to be used for manual cleanup, I did not include all files that may cause problems... In any case this was supposed to help mainly ppl upgrading to 2.2.1, now that we missed it this can go to master only.
          Petr Škoda made changes -
          Pull Master Diff URL https://github.com/skodak/moodle/compare/master...w52_MDL-30929_m23_stalefiles https://github.com/skodak/moodle/compare/master...w02_MDL-30929_m23_stalefiles
          Pull Master Branch w52_MDL-30929_m23_stalefiles w02_MDL-30929_m23_stalefiles
          Fix Version/s 2.2.2 [ 11552 ]
          Testing Instructions 1/ install Moodle 2.1
          2/ unpack Moodle 2.2 over it
          3/ observe the new error message
          1/ install Moodle 2.1
          2/ unpack Moodle 2.3 over it
          3/ observe the new error message
          Sam Hemelryk made changes -
          Currently in integration Yes [ 10041 ]
          Aparup Banerjee made changes -
          Status Waiting for integration review [ 10010 ] Integration review in progress [ 10004 ]
          Hide
          Aparup Banerjee added a comment - - edited

          Hi Petr,
          ok this is to go into master only then. I've just verified the list of files.

          Everything checks out except /lib/yui/2.8.0, http://cvs.moodle.org/moodle/lib/yui/ seems to point to 2.8.0r4 instead. I've confirmed that was deleted with `git log --diff-filter=RD – lib/yui/2.8.0r4` and that was removed in 2.0 (`git branch --contains 08fcf3a0f483f204798b3a0773887a541ec09bc4`)

          also can we agree on the strings here?

          Show
          Aparup Banerjee added a comment - - edited Hi Petr, ok this is to go into master only then. I've just verified the list of files. Everything checks out except /lib/yui/2.8.0, http://cvs.moodle.org/moodle/lib/yui/ seems to point to 2.8.0r4 instead. I've confirmed that was deleted with `git log --diff-filter=RD – lib/yui/2.8.0r4` and that was removed in 2.0 (`git branch --contains 08fcf3a0f483f204798b3a0773887a541ec09bc4`) also can we agree on the strings here?
          Hide
          Martin Dougiamas added a comment -

          "Warning: Some old PHP scripts have been detected which may indicate that you installed this version over an older one. Please fix the installation directory by removing all old scripts (except config.php) before installing the new version and then try the upgrade again."

          Show
          Martin Dougiamas added a comment - "Warning: Some old PHP scripts have been detected which may indicate that you installed this version over an older one. Please fix the installation directory by removing all old scripts (except config.php) before installing the new version and then try the upgrade again."
          Hide
          Aparup Banerjee added a comment -

          stopping review - waiting for Petr's feedback here on the yui dir.

          Show
          Aparup Banerjee added a comment - stopping review - waiting for Petr's feedback here on the yui dir.
          Aparup Banerjee made changes -
          Status Integration review in progress [ 10004 ] Waiting for integration review [ 10010 ]
          Eloy Lafuente (stronk7) made changes -
          Currently in integration Yes [ 10041 ]
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Eloy Lafuente (stronk7) made changes -
          Currently in integration Yes [ 10041 ]
          Hide
          Aparup Banerjee added a comment -

          reopening - branch can't be found now!

          Show
          Aparup Banerjee added a comment - reopening - branch can't be found now!
          Aparup Banerjee made changes -
          Status Waiting for integration review [ 10010 ] Reopened [ 4 ]
          Hide
          Petr Škoda added a comment -

          branch links fixed, resubmitting, thanks

          Show
          Petr Škoda added a comment - branch links fixed, resubmitting, thanks
          Petr Škoda made changes -
          Status Reopened [ 4 ] Waiting for integration review [ 10010 ]
          Hide
          Petr Škoda added a comment -

          yui fixed too, thanks

          Show
          Petr Škoda added a comment - yui fixed too, thanks
          Aparup Banerjee made changes -
          Status Waiting for integration review [ 10010 ] Integration review in progress [ 10004 ]
          Hide
          Aparup Banerjee added a comment -

          thanks Petr, thats been integrated and is up for testing.

          tester: you should get a 'cannot continue' message when trying to upgrade from 2.1 -> 2.3 (master) as 2.2 is required for upgrading to master.

          Show
          Aparup Banerjee added a comment - thanks Petr, thats been integrated and is up for testing. tester: you should get a 'cannot continue' message when trying to upgrade from 2.1 -> 2.3 (master) as 2.2 is required for upgrading to master.
          Aparup Banerjee made changes -
          Status Integration review in progress [ 10004 ] Waiting for testing [ 10005 ]
          moodle.com made changes -
          Tester timb
          Hide
          Tim Barker added a comment -

          Tested: Upgrading Moodle database from version 2.1.4+ (Build: 20120112) (2011070104.01) to 2.3dev (Build: 20120112) (2012020200.03)

          Show
          Tim Barker added a comment - Tested: Upgrading Moodle database from version 2.1.4+ (Build: 20120112) (2011070104.01) to 2.3dev (Build: 20120112) (2012020200.03)
          Hide
          Tim Barker added a comment -

          Testing upgrade

          Show
          Tim Barker added a comment - Testing upgrade
          Tim Barker made changes -
          Status Waiting for testing [ 10005 ] Testing in progress [ 10011 ]
          Hide
          Tim Barker added a comment -

          New warning message appears: Invalid installation files detected, upgrade cannot continue
          Some old PHP scripts have been detected which may indicate that you installed this version over an older one. Please fix the installation directory by removing all old scripts (except config.php) before installing the new version and then try the upgrade again. You can find more information in upgrade documentation at http://docs.moodle.org/21/en/Upgrading

          Show
          Tim Barker added a comment - New warning message appears: Invalid installation files detected, upgrade cannot continue Some old PHP scripts have been detected which may indicate that you installed this version over an older one. Please fix the installation directory by removing all old scripts (except config.php) before installing the new version and then try the upgrade again. You can find more information in upgrade documentation at http://docs.moodle.org/21/en/Upgrading
          Tim Barker made changes -
          Status Testing in progress [ 10011 ] Tested [ 10006 ]
          Hide
          Eloy Lafuente (stronk7) added a comment -

          This is now available in the git and cvs repositories.

          Consider the responsibility of your fingerprints engraved there for future generations!

          Thanks for the work, closing, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - This is now available in the git and cvs repositories. Consider the responsibility of your fingerprints engraved there for future generations! Thanks for the work, closing, ciao
          Eloy Lafuente (stronk7) made changes -
          Status Tested [ 10006 ] Closed [ 6 ]
          Resolution Fixed [ 1 ]
          Currently in integration Yes [ 10041 ]
          Integration date 19/Jan/12
          Petr Škoda made changes -
          Link This issue has a non-specific relationship to MDL-31312 [ MDL-31312 ]
          Hide
          Helen Foster added a comment -

          Please could an upgrade expert cast their eyes over http://docs.moodle.org/en/Upgrading and either edit the page or let me know if anything needs changing, then I'll remove the docs_required label from this issue.

          Show
          Helen Foster added a comment - Please could an upgrade expert cast their eyes over http://docs.moodle.org/en/Upgrading and either edit the page or let me know if anything needs changing, then I'll remove the docs_required label from this issue.
          Hide
          Aparup Banerjee added a comment -

          <non-expert> Looks fine to me Helen. I only put the tag so that the 2.2 as a requirement would be in docs. Its seems well documented (hopefully well known too) by now

          Show
          Aparup Banerjee added a comment - <non-expert> Looks fine to me Helen. I only put the tag so that the 2.2 as a requirement would be in docs. Its seems well documented (hopefully well known too) by now
          Hide
          Helen Foster added a comment -

          Thanks Apu, removing the label then...

          Show
          Helen Foster added a comment - Thanks Apu, removing the label then...
          Helen Foster made changes -
          Labels docs_required triaged triaged

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: