Moodle
  1. Moodle
  2. MDL-32862

Links to some 1.9 resource types break after upgrade to 2.2 followed by backup and restore

    Details

    • Testing Instructions:
      Hide

      Prerequisites

      You will need the following:

      • A 1.9 installation.
        • A course in your 1.9 installation.
      • Access to the database.
      • Patience.

      Preparation

      1. In a course in your 1.9 installation, create one each of the following:
        • A web page.
        • A text page.
        • A url.
        • A directory with files in it.
      2. Next create a new web page and add links to each of the resources that are mentioned above. (http://yourmoodleinstance.com/mod/resource/view.php?id=x)
      3. Also create links to each of the resources, but use the resource id instead of the cmid. You may have to load up the mdl_resource table and look up the ID for each resource. Your URL should end up looking like: http://yourmoodleinstance.com/mod/resource/view.php?r=x.
      4. Upgrade your installation to 2.2

      Testing

      1. Go to the course that has these resources and create a backup of the course.
      2. restore the back up on master (or some other version).
      3. Go to the website that you created with the links to the other resources and click on each one.
      4. Ensure that the links redirect to the correct page and that no errors are displayed.

      Tedium

      1. Run through the instructions again, but this time upgrade to 2.2 and then immediately to the fix versions (possibly 2.3, 2.4, 2.5 and master) and do a backup of the course, restore, etc.
      Show
      Prerequisites You will need the following: A 1.9 installation. A course in your 1.9 installation. Access to the database. Patience. Preparation In a course in your 1.9 installation, create one each of the following: A web page. A text page. A url. A directory with files in it. Next create a new web page and add links to each of the resources that are mentioned above. ( http://yourmoodleinstance.com/mod/resource/view.php?id=x ) Also create links to each of the resources, but use the resource id instead of the cmid. You may have to load up the mdl_resource table and look up the ID for each resource. Your URL should end up looking like: http://yourmoodleinstance.com/mod/resource/view.php?r=x . Upgrade your installation to 2.2 Testing Go to the course that has these resources and create a backup of the course. restore the back up on master (or some other version). Go to the website that you created with the links to the other resources and click on each one. Ensure that the links redirect to the correct page and that no errors are displayed. Tedium Run through the instructions again, but this time upgrade to 2.2 and then immediately to the fix versions (possibly 2.3, 2.4, 2.5 and master) and do a backup of the course, restore, etc.
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE
    • Pull from Repository:
    • Pull 2.4 Branch:
      wip-MDL-32862-24
    • Pull 2.5 Branch:
      wip-MDL-32862-25
    • Pull Master Branch:
      wip-MDL-32862-master
    • Story Points:
      13
    • Rank:
      51955
    • Sprint:
      BACKEND Sprint 4

      Description

      Steps to reproduce:

      1. Create a Moodle 1.9 course with a resource of the following type: URL, text page, web page, or directory
      2. In a topic of the course (or other wysiwyg-editable text in the course), create a link to your resource
      3. Upgrade your Moodle instance to Moodle 2.2
      4. At this point, you'll find that the link still works. It will point to mod/resource/view.php but will redirect to the correct Moodle 2.2 URL such as mod/page/view.php
      5. Create a backup file of the course
      6. Restore the backup file into a new course

      Result: The link in the restored course will not work. It will point to mod/resource/view.php, but now the page will show you the error message "Invalid course module ID".

        Issue Links

          Activity

          Hide
          Aaron Wells added a comment -

          The affected resources are all ones which were subtypes of the "resource" module in 1.9 but were changed to their own modules in Moodle 2, which means the URL to access them has changed from mod/resource/view.php to mod/somethingelse/view.php.

          Part of the 1.9->2 upgrade process is that these resources were removed from mdl_resource and copied into the appropriate table for their new module types. At the same time, a mdl_resource_old table is created, which maps the original resource ID to the ID in the new module's table. Then, when you access mod/resource/view.php with an ID that is not in mdl_resource, it checks mdl_resource_old as a fallback and if it finds a record there it redirects you to the appropriate URL for the new module.

          So, that's why the links still work after the upgrade process.

          The problem is, mdl_resource_old is not included in course backups. That's why it doesn't work after you backup and restore such a course.

          I can see two possible solutions. 1) Include mdl_resource_old contents in the course backup/restore process (there is a course id column in the table). Or 2) Rewrite the links from mod/resource/view.php to the correct URL for the new module, during the initial upgrade process and/or when creating a backup.

          Show
          Aaron Wells added a comment - The affected resources are all ones which were subtypes of the "resource" module in 1.9 but were changed to their own modules in Moodle 2, which means the URL to access them has changed from mod/resource/view.php to mod/somethingelse/view.php. Part of the 1.9->2 upgrade process is that these resources were removed from mdl_resource and copied into the appropriate table for their new module types. At the same time, a mdl_resource_old table is created, which maps the original resource ID to the ID in the new module's table. Then, when you access mod/resource/view.php with an ID that is not in mdl_resource, it checks mdl_resource_old as a fallback and if it finds a record there it redirects you to the appropriate URL for the new module. So, that's why the links still work after the upgrade process. The problem is, mdl_resource_old is not included in course backups. That's why it doesn't work after you backup and restore such a course. I can see two possible solutions. 1) Include mdl_resource_old contents in the course backup/restore process (there is a course id column in the table). Or 2) Rewrite the links from mod/resource/view.php to the correct URL for the new module, during the initial upgrade process and/or when creating a backup.
          Hide
          Aaron Wells added a comment -

          I've implemented a fix for this bug. When a course is being backed up, this fix locates any mod/resource links that are in the resource_old table, and rewrites them to be links to the proper URL for the resource's new module.

          Show
          Aaron Wells added a comment - I've implemented a fix for this bug. When a course is being backed up, this fix locates any mod/resource links that are in the resource_old table, and rewrites them to be links to the proper URL for the resource's new module.
          Hide
          Martin Dougiamas added a comment -

          Petr can you have a quick look at this?

          Show
          Martin Dougiamas added a comment - Petr can you have a quick look at this?
          Hide
          Petr Škoda added a comment - - edited

          Hello:
          1/ anonymous functions should never be called in a loop because they may eat up all memory, please use static class method if possible or normal function
          2/ this backup time solution looks appropriate for this problem, Eloy should review the logic anyway imo

          Thanks for the patch!

          Show
          Petr Škoda added a comment - - edited Hello: 1/ anonymous functions should never be called in a loop because they may eat up all memory, please use static class method if possible or normal function 2/ this backup time solution looks appropriate for this problem, Eloy should review the logic anyway imo Thanks for the patch!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi,

          I think it's a very good solution to clean all those old links in contents in the backup process to make them point to the new module.

          I like the patch but because of some minor things:

          1) I would statically cache the results of $DB->record_exists('resource_old'). Else it's executed for each content in the backup file. (thousands of times).

          2) Instead of preg_replace_callback() + implicit function I would use preg_match_all() + explicit iteration doing the lookup and replace.

          +1 once those points are addressed, really clever solution, thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Hi, I think it's a very good solution to clean all those old links in contents in the backup process to make them point to the new module. I like the patch but because of some minor things: 1) I would statically cache the results of $DB->record_exists('resource_old'). Else it's executed for each content in the backup file. (thousands of times). 2) Instead of preg_replace_callback() + implicit function I would use preg_match_all() + explicit iteration doing the lookup and replace. +1 once those points are addressed, really clever solution, thanks!
          Hide
          Petr Škoda added a comment -

          and explicitly specify the necessary columns in get_record() to minimise used memory when caching

          Show
          Petr Škoda added a comment - and explicitly specify the necessary columns in get_record() to minimise used memory when caching
          Hide
          Aaron Wells added a comment -

          Hi guys,

          Thanks for the review. I've updated the code in my pull branches to incorporate your feedback, so please take a look when you can.

          Show
          Aaron Wells added a comment - Hi guys, Thanks for the review. I've updated the code in my pull branches to incorporate your feedback, so please take a look when you can.
          Hide
          Dan Poltawski added a comment -

          Ping Petr/Eloy?

          Show
          Dan Poltawski added a comment - Ping Petr/Eloy?
          Hide
          Petr Škoda added a comment -

          Eloy?

          Show
          Petr Škoda added a comment - Eloy?
          Hide
          Petr Škoda added a comment -

          to me it looks ok

          Show
          Petr Škoda added a comment - to me it looks ok
          Hide
          Pau Ferrer Ocaña (crazyserver) added a comment -

          Hi!

          For this patch I have another very related problem:

          1. Create a Moodle 1.9 course with a resource of the following type: URL, text page, web page, or directory
          2. In a topic of the course (or other wysiwyg-editable text in the course), create a link to your resource
          3. Create a backup file of the course
          4. Restore the backup file into a course on Moodle 2

          Same result, but the backup cannot create the resource_old record because it will be restored in a different Moodle, so this patch does not work for this case. Any idea?

          Show
          Pau Ferrer Ocaña (crazyserver) added a comment - Hi! For this patch I have another very related problem: 1. Create a Moodle 1.9 course with a resource of the following type: URL, text page, web page, or directory 2. In a topic of the course (or other wysiwyg-editable text in the course), create a link to your resource 3. Create a backup file of the course 4. Restore the backup file into a course on Moodle 2 Same result, but the backup cannot create the resource_old record because it will be restored in a different Moodle, so this patch does not work for this case. Any idea?
          Hide
          Aaron Wells added a comment -

          Right, so instead of going "1.9->upgrade->2->backupfile->2" you're doing "1.9->backupfile->2".

          Hm, it looks like mod/resource in Moodle 2, when restoring a Moodle 1.9 backup file, converts some resources into the new module types, similar to what happens during an upgrade. Except, it doesn't try to create a resource_old table, or rewrite the links (at least, not as far as I can tell from just eyeballing the code). So, a similar issue, but it's going to need a separate solution.

          It's probably overkill if you just need to migrate one course, but there is Matt Clarkson's 1.9->2 course converter: https://github.com/catalyst/oneninetotwo . It operates by taking a 1.9 backup file, loading it into a temporary Moodle instance, upgrading that instance to 2, and then making a backup file in 2. (I've been supporting an installation of it, which is why I noticed this bug in the first place.) Since the upgrade process is generally more robust than the process of importing 1.9 backup files, it may give better results. On the other hand, it's kind of a lot of work to set up.

          Show
          Aaron Wells added a comment - Right, so instead of going "1.9->upgrade->2->backupfile->2" you're doing "1.9->backupfile->2". Hm, it looks like mod/resource in Moodle 2, when restoring a Moodle 1.9 backup file, converts some resources into the new module types, similar to what happens during an upgrade. Except, it doesn't try to create a resource_old table, or rewrite the links (at least, not as far as I can tell from just eyeballing the code). So, a similar issue, but it's going to need a separate solution. It's probably overkill if you just need to migrate one course, but there is Matt Clarkson's 1.9->2 course converter: https://github.com/catalyst/oneninetotwo . It operates by taking a 1.9 backup file, loading it into a temporary Moodle instance, upgrading that instance to 2, and then making a backup file in 2. (I've been supporting an installation of it, which is why I noticed this bug in the first place.) Since the upgrade process is generally more robust than the process of importing 1.9 backup files, it may give better results. On the other hand, it's kind of a lot of work to set up.
          Hide
          Pau Ferrer Ocaña (crazyserver) added a comment -

          Thans for your comments!

          Show
          Pau Ferrer Ocaña (crazyserver) added a comment - Thans for your comments!
          Hide
          Dan Poltawski added a comment -

          Sending all 'waiting for peer review' issues to integration before freeze, as agreed in Integrators Meeting 19/10/12. We are doing this to ensure any 'integratable issues' will not got missed before freeze..

          Show
          Dan Poltawski added a comment - Sending all 'waiting for peer review' issues to integration before freeze, as agreed in Integrators Meeting 19/10/12. We are doing this to ensure any 'integratable issues' will not got missed before freeze..
          Hide
          Tobias Marx added a comment -

          I think theses bugs are related. Importing courses from Moodle 1.X to Moodle 2.X breaks links in ressources completely.
          Fix included.

          Show
          Tobias Marx added a comment - I think theses bugs are related. Importing courses from Moodle 1.X to Moodle 2.X breaks links in ressources completely. Fix included.
          Hide
          Dan Poltawski added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Dan Poltawski added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Dan Poltawski added a comment -

          This one needs more peer review, setting state for that.

          Show
          Dan Poltawski added a comment - This one needs more peer review, setting state for that.
          Hide
          Damyon Wiese added a comment -

          Hi Aaron,

          Thanks for the patch - it is a good thing to fix.

          Some feedback for you:

          1.
          The code to redirect based on entries in the mdl_resource_old table handles 2 different types of urls.

          mod/resource/view.php?id=<cmid>
          and
          mod/resource/view.php?r=<recordid>

          I think we should be handling both cases here (this patch only handles the first case only).

          2.
          You should declare $resource_old_exists as a static class variable (don't declare it inside the function).

          3.
          I will also create a separate issue for Pau's problem restoring 1.9 backups.

          4.
          The only other minor thing I found is that the comments in the patch do not all start with a capital / end with a fullstop. This will give warnings in our codechecker based on the guidelines here:

          http://docs.moodle.org/dev/Coding_style#Inline_comments

          Thanks again, Damyon

          Show
          Damyon Wiese added a comment - Hi Aaron, Thanks for the patch - it is a good thing to fix. Some feedback for you: 1. The code to redirect based on entries in the mdl_resource_old table handles 2 different types of urls. mod/resource/view.php?id=<cmid> and mod/resource/view.php?r=<recordid> I think we should be handling both cases here (this patch only handles the first case only). 2. You should declare $resource_old_exists as a static class variable (don't declare it inside the function). 3. I will also create a separate issue for Pau's problem restoring 1.9 backups. 4. The only other minor thing I found is that the comments in the patch do not all start with a capital / end with a fullstop. This will give warnings in our codechecker based on the guidelines here: http://docs.moodle.org/dev/Coding_style#Inline_comments Thanks again, Damyon
          Hide
          Damyon Wiese added a comment -

          (Just fixing the branches/repository url)

          Show
          Damyon Wiese added a comment - (Just fixing the branches/repository url)
          Hide
          Michael de Raadt added a comment -

          Hi, Aaron.

          It would be good to get this issue resolved and it is very close now. Could you please have a look at Damyon's feedback and make those amendments?

          Show
          Michael de Raadt added a comment - Hi, Aaron. It would be good to get this issue resolved and it is very close now. Could you please have a look at Damyon's feedback and make those amendments?
          Hide
          Aaron Wells added a comment -

          Hi all,

          Unfortunately I'm no longer working on the project where I encountered this bug, so I'm unlikely to have the time to address issue #1 (the mod/resource/view?r=<recordid> URLs) in Damyon's review.

          Cheers,
          Aaron

          Show
          Aaron Wells added a comment - Hi all, Unfortunately I'm no longer working on the project where I encountered this bug, so I'm unlikely to have the time to address issue #1 (the mod/resource/view?r=<recordid> URLs) in Damyon's review. Cheers, Aaron
          Hide
          Michael de Raadt added a comment -

          Thanks for responding, Aaron.

          We'll see if we can get this wrapped up, based on your work so far.

          Show
          Michael de Raadt added a comment - Thanks for responding, Aaron. We'll see if we can get this wrapped up, based on your work so far.
          Hide
          Damyon Wiese added a comment -

          FYI: I'll cleanup the remaining issues with this patch and get it submitted.

          Show
          Damyon Wiese added a comment - FYI: I'll cleanup the remaining issues with this patch and get it submitted.
          Hide
          Chris Follin added a comment -

          Hi. We'd like to see this issue finished. It's causing problems for clients who have upgraded from 1.9 to 2.x and create new courses for a new semester by restoring master course backups.

          Show
          Chris Follin added a comment - Hi. We'd like to see this issue finished. It's causing problems for clients who have upgraded from 1.9 to 2.x and create new courses for a new semester by restoring master course backups.
          Hide
          moodle.com added a comment -

          Damyon, are you planning on working on this? If not, BACKEND team will take this.

          Show
          moodle.com added a comment - Damyon, are you planning on working on this? If not, BACKEND team will take this.
          Hide
          Adrian Greeve added a comment -

          I have looked at the comments that Damyon has made and added a commit to make the code adhere to our coding guide lines. I'll send this off to peer review and get it looked at again.

          Please note that this fix is only for 2.2 as that is the most recent 'stepping stone' for upgrading from 1.9 to the most recent moodle version.
          I have put the diff and pull information in the master section.

          Show
          Adrian Greeve added a comment - I have looked at the comments that Damyon has made and added a commit to make the code adhere to our coding guide lines. I'll send this off to peer review and get it looked at again. Please note that this fix is only for 2.2 as that is the most recent 'stepping stone' for upgrading from 1.9 to the most recent moodle version. I have put the diff and pull information in the master section.
          Hide
          Frédéric Massart added a comment -

          Hi guys,

          thanks for working on this. Here are a few comments:

          1. This patch doesn't seem to support the restore of a 1.9 backup made before upgrade. Is that expected?
          2. That raises the other question to know if the restore logic should be altered as well, not only the backup one.
          3. Can you comment on the use of this method? It looks like it could potentially take a lot of time if there are many content to check, and many entries in resource_old.
          4. array_key_exists($cmid, $oldrecs) is much slower than isset($oldrecs[$cmid]) on large arrays.

          Cheers,
          Fred

          Show
          Frédéric Massart added a comment - Hi guys, thanks for working on this. Here are a few comments: This patch doesn't seem to support the restore of a 1.9 backup made before upgrade. Is that expected? That raises the other question to know if the restore logic should be altered as well, not only the backup one. Can you comment on the use of this method? It looks like it could potentially take a lot of time if there are many content to check, and many entries in resource_old. array_key_exists($cmid, $oldrecs) is much slower than isset($oldrecs [$cmid] ) on large arrays. Cheers, Fred
          Hide
          Adrian Greeve added a comment -

          Hello Fred,

          Thanks for the review.
          As per your comments:

          1. Yes, it is expected that this patch does not support a 1.9 backup.

          See comments https://tracker.moodle.org/browse/MDL-32862?focusedCommentId=179825&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-179825
          and also point three of Damyon's comment here https://tracker.moodle.org/browse/MDL-32862?focusedCommentId=197389&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-197389

          1. I don't see why we would alter the restore logic. All the information that you need is available when backing up the course. I think the only reason that you would look at restore logic is if you are doing a restore from a 1.9 course. We're not doing that in this issue, so I don't think that it is necessary to consider it here.
          2. I believe that this would slow down the backup process. This is making a couple of database calls and if you have a lot of entries in the resource_old table then it could take a while to process them all. Are there any suggestions as to how we could implement this without slowing things down?
          3. I've changed this over.

          Thanks.

          Show
          Adrian Greeve added a comment - Hello Fred, Thanks for the review. As per your comments: Yes, it is expected that this patch does not support a 1.9 backup. See comments https://tracker.moodle.org/browse/MDL-32862?focusedCommentId=179825&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-179825 and also point three of Damyon's comment here https://tracker.moodle.org/browse/MDL-32862?focusedCommentId=197389&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-197389 I don't see why we would alter the restore logic. All the information that you need is available when backing up the course. I think the only reason that you would look at restore logic is if you are doing a restore from a 1.9 course. We're not doing that in this issue, so I don't think that it is necessary to consider it here. I believe that this would slow down the backup process. This is making a couple of database calls and if you have a lot of entries in the resource_old table then it could take a while to process them all. Are there any suggestions as to how we could implement this without slowing things down? I've changed this over. Thanks.
          Hide
          Frédéric Massart added a comment -

          Not sure how to prevent the speed of the backup to be affected. On another topic, should this be actually ported to 2.3 onwards as well? Or do the links get updated once we upgrade to 2.3+?

          Show
          Frédéric Massart added a comment - Not sure how to prevent the speed of the backup to be affected. On another topic, should this be actually ported to 2.3 onwards as well? Or do the links get updated once we upgrade to 2.3+?
          Hide
          Damyon Wiese added a comment -

          Re upgrade: You must upgrade to 2.2 from any older version before proceeding to upgrade to later versions.

          Show
          Damyon Wiese added a comment - Re upgrade: You must upgrade to 2.2 from any older version before proceeding to upgrade to later versions.
          Hide
          Frédéric Massart added a comment -

          I know, but this patch refactors the backups created in 2.2, not the backups in >= 2.3. Hence my question.

          Show
          Frédéric Massart added a comment - I know, but this patch refactors the backups created in 2.2, not the backups in >= 2.3. Hence my question.
          Hide
          Adrian Greeve added a comment - - edited

          My guess is that it probably does need backporting (frontporting?) to 2.3+ so we don't miss upgrades all the way to master via 2.2. I'm testing that now to have a definitive answer.

          *edit - Yes, this needs to be added to at least 2.4, 2.5 and master. I'm not sure about 2.3 as we only support security fixes for that.

          Show
          Adrian Greeve added a comment - - edited My guess is that it probably does need backporting (frontporting?) to 2.3+ so we don't miss upgrades all the way to master via 2.2. I'm testing that now to have a definitive answer. *edit - Yes, this needs to be added to at least 2.4, 2.5 and master. I'm not sure about 2.3 as we only support security fixes for that.
          Hide
          Adrian Greeve added a comment -

          I've created branches for the other different versions. 2.2 is located here:

          pull branch: wip-MDL-32862-22
          pull 2.2 Diff URL: https://github.com/abgreeve/moodle/compare/moodle:MOODLE_22_STABLE...wip-MDL-32862-22

          I've also updated the testing instructions to test upgrading to different versions.

          Show
          Adrian Greeve added a comment - I've created branches for the other different versions. 2.2 is located here: pull branch: wip- MDL-32862 -22 pull 2.2 Diff URL: https://github.com/abgreeve/moodle/compare/moodle:MOODLE_22_STABLE...wip-MDL-32862-22 I've also updated the testing instructions to test upgrading to different versions.
          Hide
          Frédéric Massart added a comment -

          Hi Adrian,

          looking at this again, I think there should really be a cache for the result of record_exists, but unlike the initial patch, I would store the information in the static class variable.

          Cheers,
          Fred

          Show
          Frédéric Massart added a comment - Hi Adrian, looking at this again, I think there should really be a cache for the result of record_exists, but unlike the initial patch, I would store the information in the static class variable. Cheers, Fred
          Hide
          Adrian Greeve added a comment -

          Thanks Fred,

          I've added a static variable to cache the result of recordoldexists. I have branches for the different releases.

          Heading to integration.

          Show
          Adrian Greeve added a comment - Thanks Fred, I've added a static variable to cache the result of recordoldexists. I have branches for the different releases. Heading to integration.
          Hide
          Damyon Wiese added a comment -

          Hi Adrian,

          What about urls of the form "mod/resource/view.php?r=<recordid>"?

          They need to be handled by this patch too (As I said above).

          Reopening to address this.

          Show
          Damyon Wiese added a comment - Hi Adrian, What about urls of the form "mod/resource/view.php?r=<recordid>"? They need to be handled by this patch too (As I said above). Reopening to address this.
          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
          Adrian Greeve added a comment -

          Hello Fred,

          If you could please have another look at this for me. Please note that I have only updated branch 22 and none of the others. When you are satisfied with the patch I'll update the other branches.

          Thanks.

          Show
          Adrian Greeve added a comment - Hello Fred, If you could please have another look at this for me. Please note that I have only updated branch 22 and none of the others. When you are satisfied with the patch I'll update the other branches. Thanks.
          Hide
          Adrian Greeve added a comment -

          After talking with Fred and discussing the current fix, it has become obvious that I may have to duplicate the code to handle the URL with a variable of "r" (resourceid) at the end. Trying to combine both into one sql statement is messy and difficult to read with a greater possibility of regressions being introduced.

          Pulling back into development to rework.

          Thanks.

          Show
          Adrian Greeve added a comment - After talking with Fred and discussing the current fix, it has become obvious that I may have to duplicate the code to handle the URL with a variable of "r" (resourceid) at the end. Trying to combine both into one sql statement is messy and difficult to read with a greater possibility of regressions being introduced. Pulling back into development to rework. Thanks.
          Hide
          Damyon Wiese added a comment -

          Checking the code in your branch - I don't mind the 2 sections of code - it is at least clear to follow.

          And I think if we find a broken link (link to a resource with no record in the resource_old table) we should just not change it at all.

          Show
          Damyon Wiese added a comment - Checking the code in your branch - I don't mind the 2 sections of code - it is at least clear to follow. And I think if we find a broken link (link to a resource with no record in the resource_old table) we should just not change it at all.
          Hide
          Adrian Greeve added a comment -

          Thanks Damyon for taking a quick look at how to deal with broken resource links and other concerns that we had about the code.

          I've updated all of the the branches. I'm sending this off to integration.

          Show
          Adrian Greeve added a comment - Thanks Damyon for taking a quick look at how to deal with broken resource links and other concerns that we had about the code. I've updated all of the the branches. I'm sending this off to integration.
          Hide
          Sam Hemelryk added a comment -

          Hi Adrian, quick question is there a need for this to go back to 2.3?

          Show
          Sam Hemelryk added a comment - Hi Adrian, quick question is there a need for this to go back to 2.3?
          Hide
          Adrian Greeve added a comment -

          Hello Sam,

          I know that we don't support 2.3, but if someone upgraded from 1.9 to 2.2 and then 2.3, backed up a course and then upgraded to 2.4+ They would be stuck with broken resource links. I personally can't think of why anyone would do that, but that isn't to say that it's not possible. So I threw up a branch for 2.3 just in case. It's not a security problem so I guess technically it doesn't need to go into 2.3.

          Show
          Adrian Greeve added a comment - Hello Sam, I know that we don't support 2.3, but if someone upgraded from 1.9 to 2.2 and then 2.3, backed up a course and then upgraded to 2.4+ They would be stuck with broken resource links. I personally can't think of why anyone would do that, but that isn't to say that it's not possible. So I threw up a branch for 2.3 just in case. It's not a security problem so I guess technically it doesn't need to go into 2.3.
          Hide
          Sam Hemelryk added a comment -

          Thanks for clarifying that Adrian, I've chosen to integrate to 24, 25 and master only.

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Thanks for clarifying that Adrian, I've chosen to integrate to 24, 25 and master only. Many thanks Sam
          Hide
          Sam Hemelryk added a comment -

          Grrrrr - All three branches use $DB which has not being declared global.

          Please please please test your code.

          I've now pushed fixes to all branches for this and re-run unit tests myself.

          Show
          Sam Hemelryk added a comment - Grrrrr - All three branches use $DB which has not being declared global. Please please please test your code. I've now pushed fixes to all branches for this and re-run unit tests myself.
          Hide
          Adrian Greeve added a comment -

          Sorry Sam,

          I did a cherry-pick from 2.2 to 2.3 onwards. I got a conflict which I resolved, but I didn't notice that the $DB had been dropped.

          Compounding that when reading your comments I got as far as the second line of what you said and feeling very embarrassed, went off to fix all of my branches and test each of them.

          I didn't read the last line that said that you had already fixed this and run unit tests.

          Show
          Adrian Greeve added a comment - Sorry Sam, I did a cherry-pick from 2.2 to 2.3 onwards. I got a conflict which I resolved, but I didn't notice that the $DB had been dropped. Compounding that when reading your comments I got as far as the second line of what you said and feeling very embarrassed, went off to fix all of my branches and test each of them. I didn't read the last line that said that you had already fixed this and run unit tests.
          Hide
          Petr Škoda added a comment -

          works perfectly for me, thanks a lot

          Show
          Petr Škoda added a comment - works perfectly for me, thanks a lot
          Hide
          Damyon Wiese added a comment -

          This issue along with 77 of it's friends has been sent upstream and released to the world.

          Thankyou for your contribution.

          Show
          Damyon Wiese added a comment - This issue along with 77 of it's friends has been sent upstream and released to the world. Thankyou for your contribution.

            People

            • Votes:
              12 Vote for this issue
              Watchers:
              22 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Agile