Moodle
  1. Moodle
  2. MDL-30621

Warning when attempting to upload users on a new site

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2
    • Fix Version/s: 2.2.1
    • Component/s: Installation
    • Labels:
    • Testing Instructions:
      Hide

      1/ clear error log in php
      2/ upload users
      3/ make sure no errors logged and not .htaccess created in temp dir
      4/ move tempdir to different location via $CFG->tempdir
      5/ upload again
      6/ no error expected, .htaccess should be created

      Show
      1/ clear error log in php 2/ upload users 3/ make sure no errors logged and not .htaccess created in temp dir 4/ move tempdir to different location via $CFG->tempdir 5/ upload again 6/ no error expected, .htaccess should be created
    • Affected Branches:
      MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      w51_MDL-30621_m23_protectdir
    • Rank:
      33429

      Description

      I've hit this a couple of times now.
      When I install a new site the very first thing I do is upload users, the past couple of times I have done this I have received the following when I first visit the upload users script.

      Warning: fopen(/private/var/www/moodledata/integration/2_3/temp/.htaccess): failed to open stream: No such file or directory in /private/var/www/integration/lib/setuplib.php on line 1157

      It doesn't appear to be anything critical a quick inspection after the upload and I see that the .htaccess file has now been created so no probs.

      However annoying.

      To replicate:

      1. Install a new site (MUST be an empty moodledata directory)
      2. Proceed till you get to the front page
      3. Browse to Admin settings > Users > Accounts > Upload users
      4. You'll see the notice at the top.

      Cheers
      Sam

        Activity

        Hide
        Petr Škoda added a comment -

        Thanks a lot for the report, I have changed the code to create the .htaccess to be create only if non-stadnard cache or temp dir used and made sure the temp/cache dir is created before we try to add the htaccess file.

        Show
        Petr Škoda added a comment - Thanks a lot for the report, I have changed the code to create the .htaccess to be create only if non-stadnard cache or temp dir used and made sure the temp/cache dir is created before we try to add the htaccess file.
        Hide
        Sam Hemelryk added a comment -

        Hi Petr,

        Before this gets integrated I just want to check whether removing the .htaccess files from the default temp and cache directory is a good idea?
        I quickly asked Martin and he informed me it was good general practise to include those anytime there is a possibility that user uploaded files end up there.
        I know that code has to specifically move files into those two directories but would it be a good idea/practice to still include those .htaccess files?

        Code itself looks perfect just whether we should be removing those .htaccess files or not.

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Hi Petr, Before this gets integrated I just want to check whether removing the .htaccess files from the default temp and cache directory is a good idea? I quickly asked Martin and he informed me it was good general practise to include those anytime there is a possibility that user uploaded files end up there. I know that code has to specifically move files into those two directories but would it be a good idea/practice to still include those .htaccess files? Code itself looks perfect just whether we should be removing those .htaccess files or not. Cheers Sam
        Hide
        Petr Škoda added a comment -

        yes, the removal was intentional - it was introduced accidentally together with the new temp and cache dir, it is necessary only if they are outside of our dataroot because otherwise our intentionally invalid .htaccess in dataroot must work fine, if not we would have s huge problem.

        Show
        Petr Škoda added a comment - yes, the removal was intentional - it was introduced accidentally together with the new temp and cache dir, it is necessary only if they are outside of our dataroot because otherwise our intentionally invalid .htaccess in dataroot must work fine, if not we would have s huge problem.
        Hide
        Petr Škoda added a comment -

        Ahhh, now I understand. I have added extra protection of dataroot when standard locations used, thanks!!

        Show
        Petr Škoda added a comment - Ahhh, now I understand. I have added extra protection of dataroot when standard locations used, thanks!!
        Hide
        Sam Hemelryk added a comment -

        Thanks Petr - this has been integrated now

        Show
        Sam Hemelryk added a comment - Thanks Petr - this has been integrated now
        Hide
        Gerard Caulfield added a comment -

        This patch doesn't check the return value of check_dir_exists(). Is this intentional?

        Show
        Gerard Caulfield added a comment - This patch doesn't check the return value of check_dir_exists(). Is this intentional?
        Hide
        Petr Škoda added a comment -

        yes, the error would be detected by later call to make_writable_directory(), in any case if admin does not set up proper permissions moodle will stop working

        Show
        Petr Škoda added a comment - yes, the error would be detected by later call to make_writable_directory(), in any case if admin does not set up proper permissions moodle will stop working
        Hide
        Gerard Caulfield added a comment -

        The replication instructions were incomplete and the testing instructions would have passed regardless of whether the fix was applied or not. The bug was not able to be reproduced until I used Sam's config.php (which is quite customised).

        Sam suspects and I agree that this problem will only occur if you enable debugging via the config. Setting debugging through the Moodle admin interface leads to the problem going away so you have to set it through the config and go straight to the upload users page after install. Although I didn't narrow down Sam's config to be sure exactly what was causing the issue.

        $CFG->debug = 38911;
        $CFG->debugdisplay = 1;
        

        Either way the patch addresses this issue.

        Side note: loading the upload users page will actually lead to the required file being created and the error disappearing.

        Test passed

        Show
        Gerard Caulfield added a comment - The replication instructions were incomplete and the testing instructions would have passed regardless of whether the fix was applied or not. The bug was not able to be reproduced until I used Sam's config.php (which is quite customised). Sam suspects and I agree that this problem will only occur if you enable debugging via the config. Setting debugging through the Moodle admin interface leads to the problem going away so you have to set it through the config and go straight to the upload users page after install. Although I didn't narrow down Sam's config to be sure exactly what was causing the issue. $CFG->debug = 38911; $CFG->debugdisplay = 1; Either way the patch addresses this issue. Side note: loading the upload users page will actually lead to the required file being created and the error disappearing. Test passed
        Hide
        Petr Škoda added a comment -

        Hello,

        I believe the description and the testing instructions are sufficient - any "warning" related problem is not supposed to be visible on production sites, testers should always test both in normal and debug mode (because theoretically the results may sometimes differ). In any case all developers and testers MUST watch content of their php error log.

        Thanks for the testing.

        Show
        Petr Škoda added a comment - Hello, I believe the description and the testing instructions are sufficient - any "warning" related problem is not supposed to be visible on production sites, testers should always test both in normal and debug mode (because theoretically the results may sometimes differ). In any case all developers and testers MUST watch content of their php error log. Thanks for the testing.
        Hide
        Gerard Caulfield added a comment -

        Actually you make a good point Petr, I guess it wasn't the debugging config setting as I was tailing the error log every time as your test instructions say to do. As I mentioned I couldn't be sure that it was debug that was resulting in the error displaying. I could not get the error to trigger until I switched to Sam's customized config. There were many difference from the default config, so I guess it was something else. However following the test did not produce the error on the unpatched branch.

        Show
        Gerard Caulfield added a comment - Actually you make a good point Petr, I guess it wasn't the debugging config setting as I was tailing the error log every time as your test instructions say to do. As I mentioned I couldn't be sure that it was debug that was resulting in the error displaying. I could not get the error to trigger until I switched to Sam's customized config. There were many difference from the default config, so I guess it was something else. However following the test did not produce the error on the unpatched branch.
        Hide
        Gerard Caulfield added a comment -

        ^unless I cleared the data directory post install.

        Show
        Gerard Caulfield added a comment - ^unless I cleared the data directory post install.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Whoever decided one week was worth 14 days had really one bad idea. Anyway, the nightmare is over, so thanks for your, once again, amazing contributions. Many, many thanks!

        Now... disconnect, relax and enjoy the next days, yay!

        Closing...ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Whoever decided one week was worth 14 days had really one bad idea. Anyway, the nightmare is over, so thanks for your, once again, amazing contributions. Many, many thanks! Now... disconnect, relax and enjoy the next days, yay! Closing...ciao

          People

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

            Dates

            • Created:
              Updated:
              Resolved: