Moodle
  1. Moodle
  2. MDL-41337

Can't delete the last file from filearea (under some circumstances)

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.4.6, 2.5.2, 2.6
    • Fix Version/s: 2.4.7, 2.5.3
    • Component/s: Files API
    • Labels:
    • Testing Instructions:
      Hide

      For 2.5 and 2.6:
      Precisely repeat the following instructions, first on current stable/master (make sure there is an error and now you have corrupted DB), THEN UPGRADE, then check again for the same course and for the new course. Make sure there is no error.

      1. Create new course, leave all fields default/empty. SAVE
      2. Edit course, add an image file in summary files. SAVE
      3. Edit course, change nothing. SAVE
      4. Edit course, delete the file from summary files. SAVE
      5. Edit course again - summary files should be empty

      For 2.4

      1. Edit a course section (or course settings, or any form with texteditor)
      2. Add an image to the editor. Save the form
      3. Edit the same form again, change nothing. Save
      4. Turn off javascript (to have access to filearea attached to texteditor)
      5. Edit the same form again, delete the embedded file from text area. Save
      6. Edit the same form again, make sure that file is not there.
      Show
      For 2.5 and 2.6: Precisely repeat the following instructions, first on current stable/master (make sure there is an error and now you have corrupted DB), THEN UPGRADE, then check again for the same course and for the new course. Make sure there is no error. Create new course, leave all fields default/empty. SAVE Edit course, add an image file in summary files. SAVE Edit course, change nothing. SAVE Edit course, delete the file from summary files. SAVE Edit course again - summary files should be empty For 2.4 Edit a course section (or course settings, or any form with texteditor) Add an image to the editor. Save the form Edit the same form again, change nothing. Save Turn off javascript (to have access to filearea attached to texteditor) Edit the same form again, delete the embedded file from text area. Save Edit the same form again, make sure that file is not there.
    • Workaround:
      1. Edit the course
      2. Delete the naughty file from course summary files.
      3. Add another file
      4. Save the course
      5. Edit the course
      6. Delete the file
      7. Save the course
    • Affected Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE, MOODLE_26_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE
    • Pull Master Branch:
      wip-MDL-41337-master
    • Story Points (Obsolete):
      20

      Description

      I added an image via the file uploader in the course summary to test out the feature. I didn't like the way it works, so I deleted it. And it returns as soon as I click 'save changes'. So I deleted it again. And it returns. It won't stay deleted!

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Jon Fila added a comment -

            Others having the same issue here: https://moodle.org/mod/forum/discuss.php?d=230628

            I logged in as an Administrator to delete the file, saved changes, still there.
            I turned of JavaScript so that I could see all of the files uploaded to that page then I deleted them and saved, still there.

            Show
            Jon Fila added a comment - Others having the same issue here: https://moodle.org/mod/forum/discuss.php?d=230628 I logged in as an Administrator to delete the file, saved changes, still there. I turned of JavaScript so that I could see all of the files uploaded to that page then I deleted them and saved, still there.
            Hide
            Marina Glancy added a comment -

            Thanks Sue and Jon. I tried to upload a file and then deleted it and it is gone. Could you add some screenshots and more detailed explanation of what is happenning and steps to reproduce? Also please specify the release number of your Moodle version. I try on the latest weekly release of 2.5 but there were no changes to the code in this area recently

            Show
            Marina Glancy added a comment - Thanks Sue and Jon. I tried to upload a file and then deleted it and it is gone. Could you add some screenshots and more detailed explanation of what is happenning and steps to reproduce? Also please specify the release number of your Moodle version. I try on the latest weekly release of 2.5 but there were no changes to the code in this area recently
            Hide
            Sue M. added a comment -

            1st shot is file attached is the course file showing.
            2nd & 3rd shots are of me deleting the course file.
            4th file shows the refreshed page with the file reattached.

            Show
            Sue M. added a comment - 1st shot is file attached is the course file showing. 2nd & 3rd shots are of me deleting the course file. 4th file shows the refreshed page with the file reattached.
            Hide
            Sue M. added a comment -

            I just attached files that I just took to recreate the incident. I have the latest updated version (I checked before recreating).

            Show
            Sue M. added a comment - I just attached files that I just took to recreate the incident. I have the latest updated version (I checked before recreating).
            Hide
            Sue M. added a comment -

            Just experimenting to see if I can deduce any error msgs, etc.. No error messages pop up, but here's an odd thing. I uploaded a variety of image files of all different types and sizes to various courses and they seem to randomly get stuck or not with no patterns. One png doc doesn't delete, one does. One jpg doesn't delete, another does.

            A workaround fix in case anyone wants to know it, try deleting the file that is stuck and without saving, uploading a different file. Save and then reopen to delete that course file. That worked 2 times for me... but not the third time I tried it.

            Weirdness.

            I hope that this bit of random info helps!

            Show
            Sue M. added a comment - Just experimenting to see if I can deduce any error msgs, etc.. No error messages pop up, but here's an odd thing. I uploaded a variety of image files of all different types and sizes to various courses and they seem to randomly get stuck or not with no patterns. One png doc doesn't delete, one does. One jpg doesn't delete, another does. A workaround fix in case anyone wants to know it, try deleting the file that is stuck and without saving, uploading a different file. Save and then reopen to delete that course file. That worked 2 times for me... but not the third time I tried it. Weirdness. I hope that this bit of random info helps!
            Hide
            Marina Glancy added a comment -

            Hi Sue, thanks for the screenshots. I am still unable to reproduce though.
            Can you reproduce it on some non-production site? If I give you a patch with debugging output, would you be able to apply it?

            Show
            Marina Glancy added a comment - Hi Sue, thanks for the screenshots. I am still unable to reproduce though. Can you reproduce it on some non-production site? If I give you a patch with debugging output, would you be able to apply it?
            Hide
            Sue M. added a comment -

            I don't have a non-production site, just my active production site. I don't have the time to create a non-production site and test out patches at the moment. Sorry.

            Show
            Sue M. added a comment - I don't have a non-production site, just my active production site. I don't have the time to create a non-production site and test out patches at the moment. Sorry.
            Hide
            Mary Evans added a comment - - edited

            Going by the images you uploaded you appear to have added the image twice. Once as a course image which puts the image below the title on the LEFT of the info, which can be deleted from the filemanager window/pop-up. However you also have the same image displaying in the text in the summary description. This second image needs manually removing from the summary description.

            I have just tested this in the Moodle QA test site qa.moodle.net(as broken as it is for an administrator) and works as expected!

            I think you are confusing yourself with how you think this works as opposed to how it actually works. The two images are stored in different places in Moodle. One is uploaded in the drag and drop area (which is the one you can delete) the other via the text editor which you need to remove from within the text by clicking on it and deleting it using your keyboard 'delete' button.

            Show
            Mary Evans added a comment - - edited Going by the images you uploaded you appear to have added the image twice. Once as a course image which puts the image below the title on the LEFT of the info, which can be deleted from the filemanager window/pop-up. However you also have the same image displaying in the text in the summary description. This second image needs manually removing from the summary description. I have just tested this in the Moodle QA test site qa.moodle.net(as broken as it is for an administrator) and works as expected! I think you are confusing yourself with how you think this works as opposed to how it actually works. The two images are stored in different places in Moodle. One is uploaded in the drag and drop area (which is the one you can delete) the other via the text editor which you need to remove from within the text by clicking on it and deleting it using your keyboard 'delete' button.
            Hide
            Sue M. added a comment -

            Mary,

            I first tested out the "course file" option as a way to replace the images I had placed in the text of the "course summary" in previous versions of Moodle. At that time I removed the image from the 'course summary" text box and only had the "course file" image. Which wouldn't (and still won't) delete by using the delete box as shown in the screenshots.

            However, when I took the time to reproduce the problem to provide screenshots I did not remove the "course summary" image as this is a production site and I'm trying to keep the disruptions to a minimum. So just pretend like that image isn't there as it doesn't affect the problem I am describing. If it is somehow too distracting and confuses the issue I suppose I can recreate the issue yet again and remove the "course summary" image.

            You can clearly see that the "course file" image (the one that falls underneath the course title on the left side of the screen) is still in place in the 4th image - AFTER I deleted the file. It shouldn't still be in place after I delete the file.

            If I'm somehow confusing what "delete" means (i.e. that it won't be there after I delete it), I'd love to have it cleared up!
            Thanks

            Show
            Sue M. added a comment - Mary, I first tested out the "course file" option as a way to replace the images I had placed in the text of the "course summary" in previous versions of Moodle. At that time I removed the image from the 'course summary" text box and only had the "course file" image. Which wouldn't (and still won't) delete by using the delete box as shown in the screenshots. However, when I took the time to reproduce the problem to provide screenshots I did not remove the "course summary" image as this is a production site and I'm trying to keep the disruptions to a minimum. So just pretend like that image isn't there as it doesn't affect the problem I am describing. If it is somehow too distracting and confuses the issue I suppose I can recreate the issue yet again and remove the "course summary" image. You can clearly see that the "course file" image (the one that falls underneath the course title on the left side of the screen) is still in place in the 4th image - AFTER I deleted the file. It shouldn't still be in place after I delete the file. If I'm somehow confusing what "delete" means (i.e. that it won't be there after I delete it), I'd love to have it cleared up! Thanks
            Hide
            Sue M. added a comment -

            A bit more information:

            I tried it on the production site with one of the problematic images, and the delete file option worked fine there as well. I also tried it on the demo site at http://demo.moodle.net/ and it deleted just fine.

            It looks like it is just on my site that I have this issue. I have the latest updated version of Moodle with very little modification (a theme added and Poodll). Any other information that would be helpful to you, let me know. I'm not using the course file option, so it isn't an issue for my site. But if it helps in general, let me know.

            Show
            Sue M. added a comment - A bit more information: I tried it on the production site with one of the problematic images, and the delete file option worked fine there as well. I also tried it on the demo site at http://demo.moodle.net/ and it deleted just fine. It looks like it is just on my site that I have this issue. I have the latest updated version of Moodle with very little modification (a theme added and Poodll). Any other information that would be helpful to you, let me know. I'm not using the course file option, so it isn't an issue for my site. But if it helps in general, let me know.
            Hide
            Jon Fila added a comment -

            Here's a video I made of the problem I'm experiencing: http://www.youtube.com/watch?v=OoKl2CMtiSs

            I'm logged in as a site Administrator. I didn't do it in the video, but in trying this multiple times I have tried clearing the cache as well and the video still appears after I try to delete it.

            Show
            Jon Fila added a comment - Here's a video I made of the problem I'm experiencing: http://www.youtube.com/watch?v=OoKl2CMtiSs I'm logged in as a site Administrator. I didn't do it in the video, but in trying this multiple times I have tried clearing the cache as well and the video still appears after I try to delete it.
            Hide
            Mary Evans added a comment -

            @Jon, In looking at your video I would say that is a cache problem. Have you tried the delete with Theme Designer Mode enabled in Theme settings?

            @Sue, I am not sure what's happening on your site. The only thing I can think of is that it might well be the server, such as the PHP Extention php-eaccelerator which is like a cache. Mind you I think I am grasping as straws here.

            Either way it does sound very much like a cache problem.

            Show
            Mary Evans added a comment - @Jon, In looking at your video I would say that is a cache problem. Have you tried the delete with Theme Designer Mode enabled in Theme settings? @Sue, I am not sure what's happening on your site. The only thing I can think of is that it might well be the server, such as the PHP Extention php-eaccelerator which is like a cache. Mind you I think I am grasping as straws here. Either way it does sound very much like a cache problem.
            Hide
            Sue M. added a comment -

            @Mary, When I check the Administration>Server>Environment everything is a-okay for the settings. Clearing the cache either through Moodle or on the server doesn't solve the problem. What could be the issue with PHP Extention php-eaccelerator? Are you saying it should or shouldn't be used?

            @Jon, thank you for reminding me that I'm not actually the only person having this problem! And for the video, it illustrates perfectly the problem that I am having and is much more effective than the screenshots.

            Show
            Sue M. added a comment - @Mary, When I check the Administration>Server>Environment everything is a-okay for the settings. Clearing the cache either through Moodle or on the server doesn't solve the problem. What could be the issue with PHP Extention php-eaccelerator? Are you saying it should or shouldn't be used? @Jon, thank you for reminding me that I'm not actually the only person having this problem! And for the video, it illustrates perfectly the problem that I am having and is much more effective than the screenshots.
            Hide
            Mary Evans added a comment - - edited

            Just adding some DEV's as watchers who might understand what's going on after seeing Jon video of the problem.
            http://www.youtube.com/watch?v=OoKl2CMtiSs

            I can replicate this same behaviour when trying to delete a Group image. So might be related to the filepicker/filemanager cache

            Show
            Mary Evans added a comment - - edited Just adding some DEV's as watchers who might understand what's going on after seeing Jon video of the problem. http://www.youtube.com/watch?v=OoKl2CMtiSs I can replicate this same behaviour when trying to delete a Group image. So might be related to the filepicker/filemanager cache
            Hide
            Jon Fila added a comment -

            Mary, I turned on Theme designer mode and tried to delete the file and it was still there.

            Show
            Jon Fila added a comment - Mary, I turned on Theme designer mode and tried to delete the file and it was still there.
            Hide
            Andrew Nicols added a comment -

            I'm also unable to replicate on Master, or MOODLE_25_STABLE.

            I'm removing the JS tag as I don't believe that this has anything to do with JavaScript. If anything it will be Forms related.

            Will try again with 2.5.1 in just a mo.

            Show
            Andrew Nicols added a comment - I'm also unable to replicate on Master, or MOODLE_25_STABLE. I'm removing the JS tag as I don't believe that this has anything to do with JavaScript. If anything it will be Forms related. Will try again with 2.5.1 in just a mo.
            Hide
            Andrew Nicols added a comment -

            Jon's video affirms that this is not a JS issue.

            Show
            Andrew Nicols added a comment - Jon's video affirms that this is not a JS issue.
            Hide
            Andrew Nicols added a comment -

            Unable to replicate on v2.5.1 either.

            Show
            Andrew Nicols added a comment - Unable to replicate on v2.5.1 either.
            Hide
            Andrew Nicols added a comment -

            Scrap that - replicated after I'd turned off a few developer-related settings. Tracking it back.

            Show
            Andrew Nicols added a comment - Scrap that - replicated after I'd turned off a few developer-related settings. Tracking it back.
            Hide
            Andrew Nicols added a comment -

            And now I can't stop it from replicating - handy.

            So it looks like $fs->get_area_files() isn't returning anything for the old files in file_save_draft_area_files() so we don't know there are any to remove and thus they get removed not.

            Show
            Andrew Nicols added a comment - And now I can't stop it from replicating - handy. So it looks like $fs->get_area_files() isn't returning anything for the old files in file_save_draft_area_files() so we don't know there are any to remove and thus they get removed not.
            Hide
            Andrew Nicols added a comment - - edited

            My finding show that the file is listed in $oldfiles in file_save_draft_area_files; but since there is only one file, we don't match:

            if (count($draftfiles) > 1 || count($oldfiles) > 1) {
            

            and therefore that one file is never removed.

            It's at this point I get a little sketchy on things, but AIR, you should always have a directory record (.) for the parent directory that the file is in - another course I've added a course overview file to confirms this.

            I can only imagine that it's possible to somehow get into a situation where you remove the directory record, without removing the child records, though I don't quite know how.

            Show
            Andrew Nicols added a comment - - edited My finding show that the file is listed in $oldfiles in file_save_draft_area_files; but since there is only one file, we don't match: if (count($draftfiles) > 1 || count($oldfiles) > 1) { and therefore that one file is never removed. It's at this point I get a little sketchy on things, but AIR, you should always have a directory record (.) for the parent directory that the file is in - another course I've added a course overview file to confirms this. I can only imagine that it's possible to somehow get into a situation where you remove the directory record, without removing the child records, though I don't quite know how.
            Hide
            Andrew Nicols added a comment -

            We'll have to track down under what situation this can happen.

            Show
            Andrew Nicols added a comment - We'll have to track down under what situation this can happen.
            Hide
            Mary Evans added a comment -

            It's been reported for Theme Logos too now in MDL-41702

            And also in Themes Forum this week.
            https://moodle.org/mod/forum/discuss.php?d=237959

            Show
            Mary Evans added a comment - It's been reported for Theme Logos too now in MDL-41702 And also in Themes Forum this week. https://moodle.org/mod/forum/discuss.php?d=237959
            Hide
            Jason Platts added a comment -

            I've also spotted this in module plugins as well.

            From debugging I think the following is happening:

            You add some files to the file area of whatever plugin - this adds records for the files and the 'root' directory (.) in the files table.

            You delete one of the files and save the form.

            file_save_draft_area_files() gets the saved records ($oldfiles) and new records from the draft area ($draftfiles).
            It then goes through the new records and adds those to an array ($newhashes). Anything not in that array gets deleted.

            If $options['subdirs'] is not true then directories are not include and get deleted.

            So when you are left with a single existing file record the code expects this single value in $oldfiles to be the 'root' directory so then doesn't execute the delete code.

            I think file_save_draft_area_files() shouldn't be deleting the 'root' directory record in any circumstance, so should there be checks to stop this happening?

            Show
            Jason Platts added a comment - I've also spotted this in module plugins as well. From debugging I think the following is happening: You add some files to the file area of whatever plugin - this adds records for the files and the 'root' directory (.) in the files table. You delete one of the files and save the form. file_save_draft_area_files() gets the saved records ($oldfiles) and new records from the draft area ($draftfiles). It then goes through the new records and adds those to an array ($newhashes). Anything not in that array gets deleted. If $options ['subdirs'] is not true then directories are not include and get deleted. So when you are left with a single existing file record the code expects this single value in $oldfiles to be the 'root' directory so then doesn't execute the delete code. I think file_save_draft_area_files() shouldn't be deleting the 'root' directory record in any circumstance, so should there be checks to stop this happening?
            Hide
            Marina Glancy added a comment -

            Thank you very much Jason, now it all makes sense. I will provide a patch shortly

            Show
            Marina Glancy added a comment - Thank you very much Jason, now it all makes sense. I will provide a patch shortly
            Hide
            Mary Evans added a comment -

            @Andrew,
            If it's any help Petr Skoda added the ability to upload images in themes/plugins in MDL-35434 in April.

            Show
            Mary Evans added a comment - @Andrew, If it's any help Petr Skoda added the ability to upload images in themes/plugins in MDL-35434 in April.
            Hide
            Marina Glancy added a comment - - edited

            the patch suggested by Andrew and Jason works but it's not the correct solution to the problem.
            Root directory should always be present and under the described conditions it is created when file is uploaded for the first time but then gets deleted when the same file is uploaded in another filearea. I've been trying to track this problem half day yesterday and did not succeed so far. Hopefully will come up with some solution today.

            Show
            Marina Glancy added a comment - - edited the patch suggested by Andrew and Jason works but it's not the correct solution to the problem. Root directory should always be present and under the described conditions it is created when file is uploaded for the first time but then gets deleted when the same file is uploaded in another filearea. I've been trying to track this problem half day yesterday and did not succeed so far. Hopefully will come up with some solution today.
            Hide
            Marina Glancy added a comment -

            Hi Petr, I finally found who was responsible for deleting the root directory and corrected it.

            But now we need some procedure to restore all accidentally deleted root directory.

            I have two options:
            Option 1. Create an upgrade script looking for all missing root directories, something like

            select distinct f1.contextid, f1.component, f1.filearea, f1.itemid
            from mdl_files f1 left join mdl_files f2
            on f1.contextid=f2.contextid 
            and f1.component=f2.component
            and f1.filearea=f2.filearea
            and f1.itemid=f2.itemid
            and f2.filename = '.'
            and f2.filepath = '/'
            WHERE
            (f1.component <> 'user' or f1.filearea <> 'draft')
            and f2.id is null
            

            but I'm afraid it would take too long on big databases

            Option 2. Build the check/fix into file_storage::get_area_files() and it will eventually fix all file areas. The downside of it - what if somebody retrieves files with other method?

            What do you think?

            Show
            Marina Glancy added a comment - Hi Petr, I finally found who was responsible for deleting the root directory and corrected it. But now we need some procedure to restore all accidentally deleted root directory. I have two options: Option 1. Create an upgrade script looking for all missing root directories, something like select distinct f1.contextid, f1.component, f1.filearea, f1.itemid from mdl_files f1 left join mdl_files f2 on f1.contextid=f2.contextid and f1.component=f2.component and f1.filearea=f2.filearea and f1.itemid=f2.itemid and f2.filename = '.' and f2.filepath = '/' WHERE (f1.component <> 'user' or f1.filearea <> 'draft') and f2.id is null but I'm afraid it would take too long on big databases Option 2. Build the check/fix into file_storage::get_area_files() and it will eventually fix all file areas. The downside of it - what if somebody retrieves files with other method? What do you think?
            Hide
            Marina Glancy added a comment -

            Petr, I created an issue MDL-41909 about missing check for parent folder in stored_file::update() but it is not related to this bug

            Show
            Marina Glancy added a comment - Petr, I created an issue MDL-41909 about missing check for parent folder in stored_ file::update( ) but it is not related to this bug
            Hide
            Marina Glancy added a comment -

            Just to mention that the same happens in 2.4 for fileareas where subdirs are not allowed. It's not that obvious there because they are mostly fileareas attached to text editors which don't have functionality to remove files anyway.

            Show
            Marina Glancy added a comment - Just to mention that the same happens in 2.4 for fileareas where subdirs are not allowed. It's not that obvious there because they are mostly fileareas attached to text editors which don't have functionality to remove files anyway.
            Hide
            Petr Skoda added a comment -

            Hi, the only faster way I can think of is to create a temporary table (or even real table) and bulk insert the result of your query there and then second bulk insert back into the files table and then drop the temp table. We do something similar in contexts. It is also possible to optimise for specific databases only where you could do bulk insert without the other table. Eloy would sure know more about this.

            I guess the Option 2 is not good enough.

            Show
            Petr Skoda added a comment - Hi, the only faster way I can think of is to create a temporary table (or even real table) and bulk insert the result of your query there and then second bulk insert back into the files table and then drop the temp table. We do something similar in contexts. It is also possible to optimise for specific databases only where you could do bulk insert without the other table. Eloy would sure know more about this. I guess the Option 2 is not good enough.
            Hide
            Eloy Lafuente (stronk7) added a comment - - edited

            Hi, some comments:

            1) If I'm not wrong this is a regression caused by MDL-39177, so it requires fixing also in all (24, 25 and master) supported versions.

            2) I think the query will be pretty fast that way. No subqueries involved, just the inequalities will penalize it. The only point about the query is that I'm not 100% sold about taking 'user' file areas out from the equation. It's possible to have 'user' areas out there (contrib) suffering the same problem IMO.

            3) The main problem is that, after picking the candidates, you cannot insert them straight because some of the columns (hashes) require calculations (pathnamehash and userid, if I'm not wrong). So having a temp table won't help much in this case. The number of insert will be N, no way to avoid that.

            4) So I think a typical progress bar, with your query (without excluding 'user' file areas and adding the userid column) in a recordset and then calculating the hashes and inserting 1 by 1 is ok. We have executed longer than that processes before. I think there is a good example in current upgrade code related to allowed modules. Don't forget to "give it more time" at some intervals.

            Ciao

            Edited, for reference: In a real site with 40.000 records in mdl_files (mysql) the query was executed in 0.3 seconds and it detected 5 orphaned items (5 glossary entries).

            Edited2: PS: Some unit-test covering the case would be great.

            Show
            Eloy Lafuente (stronk7) added a comment - - edited Hi, some comments: 1) If I'm not wrong this is a regression caused by MDL-39177 , so it requires fixing also in all (24, 25 and master) supported versions. 2) I think the query will be pretty fast that way. No subqueries involved, just the inequalities will penalize it. The only point about the query is that I'm not 100% sold about taking 'user' file areas out from the equation. It's possible to have 'user' areas out there (contrib) suffering the same problem IMO. 3) The main problem is that, after picking the candidates, you cannot insert them straight because some of the columns (hashes) require calculations (pathnamehash and userid, if I'm not wrong). So having a temp table won't help much in this case. The number of insert will be N, no way to avoid that. 4) So I think a typical progress bar, with your query (without excluding 'user' file areas and adding the userid column) in a recordset and then calculating the hashes and inserting 1 by 1 is ok. We have executed longer than that processes before. I think there is a good example in current upgrade code related to allowed modules. Don't forget to "give it more time" at some intervals. Ciao Edited, for reference: In a real site with 40.000 records in mdl_files (mysql) the query was executed in 0.3 seconds and it detected 5 orphaned items (5 glossary entries). Edited2: PS: Some unit-test covering the case would be great.
            Hide
            Marina Glancy added a comment -

            Hi Eloy

            1) no, it is much older MDL-19002 : https://github.com/moodle/moodle/commit/a08171c5c2c1a48ea5fc6e5c1b6fe9503437c9e0#L2L276
            2) it takes user-draft out, not all user areas. IMHO there is no need to fix user draft areas during upgrade, they are not going to be used any more
            3) ok
            4) do you know any examples of progress bars for particular update script step? (I'd like to copy-paste )

            Thanks for review!

            Show
            Marina Glancy added a comment - Hi Eloy 1) no, it is much older MDL-19002 : https://github.com/moodle/moodle/commit/a08171c5c2c1a48ea5fc6e5c1b6fe9503437c9e0#L2L276 2) it takes user-draft out, not all user areas. IMHO there is no need to fix user draft areas during upgrade, they are not going to be used any more 3) ok 4) do you know any examples of progress bars for particular update script step? (I'd like to copy-paste ) Thanks for review!
            Hide
            Jason Platts added a comment -

            FYI: Running the SQL above on a very large files table (1.8 Million records!) took 65 Seconds and returned 0 results.
            (Which suggests this issue isn't common in our system; I ran the query on a dev server where this issue had been spotted and that did return results as expected)

            Show
            Jason Platts added a comment - FYI: Running the SQL above on a very large files table (1.8 Million records!) took 65 Seconds and returned 0 results. (Which suggests this issue isn't common in our system; I ran the query on a dev server where this issue had been spotted and that did return results as expected)
            Hide
            Eloy Lafuente (stronk7) added a comment - - edited

            1) aha. I did not look to the trailing diff of https://github.com/moodle/moodle/commit/32495c06, so the problem was there already, yup. Still it requires fix in all supported.
            2) Well, I read OR not AND in your query, so it's excluding all component = user areas (profile, icon, private, backup, profile…). Not only draft areas.
            3) ok
            4) There is one in current upgrade (master) code. Look for "progress" or "upgrade_set_timeout". A call to it at the beginning will be necessary, based in Jason's info, thanks! Say 60*20 or so. And then, at intervals within the loop, just in case there are many.

            Ciao

            Show
            Eloy Lafuente (stronk7) added a comment - - edited 1) aha. I did not look to the trailing diff of https://github.com/moodle/moodle/commit/32495c06 , so the problem was there already, yup. Still it requires fix in all supported. 2) Well, I read OR not AND in your query, so it's excluding all component = user areas (profile, icon, private, backup, profile…). Not only draft areas. 3) ok 4) There is one in current upgrade (master) code. Look for "progress" or "upgrade_set_timeout". A call to it at the beginning will be necessary, based in Jason's info, thanks! Say 60*20 or so. And then, at intervals within the loop, just in case there are many. Ciao
            Hide
            Marina Glancy added a comment -

            Jason, thanks for testing.
            It surprised me though that it returned no results on the DB where you had this problem. Would you be so kind to help us to trace it. Can you please execute the following query on the course where you are unable to delete a course summary file. I would expect it to return a row for actual file and no row for root directory:

            SELECT f.*
            FROM mdl_files f
            JOIN mdl_context c ON c.contextlevel = 50
            AND f.contextid = c.id
            WHERE c.instanceid = <COURSEID>
            AND component = 'course'
            AND filearea = 'overviewfiles'
            

            Show
            Marina Glancy added a comment - Jason, thanks for testing. It surprised me though that it returned no results on the DB where you had this problem. Would you be so kind to help us to trace it. Can you please execute the following query on the course where you are unable to delete a course summary file. I would expect it to return a row for actual file and no row for root directory: SELECT f.* FROM mdl_files f JOIN mdl_context c ON c.contextlevel = 50 AND f.contextid = c.id WHERE c.instanceid = <COURSEID> AND component = 'course' AND filearea = 'overviewfiles'
            Hide
            Jason Platts added a comment -

            Sorry Marina, Just to be clear on my information - on the DB where I did experience this problem your query did return results. On our live system it didn't - but we haven't had any reports of this problem on there.

            Show
            Jason Platts added a comment - Sorry Marina, Just to be clear on my information - on the DB where I did experience this problem your query did return results. On our live system it didn't - but we haven't had any reports of this problem on there.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Forget my 2) above, I'm idiot, slept and old. Sorry for the noise, Marina.

            Show
            Eloy Lafuente (stronk7) added a comment - Forget my 2) above, I'm idiot, slept and old. Sorry for the noise, Marina.
            Hide
            Marina Glancy added a comment -

            Thanks Jason, I misread your previous comment.
            Thanks Eloy for comments.
            Will try to implement it all on the weekend so it gets into integration on Monday.

            Show
            Marina Glancy added a comment - Thanks Jason, I misread your previous comment. Thanks Eloy for comments. Will try to implement it all on the weekend so it gets into integration on Monday.
            Hide
            Petr Skoda added a comment -

            +1 for integration, I believe the patch is correct

            Show
            Petr Skoda added a comment - +1 for integration, I believe the patch is correct
            Hide
            Michael de Raadt added a comment -

            Carrying over to the next sprint.

            Show
            Michael de Raadt added a comment - Carrying over to the next sprint.
            Hide
            moodle.com added a comment -

            Removing this issue from the current sprint as Marina is not currently working as part of the BACKEND team. She may continue working on the issue independently.

            Show
            moodle.com added a comment - Removing this issue from the current sprint as Marina is not currently working as part of the BACKEND team. She may continue working on the issue independently.
            Hide
            Marina Glancy added a comment -

            Submitting for integration, there were plenty of reviews already.

            Show
            Marina Glancy added a comment - Submitting for integration, there were plenty of reviews already.
            Hide
            Damyon Wiese added a comment -

            Thanks Marina,

            Integrated to 24, 25 and master.

            I did a bunch of testing on this in integration including the testing instructions and some "delete from mdl_files where filename = '.'" tests. All passed.

            I'll do some quick SQL Server/Oracle tests and then I'll be happy to pass it completely.

            Show
            Damyon Wiese added a comment - Thanks Marina, Integrated to 24, 25 and master. I did a bunch of testing on this in integration including the testing instructions and some "delete from mdl_files where filename = '.'" tests. All passed. I'll do some quick SQL Server/Oracle tests and then I'll be happy to pass it completely.
            Hide
            Damyon Wiese added a comment -

            No errors on sql server or oracle. Passing this test.

            Show
            Damyon Wiese added a comment - No errors on sql server or oracle. Passing this test.
            Hide
            Damyon Wiese added a comment -

            Thanks!

            Show
            Damyon Wiese added a comment - Thanks!
            Hide
            Dan Poltawski added a comment -

            Congratulations - this issue has been included in Moodle and is now available on our git mirrors and shortly will become available on the download servers shortly.

            Show
            Dan Poltawski added a comment - Congratulations - this issue has been included in Moodle and is now available on our git mirrors and shortly will become available on the download servers shortly.

              People

              • Votes:
                4 Vote for this issue
                Watchers:
                14 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: