Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-34046

"Invalid installation files detected" message may be confusing

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3
    • Fix Version/s: 2.3.2
    • Component/s: Installation
    • Labels:
      None

      Description

      The message "Invalid installation files detected, upgrade cannot continue", is not helpful as it includes no indication of which files are blocking the upgrade.

      To reproduce the problem

      • add a file at /admin/generator.php
      • decrement Moodle version number in database

        UPDATE mdl_config SET value=(value-1) WHERE name='version'

      • log in to Moodle as admin to initiate upgrade process
      • notice that upgrade "Invalid installation files detected" message does not include any indication of which files are blocking the upgrade.

      The following modifications will report a list of files and folders that are blocking the upgrade and allow the server admin to take remedial action:

      lib/upgradelib

      • around line 319
      • upgrade_stale_php_files_present()
      • return an array of stale files or false

        upgrade_stale_php_files_present() in lib/upgradelib.php

        function upgrade_stale_php_files_present() {
            global $CFG;
         
            $files = array();
         
            $someexamplesofremovedfiles = array(
                /* list of files goes here */
            );
         
            foreach ($someexamplesofremovedfiles as $file) {
                if (file_exists($CFG->dirroot.$file)) {
                    $files[] = $file;
                }
            }
         
            if (count($files)) {
                return $files;
            } else {
                return false;
            }
        }

      admin/renderer.php

      • around line 66
      • upgrade_stale_php_files_page() method
      • optionally accept and display an array of file/folder paths

        upgrade_stale_php_files_page() in admin/renderer.php

        public function upgrade_stale_php_files_page($files = false) {
         
            /* start output as before */
            $output .= get_string('upgradestalefilesinfo', 'admin', get_docs_url('Upgrading'));
            if ($files) {
                $output .= html_writer::start_tag('ul');
                foreach ($files as $file) {
                    $output .= html_writer::tag('li', $file);
                }
                $output .= html_writer::end_tag('ul');
            } else {
                $output .= html_writer::empty_tag('br');
            }
            /* finish output as before */
        }

      admin/index.php

      • around line 203
      • pass list of stale files from upgrade_stale_php_files_present() to $output->upgrade_stale_php_files_page($files)

        admin/index.php

        if ($files = upgrade_stale_php_files_present()) {
            $PAGE->set_title($stradministration);
            $PAGE->set_cacheable(false);
         
            $output = $PAGE->get_renderer('core', 'admin');
            echo $output->upgrade_stale_php_files_page($files);
            die();
        }

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            skodak Petr Skoda added a comment -

            It was intentionally implemented like this, admins must learn to upgrade sites properly, that is during each major upgrade to move away all existing files and copy back config.php + new versions of contrib plugins, or better use git. This was a source of many problems before...

            Your patch is wrong because the list is NOT complete, you can not delete only those listed files! You need to upgrade properly, sorry. More explanation in the linked issue.

            Petr

            Show
            skodak Petr Skoda added a comment - It was intentionally implemented like this, admins must learn to upgrade sites properly, that is during each major upgrade to move away all existing files and copy back config.php + new versions of contrib plugins, or better use git. This was a source of many problems before... Your patch is wrong because the list is NOT complete, you can not delete only those listed files! You need to upgrade properly, sorry. More explanation in the linked issue. Petr
            Hide
            xxxxxxx Gordon Bateson added a comment -

            > Your patch is wrong because ...

            No, my patch is perfectly correct. It works exactly as stated. It reports any missing stale files that were detected. I know for sure because I tested it.

            As it turned out, the message was caused by my logging in as Moodle admin when the CVS update was only half complete on the server. I was upgrading the HEAD branch. How was I to know it was going from 2.3 to 2.4? Once the CVS upgrade was complete, the message went away.

            The message without a list of stale files is not helpful. It is mysterious and frustrating. I'm going to keep my friendly and informative patch on my site, and you can keep on explaining to people why it is better for the software to be mysterious and frustrating.

            Gordon

            Show
            xxxxxxx Gordon Bateson added a comment - > Your patch is wrong because ... No, my patch is perfectly correct. It works exactly as stated. It reports any missing stale files that were detected. I know for sure because I tested it. As it turned out, the message was caused by my logging in as Moodle admin when the CVS update was only half complete on the server. I was upgrading the HEAD branch. How was I to know it was going from 2.3 to 2.4? Once the CVS upgrade was complete, the message went away. The message without a list of stale files is not helpful. It is mysterious and frustrating. I'm going to keep my friendly and informative patch on my site, and you can keep on explaining to people why it is better for the software to be mysterious and frustrating. Gordon
            Hide
            skodak Petr Skoda added a comment -

            No, your patch is incorrect, it completely misses the purpose of this check.

            Show
            skodak Petr Skoda added a comment - No, your patch is incorrect, it completely misses the purpose of this check.
            Hide
            xxxxxxx Gordon Bateson added a comment -

            My patch is correct, because it fixes the issue that I had, which is that I wanted to know what files were missing.

            I understand that you have your reasons why you don't want to accept this suggestion. Too bad.

            Show
            xxxxxxx Gordon Bateson added a comment - My patch is correct, because it fixes the issue that I had, which is that I wanted to know what files were missing. I understand that you have your reasons why you don't want to accept this suggestion. Too bad.
            Hide
            skodak Petr Skoda added a comment -

            Hmm, you still do not understand what the $someexamplesofremovedfiles means, it is a tiny subset of files that were present in previous installs but were removed in later versions. You solve absolutely nothing if you remove the listed files from borked installs, you only hide existing problems - that is definitely not a fix.

            Show
            skodak Petr Skoda added a comment - Hmm, you still do not understand what the $someexamplesofremovedfiles means, it is a tiny subset of files that were present in previous installs but were removed in later versions. You solve absolutely nothing if you remove the listed files from borked installs, you only hide existing problems - that is definitely not a fix.
            Hide
            xxxxxxx Gordon Bateson added a comment - - edited

            FYI, I understand perfectly what $someexamplesofremovedfiles means, and what the purpose of the check is.

            What you have failed to understand is the effect of the message on the unfortunate Moodle admin who sees it.

            The message says:

            Some old PHP scripts have been detected ... Please fix the installation directory by removing all old scripts

            This message is mysterious and confusing. It tells the admin to remove some files but it doesn't tell me what the files are. Don't tell me again that the admin doesn't need to know the list of files that Moodle found. I understand that point. I am talking here about the effect of the message on the admin who reads it for the first time.

            Through my interaction with you on this issue I have learned that this message actually means, "Always install a new Moodle release into a new directory". Removing files should not be mentioned in the message, or if we do mention removing files, we should explain what files were found and what that means.

            I can see you adamantly believe that "you solve absolutely nothing if you remove the listed files", in which case I am sure you would agree with me that the message should be changed to emphasize installing into a clean directory rather than "removing old scripts".

            For reference, here is the complete heading and message regarding stale scripts:

            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

            Show
            xxxxxxx Gordon Bateson added a comment - - edited FYI, I understand perfectly what $someexamplesofremovedfiles means, and what the purpose of the check is. What you have failed to understand is the effect of the message on the unfortunate Moodle admin who sees it. The message says: Some old PHP scripts have been detected ... Please fix the installation directory by removing all old scripts This message is mysterious and confusing. It tells the admin to remove some files but it doesn't tell me what the files are. Don't tell me again that the admin doesn't need to know the list of files that Moodle found. I understand that point. I am talking here about the effect of the message on the admin who reads it for the first time. Through my interaction with you on this issue I have learned that this message actually means, "Always install a new Moodle release into a new directory". Removing files should not be mentioned in the message, or if we do mention removing files, we should explain what files were found and what that means. I can see you adamantly believe that "you solve absolutely nothing if you remove the listed files", in which case I am sure you would agree with me that the message should be changed to emphasize installing into a clean directory rather than "removing old scripts". For reference, here is the complete heading and message regarding stale scripts: 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
            Hide
            jmvedrine Jean-Michel Vedrine added a comment -

            Hello Gordon,
            You are not reading the warning correctly you say "it tells the admin to remove some files" this is not true, the exact words are "removing ALL (note the word all please !) old scripts (except config.php)". Even if english is not my native language, for me (and for nearly all other Moodle users I guess !) it means exactly "Always install a new Moodle release into a new directory" with just a config.php script in it !
            Don't you feel like wasting Petr's time on this issue ?

            Show
            jmvedrine Jean-Michel Vedrine added a comment - Hello Gordon, You are not reading the warning correctly you say "it tells the admin to remove some files" this is not true, the exact words are "removing ALL (note the word all please !) old scripts (except config.php)". Even if english is not my native language, for me (and for nearly all other Moodle users I guess !) it means exactly "Always install a new Moodle release into a new directory" with just a config.php script in it ! Don't you feel like wasting Petr's time on this issue ?
            Hide
            skodak Petr Skoda added a comment -

            Let me give you a few examples why we have this ugly warning (again this list is not complete):

            1/ the admin and course reports were moved to new location in 2.2, if admin just unzips the directory over old 2.1 install they get the same files twice which creates fatal errors in some areas, the $someexamplesofremovedfiles does NOT contain all of these directory, just one of each of them

            2/ CVS creates empty dirs during update if I remember it correctly, this breaks enumeration of plugins in some places - again we have detection of 3 dirs on in $someexamplesofremovedfiles

            3/ global search was deleted completely - if you keep the files you get fatal errors

            As I said in the other issue the list of all files that should not be there any more is really huge, but the problem there is that we do not know what is supposed to be there. Solution would be to whitelist instead of blacklist all files that should be there, but to do that we would need to manage the installed plugins through moodle and most probably keep a copy (or at least a file list) of everything that was installed. That is a huge task, but hopefully somebody will work on that soon.

            I did not write this message alone, I asked other developers for help because English is not my native language and sometimes what I write/say does not make much sense (I wish it always did in Czech at least). So please if you think the message is confusing (I think it may be) please propose a better message that tell admins: "Stop unzipping new PHP files over old install when upgrading to next major version (it is ok for minor releases though) - you must remove ALL previous files except config.php first. This warning may be caused by incomplete CVS update too."

            Show
            skodak Petr Skoda added a comment - Let me give you a few examples why we have this ugly warning (again this list is not complete): 1/ the admin and course reports were moved to new location in 2.2, if admin just unzips the directory over old 2.1 install they get the same files twice which creates fatal errors in some areas, the $someexamplesofremovedfiles does NOT contain all of these directory, just one of each of them 2/ CVS creates empty dirs during update if I remember it correctly, this breaks enumeration of plugins in some places - again we have detection of 3 dirs on in $someexamplesofremovedfiles 3/ global search was deleted completely - if you keep the files you get fatal errors As I said in the other issue the list of all files that should not be there any more is really huge, but the problem there is that we do not know what is supposed to be there. Solution would be to whitelist instead of blacklist all files that should be there, but to do that we would need to manage the installed plugins through moodle and most probably keep a copy (or at least a file list) of everything that was installed. That is a huge task, but hopefully somebody will work on that soon. I did not write this message alone, I asked other developers for help because English is not my native language and sometimes what I write/say does not make much sense (I wish it always did in Czech at least). So please if you think the message is confusing (I think it may be) please propose a better message that tell admins: "Stop unzipping new PHP files over old install when upgrading to next major version (it is ok for minor releases though) - you must remove ALL previous files except config.php first. This warning may be caused by incomplete CVS update too."
            Hide
            xxxxxxx Gordon Bateson added a comment - - edited

            Thank you Petr. I appreciate your taking the time to consider this issue with me, and for suggesting a revision to the strings which appear when mixed Moodle versions are detected.

            Below is the my proposal for the revised "upgradestalefiles" and "upgradestalefilesinfo" strings in the "lang/en/admin.php" lang file.

            I am not sure who to present these revised strings to for consideration, and I would be grateful for any advice you can give.

            regards
            Gordon

            Mixed Moodle versions detected, upgrade cannot continue

            The Moodle update process has been paused because PHP scripts from at least two major versions of Moodle have been detected in the Moodle directory.

            This can cause significant problems later, so in order to continue you must ensure that the Moodle directory contains only files for a single version of Moodle.

            The recommended way to clean your Moodle directory is as follows:

            • rename the current Moodle directory to "moodle_old"
            • create a new Moodle directory containing only files from either a standard Moodle package download, or from the Moodle CVS or GIT repositories
            • move the original config.php file and any non-standard plugins from the "moodle_old" directory to the new Moodle directory

            When you have a clean Moodle directory, refresh this page to resume the Moodle update process.

            This warning is often caused by unzipping a standard Moodle package over a previous version of Moodle. While this is OK for minor upgrades, it is strongly discouraged for major Moodle upgrades.

            This warning can also be caused by an incomplete checkout or update operation from a CVS or GIT repository, in which case you may just have to wait for the operation complete, or perhaps run the appropriate clean up command and retry the operation.

            Show
            xxxxxxx Gordon Bateson added a comment - - edited Thank you Petr. I appreciate your taking the time to consider this issue with me, and for suggesting a revision to the strings which appear when mixed Moodle versions are detected. Below is the my proposal for the revised "upgradestalefiles" and "upgradestalefilesinfo" strings in the "lang/en/admin.php" lang file. I am not sure who to present these revised strings to for consideration, and I would be grateful for any advice you can give. regards Gordon Mixed Moodle versions detected, upgrade cannot continue The Moodle update process has been paused because PHP scripts from at least two major versions of Moodle have been detected in the Moodle directory. This can cause significant problems later, so in order to continue you must ensure that the Moodle directory contains only files for a single version of Moodle. The recommended way to clean your Moodle directory is as follows: rename the current Moodle directory to "moodle_old" create a new Moodle directory containing only files from either a standard Moodle package download, or from the Moodle CVS or GIT repositories move the original config.php file and any non-standard plugins from the "moodle_old" directory to the new Moodle directory When you have a clean Moodle directory, refresh this page to resume the Moodle update process. This warning is often caused by unzipping a standard Moodle package over a previous version of Moodle. While this is OK for minor upgrades, it is strongly discouraged for major Moodle upgrades. This warning can also be caused by an incomplete checkout or update operation from a CVS or GIT repository, in which case you may just have to wait for the operation complete, or perhaps run the appropriate clean up command and retry the operation.
            Hide
            skodak Petr Skoda added a comment - - edited

            reopening and changing title to reflect our conclusion that the text is confusing, thanks for the proposal

            Show
            skodak Petr Skoda added a comment - - edited reopening and changing title to reflect our conclusion that the text is confusing, thanks for the proposal
            Hide
            skodak Petr Skoda added a comment -

            I am going to bring this forward on Monday during HQ meeting.

            Show
            skodak Petr Skoda added a comment - I am going to bring this forward on Monday during HQ meeting.
            Hide
            xxxxxxx Gordon Bateson added a comment - - edited

            +1 for the new issue title, and thanks for including the new strings in the discussions at HQ. Hope that goes well !

            Show
            xxxxxxx Gordon Bateson added a comment - - edited +1 for the new issue title, and thanks for including the new strings in the discussions at HQ. Hope that goes well !
            Hide
            poltawski Dan Poltawski added a comment -

            Hi,

            I just ran into this. It would be helpful if the installer could tell the admin what the files it detected were. I am a Moodle developer and had to do the following to work otu what was going on:

            diff --git a/lib/upgradelib.php b/lib/upgradelib.php
            index aa6ee8f..c12256d 100644
            --- a/lib/upgradelib.php
            +++ b/lib/upgradelib.php
            @@ -338,6 +338,7 @@ function upgrade_stale_php_files_present() {
             
                 foreach ($someexamplesofremovedfiles as $file) {
                     if (file_exists($CFG->dirroot.$file)) {
            +            error_log($file);
                         return true;
                     }
                 }

            Show
            poltawski Dan Poltawski added a comment - Hi, I just ran into this. It would be helpful if the installer could tell the admin what the files it detected were. I am a Moodle developer and had to do the following to work otu what was going on: diff --git a/lib/upgradelib.php b/lib/upgradelib.php index aa6ee8f..c12256d 100644 --- a/lib/upgradelib.php +++ b/lib/upgradelib.php @@ -338,6 +338,7 @@ function upgrade_stale_php_files_present() { foreach ($someexamplesofremovedfiles as $file) { if (file_exists($CFG->dirroot.$file)) { + error_log($file); return true; } }
            Hide
            skodak Petr Skoda added a comment -

            Dan, again, listing of the files solves nothing, quite the opposite, you need to start from fresh checkout.

            Show
            skodak Petr Skoda added a comment - Dan, again, listing of the files solves nothing, quite the opposite, you need to start from fresh checkout.
            Hide
            poltawski Dan Poltawski added a comment -

            For the record, I agree with Petr that we shouldn't be displaying these files, but the message is very similar to other 'out of date plugins' type errors we've seen in the past.

            Show
            poltawski Dan Poltawski added a comment - For the record, I agree with Petr that we shouldn't be displaying these files, but the message is very similar to other 'out of date plugins' type errors we've seen in the past.
            Hide
            skodak Petr Skoda added a comment -

            Ah, right, I think you found the reason why it is confusing.

            Show
            skodak Petr Skoda added a comment - Ah, right, I think you found the reason why it is confusing.
            Hide
            xxxxxxx Gordon Bateson added a comment -

            Dan, could you shed any light on how your Moodle directory came to contain files from multiple Moodle versions? Would that be covered by either (1) unzipping a Moodle zip file over a previous version of Moodle, or (2) an incomplete CVS update, or (3) something else?

            Show
            xxxxxxx Gordon Bateson added a comment - Dan, could you shed any light on how your Moodle directory came to contain files from multiple Moodle versions? Would that be covered by either (1) unzipping a Moodle zip file over a previous version of Moodle, or (2) an incomplete CVS update, or (3) something else?
            Hide
            poltawski Dan Poltawski added a comment -

            In my case it was a bad git state, very visible if you do git status (hadn't run git clean -fd after cancelling a git checkout command).

            Show
            poltawski Dan Poltawski added a comment - In my case it was a bad git state, very visible if you do git status (hadn't run git clean -fd after cancelling a git checkout command).
            Hide
            xxxxxxx Gordon Bateson added a comment - - edited

            > a bad git state

            I see. In which case, what do you think of the modifying the last part of the suggested rewording of the message (given earlier in this thread) to the following:

            This warning can also be caused by an incomplete checkout or update operation from a CVS, SVN or GIT repository, in which case you may just have to wait for the operation complete, or perhaps run the appropriate clean up command and retry the operation.

            Show
            xxxxxxx Gordon Bateson added a comment - - edited > a bad git state I see. In which case, what do you think of the modifying the last part of the suggested rewording of the message (given earlier in this thread) to the following: This warning can also be caused by an incomplete checkout or update operation from a CVS, SVN or GIT repository, in which case you may just have to wait for the operation complete, or perhaps run the appropriate clean up command and retry the operation.
            Hide
            xxxxxxx Gordon Bateson added a comment -

            I have modified the proposed revision to the "upgradestalefilesinfo" message to include references to the GIT repository situation that Dan came across.

            Show
            xxxxxxx Gordon Bateson added a comment - I have modified the proposed revision to the "upgradestalefilesinfo" message to include references to the GIT repository situation that Dan came across.
            Hide
            poltawski Dan Poltawski added a comment -

            Yep, makes sense.

            Show
            poltawski Dan Poltawski added a comment - Yep, makes sense.
            Hide
            skodak Petr Skoda added a comment -

            To integrators: I have used markdown to make this consistent with help strings, feel free to improve it.

            GOrdon: thanks a lot for the patch and especially the proposed solution!

            Show
            skodak Petr Skoda added a comment - To integrators: I have used markdown to make this consistent with help strings, feel free to improve it. GOrdon: thanks a lot for the patch and especially the proposed solution!
            Hide
            xxxxxxx Gordon Bateson added a comment - - edited

            Great news - and thanks for making the solution a reality Petr!

            Show
            xxxxxxx Gordon Bateson added a comment - - edited Great news - and thanks for making the solution a reality Petr!
            Hide
            poltawski Dan Poltawski added a comment -

            Thanks Petr/Gordon. I've integrated this now.

            Show
            poltawski Dan Poltawski added a comment - Thanks Petr/Gordon. I've integrated this now.
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Message is more explanatory now.

            Thanks for fixing this, Petr

            Show
            rajeshtaneja Rajesh Taneja added a comment - Message is more explanatory now. Thanks for fixing this, Petr
            Hide
            poltawski Dan Poltawski added a comment -

            Congratulations!

            You've made it into the weekly release!

            Thanks for your contribution - here are some random drummers to keep you inspired for the next week!
            http://www.youtube.com/watch?v=_QhpHUmVCmY

            Show
            poltawski Dan Poltawski added a comment - Congratulations! You've made it into the weekly release! Thanks for your contribution - here are some random drummers to keep you inspired for the next week! http://www.youtube.com/watch?v=_QhpHUmVCmY

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  10/Sep/12