Moodle
  1. Moodle
  2. MDL-38314

Manage repositories error when the context of an instance is missing

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.4.1, 2.5
    • Fix Version/s: 2.4.6, 2.5.1, 2.6
    • Component/s: Repositories
    • Labels:
    • Testing Instructions:
      Hide

      Upgrade test

      This upgrade test has to be done on all supported DB engines

      1. Enable Flickr Public repository
      2. Create a User instance of that repository in a new user account
      3. Create a Course instance of that repository in a new course
      4. Delete the user
      5. Delete the course
      6. Visit Home / Site administration / Plugins / Repositories / Manage repositories
        • Make sure you SEE errors
      7. Upgrade to the latest
      8. Visit Home / Site administration / Plugins / Repositories / Manage repositories
        • Make sure you DO NOT see any errors

      Test pre-requisites

      • Enable the File System repository with Course/User instances allowed
      • Create the folders
        • $dataroot/repository/course
        • $dataroot/repository/user
      • Place a different picture in each folder
      • Create a new course (C1)
      • Create a new course (C2)
      • Create a new user (U1)
      • Make U1 an admin so it has all the capabilities

      Test preparation

      1. Login as U1
      2. Go to C1
      3. Navigate to Home / Courses / Miscellaneous / C1 / Repositories / Course repositories
      4. Create a File System instance using the folder course (call it FS Course)
      5. Add a file module to the course (call it 'Please pick me')
        • As content, place a alias/shortcut of the image in FS Course
      1. Navigate to Home / My profile / Repositories / U1 / Repositories
      2. Create a File System instance using the folder user (call it FS User)
      1. Navigate to C2
      2. Create a file activity called 'Reference to User repo'
        • As a content, place a alias/shortcut of the image in FS User
      3. Create another file activity called 'Reference to Course repo'
        • As a content, browse 'Server files' up to the course C1
        • Pick an alias/shortcut of the image in 'Please pick me' file module.

      Test steps

      1. Login as admin
      2. Go to C2 and confirm that both file resources contain alias/shortcuts
        • One to FS User
        • Another one to FS Course
      3. Delete the user U1
        • Make sure the FS User entry in mdl_repository_instances is deleted
        • Make sure the file in module 'Reference to a User repo' in C2 is now a real copy
      4. Delete the course C1
        • Make sure the FS Course entry in mdl_repository_instances is deleted
        • Make sure the file in module 'Reference to a Course repo' in C2 is now a real copy
      5. Visit Home / Site administration / Plugins / Repositories / Manage repositories
        • Make sure you don't see any errors
      Show
      Upgrade test This upgrade test has to be done on all supported DB engines Enable Flickr Public repository Create a User instance of that repository in a new user account Create a Course instance of that repository in a new course Delete the user Delete the course Visit Home / Site administration / Plugins / Repositories / Manage repositories Make sure you SEE errors Upgrade to the latest Visit Home / Site administration / Plugins / Repositories / Manage repositories Make sure you DO NOT see any errors Test pre-requisites Enable the File System repository with Course/User instances allowed Create the folders $dataroot/repository/course $dataroot/repository/user Place a different picture in each folder Create a new course (C1) Create a new course (C2) Create a new user (U1) Make U1 an admin so it has all the capabilities Test preparation Login as U1 Go to C1 Navigate to Home / Courses / Miscellaneous / C1 / Repositories / Course repositories Create a File System instance using the folder course (call it FS Course) Add a file module to the course (call it 'Please pick me') As content, place a alias/shortcut of the image in FS Course Navigate to Home / My profile / Repositories / U1 / Repositories Create a File System instance using the folder user (call it FS User) Navigate to C2 Create a file activity called 'Reference to User repo' As a content, place a alias/shortcut of the image in FS User Create another file activity called 'Reference to Course repo' As a content, browse 'Server files' up to the course C1 Pick an alias/shortcut of the image in 'Please pick me' file module. Test steps Login as admin Go to C2 and confirm that both file resources contain alias/shortcuts One to FS User Another one to FS Course Delete the user U1 Make sure the FS User entry in mdl_repository_instances is deleted Make sure the file in module 'Reference to a User repo' in C2 is now a real copy Delete the course C1 Make sure the FS Course entry in mdl_repository_instances is deleted Make sure the file in module 'Reference to a Course repo' in C2 is now a real copy Visit Home / Site administration / Plugins / Repositories / Manage repositories Make sure you don't see any errors
    • Workaround:
      Hide

      From chat with Eloy:

      1) SELECT contextid FROM mdl_repository_instances
      WHERE NOT EXISTS (SELECT 'x' FROM mdl_context WHERE id = contextid);

      2) That's the list of orphaned repositories in the site

      3) DELETE FROM mdl_repository_instances WHERE contextid = xxxxx

      note that the 1037 should appear in the list produced by 1).

      Show
      From chat with Eloy: 1) SELECT contextid FROM mdl_repository_instances WHERE NOT EXISTS (SELECT 'x' FROM mdl_context WHERE id = contextid); 2) That's the list of orphaned repositories in the site 3) DELETE FROM mdl_repository_instances WHERE contextid = xxxxx note that the 1037 should appear in the list produced by 1).
    • Affected Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE, MOODLE_26_STABLE
    • Pull from Repository:
    • Pull 2.4 Branch:
    • Pull 2.5 Branch:
    • Pull Master Branch:
      MDL-38314-master
    • Rank:
      48179
    • Sprint:
      BACKEND Sprint 2

      Description

      In admin/repository.php:

      Can not find data record in database table context.

      Debug info: SELECT * FROM

      {context}

      WHERE id = ?
      [array (
      0 => '1037',
      )]
      Error code: invalidrecord
      Stack trace:

      line 1357 of /lib/dml/moodle_database.php: dml_missing_record_exception thrown
      line 1333 of /lib/dml/moodle_database.php: call to moodle_database->get_record_select()
      line 5137 of /lib/accesslib.php: call to moodle_database->get_record()
      line 504 of /repository/lib.php: call to context::instance_by_id()
      line 61 of /repository/flickr_public/lib.php: call to repository->__construct()
      line 934 of /repository/lib.php: call to repository_flickr_public->__construct()
      line ? of unknownfile: call to repository::get_instances()
      line 1047 of /repository/lib.php: call to call_user_func_array()
      line 312 of /admin/repository.php: call to repository::static_function()

        Issue Links

          Activity

          Hide
          Helen Foster added a comment -

          From chat with Eloy:

          stronk7: aha, really looks like some context (course, user) was deleted, but not its associated repositories...
          Helen: can the cron clean that up?
          stronk7: maybe it performs deletion of orphaned repos... let me see.
          I cannot find anything in repository code doing that cleanup.
          only the dropbox repo seems to have some cron-code.

          well, I think the case is something like:

          • For a given context X it happens that:

          1) There is one repository instance having that X contextid.
          2) There isn't context X in the system anymore.

          stronk7: And that only can happen if the deletion of contexts doesn't produce the deletion of associated repos.

          Helen: what type of context are we talking about?
          stronk7: We don't know, hehe, because it has been deleted. But I bet it's a user or course context.
          Helen: e.g?
          stronk7: anyway, I'm looking to the context->delete() code and cannot find any use of repositories at all.
          so perhaps it can be tested by:

          1) create user (teacher in some course)
          2) create any repository and enable the "Allow users to add a repository instance into the course/user" (2 checks)
          3) logged as user add one instance of the repository to the course and another to the user.
          4) verify "manage repos" work
          5) delete the course
          6) verify "manage repos" work
          7) delete the user
          8) verify "manage repos" work

          stronk7: flickr_public, webdav seem to have that feature (create course and/or user instances)
          the rest are always system-context repos so cannot lead to the problem.
          in fact the error you pasted above happens for some flickr_public repo.
          but then it's a general issue for sure.
          deletion of a context should lead to deletion of the repos in that context.
          (to keep data consistent)
          delete_user() calls delete_context() but none of them look for existing repos to be deleted.
          same for delete_course(). It calls both remove_course_contents() and delete_context() but none of them look for existing repos to be deleted.
          (note it's a quick look)
          but seems that any of them can lead to the problem of orphaned repository instances.
          sure it needs to verify the steps above are able to reproduce it.

          offtopic: I bet the associated web services for deleting a user and a course... will expose the same problem.

          Show
          Helen Foster added a comment - From chat with Eloy: stronk7: aha, really looks like some context (course, user) was deleted, but not its associated repositories... Helen: can the cron clean that up? stronk7: maybe it performs deletion of orphaned repos... let me see. I cannot find anything in repository code doing that cleanup. only the dropbox repo seems to have some cron-code. well, I think the case is something like: For a given context X it happens that: 1) There is one repository instance having that X contextid. 2) There isn't context X in the system anymore. stronk7: And that only can happen if the deletion of contexts doesn't produce the deletion of associated repos. Helen: what type of context are we talking about? stronk7: We don't know, hehe, because it has been deleted. But I bet it's a user or course context. Helen: e.g? stronk7: anyway, I'm looking to the context->delete() code and cannot find any use of repositories at all. so perhaps it can be tested by: 1) create user (teacher in some course) 2) create any repository and enable the "Allow users to add a repository instance into the course/user" (2 checks) 3) logged as user add one instance of the repository to the course and another to the user. 4) verify "manage repos" work 5) delete the course 6) verify "manage repos" work 7) delete the user 8) verify "manage repos" work stronk7: flickr_public, webdav seem to have that feature (create course and/or user instances) the rest are always system-context repos so cannot lead to the problem. in fact the error you pasted above happens for some flickr_public repo. but then it's a general issue for sure. deletion of a context should lead to deletion of the repos in that context. (to keep data consistent) delete_user() calls delete_context() but none of them look for existing repos to be deleted. same for delete_course(). It calls both remove_course_contents() and delete_context() but none of them look for existing repos to be deleted. (note it's a quick look) but seems that any of them can lead to the problem of orphaned repository instances. sure it needs to verify the steps above are able to reproduce it. offtopic: I bet the associated web services for deleting a user and a course... will expose the same problem.
          Hide
          Helen Foster added a comment -

          PS This issue is affecting http://school.demo.moodle.net.

          Show
          Helen Foster added a comment - PS This issue is affecting http://school.demo.moodle.net .
          Hide
          Frédéric Massart added a comment - - edited

          This seems to come from d197ea43, where we switched to context::instance_by_id(), which is MUST_EXIST by default, unlike get_context_instance_by_id(). I don't think we can assume that a context does not exist any more, and so the error above makes sense. The problem is that it prevents the admin to edit some repositories. From the look of it, an upgrade script to remove the orphan repositories and some more logic to delete the associated contexts upon user/course deletion seem the right way to go to me.

          (This does not affect 2.3)

          Show
          Frédéric Massart added a comment - - edited This seems to come from d197ea43, where we switched to context::instance_by_id(), which is MUST_EXIST by default, unlike get_context_instance_by_id(). I don't think we can assume that a context does not exist any more, and so the error above makes sense. The problem is that it prevents the admin to edit some repositories. From the look of it, an upgrade script to remove the orphan repositories and some more logic to delete the associated contexts upon user/course deletion seem the right way to go to me. (This does not affect 2.3)
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Sorry but... which it the rationale about allowing one repo instance to be pointing to a non-existing context? (that for sure won't exist anymore, we still have not invented context-reviving, lol).

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Sorry but... which it the rationale about allowing one repo instance to be pointing to a non-existing context? (that for sure won't exist anymore, we still have not invented context-reviving, lol). Ciao
          Hide
          Frédéric Massart added a comment -

          Hi Eloy, I think I failed to construct a proper sentence here. What I meant was that the previous logic in 2.3 was to ignore missing context, and proceed with the rest of the code. As you said a missing context will be missing for ever so it was not correct in any way. As in >=2.4 we changed to context::instance_by_id(), an exception is raised, my guess is that we should write an upgrade process to clean up the orphans, and add a logic to course/user deletion. (So my guess is just copying what you said above ahah).

          Cheers

          Show
          Frédéric Massart added a comment - Hi Eloy, I think I failed to construct a proper sentence here. What I meant was that the previous logic in 2.3 was to ignore missing context, and proceed with the rest of the code. As you said a missing context will be missing for ever so it was not correct in any way. As in >=2.4 we changed to context::instance_by_id(), an exception is raised, my guess is that we should write an upgrade process to clean up the orphans, and add a logic to course/user deletion. (So my guess is just copying what you said above ahah). Cheers
          Hide
          Eloy Lafuente (stronk7) added a comment -

          ahah, thanks for the clarification, so you are not crazy after all, lol!

          Show
          Eloy Lafuente (stronk7) added a comment - ahah, thanks for the clarification, so you are not crazy after all, lol!
          Hide
          David added a comment - - edited

          Edit - minutes after posting the following, I was able to address the issue. It seems that, after making the post, the solution offered at the top of the page suddenly made sense and I worked through it using WebMin. This is not an issue with the instructions, but rather with me. Sorry for any inconvenience. And many thanks for the solution!

          #####

          Hello, All.

          I won't claim to understand all of the discussion up to this point, but I would like to add that this problem is occurring in our Moodle 2.5. Here is the debugging message and stack trace:

          ****
          Manage repositories

          Can not find data record in database table context.

          More information about this error
          Debug info: SELECT * FROM

          {context}

          WHERE id = ?
          [array (
          0 => '90797',
          )]
          Error code: invalidrecord
          Stack trace:

          line 1372 of /lib/dml/moodle_database.php: dml_missing_record_exception thrown
          line 1348 of /lib/dml/moodle_database.php: call to moodle_database->get_record_select()
          line 5172 of /lib/accesslib.php: call to moodle_database->get_record()
          line 323 of /admin/repository.php: call to context::instance_by_id()
          ****

          Show
          David added a comment - - edited Edit - minutes after posting the following, I was able to address the issue. It seems that, after making the post, the solution offered at the top of the page suddenly made sense and I worked through it using WebMin. This is not an issue with the instructions, but rather with me. Sorry for any inconvenience. And many thanks for the solution! ##### Hello, All. I won't claim to understand all of the discussion up to this point, but I would like to add that this problem is occurring in our Moodle 2.5. Here is the debugging message and stack trace: **** Manage repositories Can not find data record in database table context. More information about this error Debug info: SELECT * FROM {context} WHERE id = ? [array ( 0 => '90797', )] Error code: invalidrecord Stack trace: line 1372 of /lib/dml/moodle_database.php: dml_missing_record_exception thrown line 1348 of /lib/dml/moodle_database.php: call to moodle_database->get_record_select() line 5172 of /lib/accesslib.php: call to moodle_database->get_record() line 323 of /admin/repository.php: call to context::instance_by_id() ****
          Hide
          Frédéric Massart added a comment -

          So here is a patch.

          1. I've added a simple upgrade script to remove the orphan instances, I thought of placing that in a dedicated method in upgradelib, but that's too straightforward...
          2. Now when we delete the context of a course or a user, we call the new corresponding method in repositories to delete the attached repo. By default I decided to transform the references into hard copies because that is what happens in stored_file::delete(), which is called upon course deletion.
          3. I've added a basic Test coverage, it does not include the conversion references to hard copies, because it might take a while, and is not really related to this new method.

          Up for review.
          Cheers!

          Fred

          Show
          Frédéric Massart added a comment - So here is a patch. I've added a simple upgrade script to remove the orphan instances, I thought of placing that in a dedicated method in upgradelib, but that's too straightforward... Now when we delete the context of a course or a user, we call the new corresponding method in repositories to delete the attached repo. By default I decided to transform the references into hard copies because that is what happens in stored_ file::delete( ), which is called upon course deletion. I've added a basic Test coverage, it does not include the conversion references to hard copies, because it might take a while, and is not really related to this new method. Up for review. Cheers! Fred
          Hide
          Nick Varney added a comment -

          We have exactly the same issue (Moodle 2.4.4 (Build: 20130513)) and would appreciate a patch to be released.

          Show
          Nick Varney added a comment - We have exactly the same issue (Moodle 2.4.4 (Build: 20130513)) and would appreciate a patch to be released.
          Hide
          Mark Nelson added a comment -

          Hi Fred,

          Code looks nice as always, just a couple of points.

          1. How does a user locate the files once the repository has been deleted? You loop through the contents and if $downloadcontents is set to true, which it is in this case, then the files are downloaded before the repository is deleted. However, once the repository is deleted how is the user supposed to locate these files? Won't they just be sitting in some hashed location in the file storage area?
          2. You have a lot of tests which is great, but do the user and course context cover all possible scenarios?
          Show
          Mark Nelson added a comment - Hi Fred, Code looks nice as always, just a couple of points. How does a user locate the files once the repository has been deleted? You loop through the contents and if $downloadcontents is set to true, which it is in this case, then the files are downloaded before the repository is deleted. However, once the repository is deleted how is the user supposed to locate these files? Won't they just be sitting in some hashed location in the file storage area? You have a lot of tests which is great, but do the user and course context cover all possible scenarios?
          Hide
          Mark Nelson added a comment -

          Hi Fred, please ignore point 1. I went through the code and saw that this function is not actually downloading all the files in the repository before it is deleted (I was misinterpreting convert_references_to_local as something like convert_repositories_to_local due to the context of this issue), but is creating hard copies of files that are referencing the one being removed so that they still work. Which makes sense.

          So, now my only point regarding this function is whether the second parameter is needed, would it ever be false? It is a new function and only used once, so why create a second parameter you do not actually specify when you call the function because it has a default value of true? If it is needed later on, then it can be added then, but for now I would just only pass the contextid paremeter and remove the if $downloadcontents statement.

          Show
          Mark Nelson added a comment - Hi Fred, please ignore point 1. I went through the code and saw that this function is not actually downloading all the files in the repository before it is deleted (I was misinterpreting convert_references_to_local as something like convert_repositories_to_local due to the context of this issue), but is creating hard copies of files that are referencing the one being removed so that they still work. Which makes sense. So, now my only point regarding this function is whether the second parameter is needed, would it ever be false? It is a new function and only used once, so why create a second parameter you do not actually specify when you call the function because it has a default value of true? If it is needed later on, then it can be added then, but for now I would just only pass the contextid paremeter and remove the if $downloadcontents statement.
          Hide
          Frédéric Massart added a comment -

          Thanks for your review Mark. I decided to use the parameter $downloadcontents, even though it is set to true to emphasise the behaviour of this method. A similar parameter is used in the repository::delete() method, though set to false by default, and so I thought it'd be better to have it there even if it's not currently use.

          I am happy to take it out if necessary, but in the meantime I am pushing this for integration.

          Cheers!
          Fred

          Show
          Frédéric Massart added a comment - Thanks for your review Mark. I decided to use the parameter $downloadcontents, even though it is set to true to emphasise the behaviour of this method. A similar parameter is used in the repository::delete() method, though set to false by default, and so I thought it'd be better to have it there even if it's not currently use. I am happy to take it out if necessary, but in the meantime I am pushing this for integration. Cheers! Fred
          Hide
          Sam Hemelryk added a comment -

          Hi Fred,

          This change looks good to me however before I integrate I want to just get Eloy to check the SQL here just to be sure that its a) optimal and b) cross-db fine.
          All going well this will land tomorrow.

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Hi Fred, This change looks good to me however before I integrate I want to just get Eloy to check the SQL here just to be sure that its a) optimal and b) cross-db fine. All going well this will land tomorrow. Many thanks Sam
          Hide
          Sam Hemelryk added a comment -

          Hi Fred,

          Reopening this week sorry.
          After talking to Eloy about it the consensus is that it would be better to handle this with a single DELETE FROM . WHERE NOT EXISTS statement.
          Eloy believed it was cross-db compatible (although do not alias the table as PG will barf).
          MySQL can be slow as well, so if you expect that there may be lots of records to delete then using the following style would probably be good:
          https://github.com/FMCorz/moodle/blob/cdef38299b48a9d1cd7da1fb9f47da7e02b6bf92/lib/db/upgrade.php#L403

          Other than that it all looks spot on.

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Hi Fred, Reopening this week sorry. After talking to Eloy about it the consensus is that it would be better to handle this with a single DELETE FROM . WHERE NOT EXISTS statement. Eloy believed it was cross-db compatible (although do not alias the table as PG will barf). MySQL can be slow as well, so if you expect that there may be lots of records to delete then using the following style would probably be good: https://github.com/FMCorz/moodle/blob/cdef38299b48a9d1cd7da1fb9f47da7e02b6bf92/lib/db/upgrade.php#L403 Other than that it all looks spot on. Many thanks Sam
          Hide
          CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          Frédéric Massart added a comment -

          Thanks Sam,

          Resubmitting straight to integration as I've just improved the SQL query in the upgrade script. As it seems like aliases are a concern, I decided to use the full table name. I have tested this upgrade code on PostgreSQL and MySQL and it all went fine.

          Cheers!
          Fred

          Show
          Frédéric Massart added a comment - Thanks Sam, Resubmitting straight to integration as I've just improved the SQL query in the upgrade script. As it seems like aliases are a concern, I decided to use the full table name. I have tested this upgrade code on PostgreSQL and MySQL and it all went fine. Cheers! Fred
          Hide
          Damyon Wiese added a comment -

          Thanks Fred,

          The fix looks correct to me.

          Integrated to 24, 25 and master.

          Show
          Damyon Wiese added a comment - Thanks Fred, The fix looks correct to me. Integrated to 24, 25 and master.
          Hide
          Damyon Wiese added a comment -

          Hi Fred,

          This is failing unit tests on 24 in integration.

          Call to undefined method phpunit_data_generator::create_repository_type().

          Show
          Damyon Wiese added a comment - Hi Fred, This is failing unit tests on 24 in integration. Call to undefined method phpunit_data_generator::create_repository_type().
          Hide
          Damyon Wiese added a comment -

          Pulled a fix from Fred for 24 (Thanks!)

          Show
          Damyon Wiese added a comment - Pulled a fix from Fred for 24 (Thanks!)
          Hide
          Dan Poltawski added a comment -

          This is causing an issue on php5.4. Fred informs me its already reported as MDL-38366.

          Moodle 2.6dev (Build: 20130704), pgsql, 92e9260d964fac9182854a98a3d14c9100079292
          PHPUnit 3.7.21 by Sebastian Bergmann.
          
          Configuration read from /Users/danp/git/integration/phpunit.xml
          
          .....E
          
          Time: 5 seconds, Memory: 75.00Mb
          
          There was 1 error:
          
          1) repositorylib_testcase::test_delete_all_for_context
          Array to string conversion
          
          /Users/danp/git/integration/repository/filesystem/lib.php:53
          /Users/danp/git/integration/repository/lib.php:605
          /Users/danp/git/integration/repository/lib.php:1134
          /Users/danp/git/integration/repository/lib.php:2048
          /Users/danp/git/integration/repository/filesystem/lib.php:226
          /Users/danp/git/integration/repository/lib.php:1172
          /Users/danp/git/integration/lib/testing/generator/repository_generator.php:146
          /Users/danp/git/integration/lib/testing/generator/data_generator.php:590
          /Users/danp/git/integration/repository/tests/repository_test.php:480
          /Users/danp/git/integration/lib/phpunit/classes/advanced_testcase.php:76
          
          To re-run:
          vendor/bin/phpunit repositorylib_testcase repository/tests/repository_test.php
          
          Show
          Dan Poltawski added a comment - This is causing an issue on php5.4. Fred informs me its already reported as MDL-38366 . Moodle 2.6dev (Build: 20130704), pgsql, 92e9260d964fac9182854a98a3d14c9100079292 PHPUnit 3.7.21 by Sebastian Bergmann. Configuration read from /Users/danp/git/integration/phpunit.xml .....E Time: 5 seconds, Memory: 75.00Mb There was 1 error: 1) repositorylib_testcase::test_delete_all_for_context Array to string conversion /Users/danp/git/integration/repository/filesystem/lib.php:53 /Users/danp/git/integration/repository/lib.php:605 /Users/danp/git/integration/repository/lib.php:1134 /Users/danp/git/integration/repository/lib.php:2048 /Users/danp/git/integration/repository/filesystem/lib.php:226 /Users/danp/git/integration/repository/lib.php:1172 /Users/danp/git/integration/lib/testing/generator/repository_generator.php:146 /Users/danp/git/integration/lib/testing/generator/data_generator.php:590 /Users/danp/git/integration/repository/tests/repository_test.php:480 /Users/danp/git/integration/lib/phpunit/classes/advanced_testcase.php:76 To re-run: vendor/bin/phpunit repositorylib_testcase repository/tests/repository_test.php
          Hide
          Frédéric Massart added a comment -

          The fix is up for grabs on MDL-38366.

          Show
          Frédéric Massart added a comment - The fix is up for grabs on MDL-38366 .
          Hide
          Marina Glancy added a comment -

          So what are we doing now? Reverting this or integrating MDL-38366?

          Show
          Marina Glancy added a comment - So what are we doing now? Reverting this or integrating MDL-38366 ?
          Hide
          Frédéric Massart added a comment -

          This patch is not the problem, this patch highlights it. I would prefer MDL-38366 to be integrated.

          Show
          Frédéric Massart added a comment - This patch is not the problem, this patch highlights it. I would prefer MDL-38366 to be integrated.
          Hide
          Dan Poltawski added a comment -

          Defintely don't think this should be reverted. But the issue breaks the unit tests (on 5.4). We can never release with broken unit tests, hence this issue fails.

          I have a small bit of concern about that solution to the problem. No doubt its better API, but could it break compatibility with any other plugins?

          Show
          Dan Poltawski added a comment - Defintely don't think this should be reverted. But the issue breaks the unit tests (on 5.4). We can never release with broken unit tests, hence this issue fails. I have a small bit of concern about that solution to the problem. No doubt its better API, but could it break compatibility with any other plugins?
          Hide
          Marina Glancy added a comment -

          Sending back to testing. MDL-38366 has just been integrated, it should fix the unittest problems. Thanks

          Show
          Marina Glancy added a comment - Sending back to testing. MDL-38366 has just been integrated, it should fix the unittest problems. Thanks
          Hide
          David Monllaó added a comment -

          I've gone through all the test but the upgrade test, I'll continue later

          Show
          David Monllaó added a comment - I've gone through all the test but the upgrade test, I'll continue later
          Hide
          Michael de Raadt added a comment -

          David asked me to help with the upgrade tests. I will start with the more exotic DBs (SQLSVR, MSSQL and Oracle).

          Show
          Michael de Raadt added a comment - David asked me to help with the upgrade tests. I will start with the more exotic DBs (SQLSVR, MSSQL and Oracle).
          Hide
          Dan Poltawski added a comment -

          I can take oracle or mssql if you like Michael?

          Show
          Dan Poltawski added a comment - I can take oracle or mssql if you like Michael?
          Hide
          Michael de Raadt added a comment -

          I had a chat with Dan and he agreed to take MySQL and PostgreSQL for the upgrade test.

          I can confirm that the fix works for the sqlsvr, mssql and oracle drivers.

          I tested on an upgrade from 2.5 stable to master.

          Show
          Michael de Raadt added a comment - I had a chat with Dan and he agreed to take MySQL and PostgreSQL for the upgrade test. I can confirm that the fix works for the sqlsvr, mssql and oracle drivers. I tested on an upgrade from 2.5 stable to master.
          Hide
          Dan Poltawski added a comment -

          Tested mysql on master, 25 and 24 ok.

          Show
          Dan Poltawski added a comment - Tested mysql on master, 25 and 24 ok.
          Hide
          Dan Poltawski added a comment -

          And done a 2.5->2.5 upgrade test on pgsql and also all good.

          I guess this can be passed!

          Show
          Dan Poltawski added a comment - And done a 2.5->2.5 upgrade test on pgsql and also all good. I guess this can be passed!
          Hide
          David Monllaó added a comment -

          Postgres upgrade from 2.4 (before MDL-38314) to 2.4 latest integration also working as described. Passing it.

          Many thanks Michael and Dan!

          Show
          David Monllaó added a comment - Postgres upgrade from 2.4 (before MDL-38314 ) to 2.4 latest integration also working as described. Passing it. Many thanks Michael and Dan!
          Hide
          Damyon Wiese added a comment -

          a single bug fix
          a drop in a waterfall
          hear the mighty roar

          Thanks for your contribution!

          Show
          Damyon Wiese added a comment - a single bug fix a drop in a waterfall hear the mighty roar Thanks for your contribution!

            People

            • Votes:
              7 Vote for this issue
              Watchers:
              16 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Agile