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

          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.
          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)
          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
          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
          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
          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.
          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.
          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
          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!
          Hide
          Petr Škoda added a comment -

          branch links fixed, resubmitting, thanks

          Show
          Petr Škoda added a comment - branch links fixed, resubmitting, thanks
          Hide
          Petr Škoda added a comment -

          yui fixed too, thanks

          Show
          Petr Škoda added a comment - yui fixed too, thanks
          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.
          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
          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
          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
          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...

            People

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

              Dates

              • Created:
                Updated:
                Resolved: