Moodle
  1. Moodle
  2. MDL-37942

Images with non-alphanumeric chars in file name won't export to portfolio

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.4.1
    • Fix Version/s: 2.3.7, 2.4.4
    • Component/s: Portfolio API
    • Labels:
    • Testing Instructions:
      Hide

      Requirements:

      Portfolio exports enabled
      File download portfolio plugin enabled and visible
      Glossary activity
      Image with spaces in filename

      Steps:

      1. Access a glossary activity
      2. Add a new glossary entry - title (definition) can be any text
      3. In the text editor for the main content (concept) of the glossary entry upload and insert an image that has spaces in the filename
      4. Save glossary entry and then view it
      5. Select to 'export' the glossary entry to portfolio by selecting the export icon
      6. Select 'File download' export type if prompted, then next button
      7. Select 'HTML with attachments', then next button, confirm the export if prompted
      8. A zip file should be downloaded
      9. Unzip using winzip software or similar, making sure folder structure in zip is replicated
      10. In the site_files folder the uploaded image should be present
      11. When opening the html file in the zip the uploaded image should be displayed in the page in the browser

      Show
      Requirements: Portfolio exports enabled File download portfolio plugin enabled and visible Glossary activity Image with spaces in filename Steps: 1. Access a glossary activity 2. Add a new glossary entry - title (definition) can be any text 3. In the text editor for the main content (concept) of the glossary entry upload and insert an image that has spaces in the filename 4. Save glossary entry and then view it 5. Select to 'export' the glossary entry to portfolio by selecting the export icon 6. Select 'File download' export type if prompted, then next button 7. Select 'HTML with attachments', then next button, confirm the export if prompted 8. A zip file should be downloaded 9. Unzip using winzip software or similar, making sure folder structure in zip is replicated 10. In the site_files folder the uploaded image should be present 11. When opening the html file in the zip the uploaded image should be displayed in the page in the browser
    • Affected Branches:
      MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull 2.4 Branch:
      wip-MDL-37942-MOODLE_24_STABLE
    • Pull Master Branch:
      wip-MDL-37942-master
    • Rank:
      47704

      Description

      I find that if I add images with non-alphanumeric characters, that are url encoded, into content that is then exportable via the Portfolio API those images cause an error in the export process.

      Error is as follows:

      Couldn't find a file from the embedded path info context 39 component mod_glossary filearea entry itemid 104 filepath / name /logo%20space.gif

      line 1254 of /lib/portfoliolib.php: call to debugging()

      Looking at lib/portfoliolib.php I can see that portfolio_rewrite_pluginfile_url_callback() does not have any url decoding. The pluginfile image name would be url encoded when sent and this won't match the name in the file table without being decoded.

        Issue Links

          Activity

          Hide
          Jason Fowler added a comment -

          [Y] Syntax
          [-] Output
          [Y] Whitespace
          [-] Language
          [-] Databases
          [Y] Testing
          [-] Security
          [-] Documentation
          [Y] Git
          [Y] Sanity check

          Does this need to go back to 2.3 as well?

          Show
          Jason Fowler added a comment - [Y] Syntax [-] Output [Y] Whitespace [-] Language [-] Databases [Y] Testing [-] Security [-] Documentation [Y] Git [Y] Sanity check Does this need to go back to 2.3 as well?
          Hide
          Jason Platts added a comment -

          Added 23 stable branch fix.

          Show
          Jason Platts added a comment - Added 23 stable branch fix.
          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 -

          Hi,

          Sorry but this fix doesn't look complete to me. Couldn't there also be non-alphanumeric characters in a custom path? In which case don't the path parts also need to accounted for?

          thanks,
          Dan

          Show
          Dan Poltawski added a comment - Hi, Sorry but this fix doesn't look complete to me. Couldn't there also be non-alphanumeric characters in a custom path? In which case don't the path parts also need to accounted for? thanks, Dan
          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
          Jason Platts added a comment -

          Dan, are there any examples of plugins that use portfolio API with file paths?

          The only one I have come across is the assign module, and this doesn't seem to use file paths in the export (or have issues with non-alphanumeric chars in file names) - presumably because it pulls directly from files table rather than using portfolio_rewrite_pluginfile_urls().

          Show
          Jason Platts added a comment - Dan, are there any examples of plugins that use portfolio API with file paths? The only one I have come across is the assign module, and this doesn't seem to use file paths in the export (or have issues with non-alphanumeric chars in file names) - presumably because it pulls directly from files table rather than using portfolio_rewrite_pluginfile_urls().
          Hide
          Jason Platts added a comment -

          This issue seems to have stalled...

          I'm happy to add url decoding to the custom paths also, but unless there is a core plugin or unit test that utilises this aspect then I don't see how the fix can be verified in testing.

          Show
          Jason Platts added a comment - This issue seems to have stalled... I'm happy to add url decoding to the custom paths also, but unless there is a core plugin or unit test that utilises this aspect then I don't see how the fix can be verified in testing.
          Hide
          Jason Fowler added a comment -

          you could write a unit test for it, that would be the best solution, even if there is a core plugin that utilises this aspect.

          Show
          Jason Fowler added a comment - you could write a unit test for it, that would be the best solution, even if there is a core plugin that utilises this aspect.
          Hide
          Jason Platts added a comment -

          There is no existing unit test framework/lib for portfolio API though

          See MDL-15666 - Seems to be going nowhere fast, I'm certainly not volunteering to write it in order to fix this bug

          I could fix the custom paths in this patch but not specifically test for that aspect as it doesn't appear to be used anywhere (only test in terms of regression).

          Show
          Jason Platts added a comment - There is no existing unit test framework/lib for portfolio API though See MDL-15666 - Seems to be going nowhere fast, I'm certainly not volunteering to write it in order to fix this bug I could fix the custom paths in this patch but not specifically test for that aspect as it doesn't appear to be used anywhere (only test in terms of regression).
          Hide
          Dan Poltawski added a comment -

          Sending back to integration to consider this

          Show
          Dan Poltawski added a comment - Sending back to integration to consider this
          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 -

          Thanks Jason, your argument makes sense. I've integrated to master, 24 and 23.

          Show
          Dan Poltawski added a comment - Thanks Jason, your argument makes sense. I've integrated to master, 24 and 23.
          Hide
          Andrew Davis added a comment -

          This doesn't appear to work in master. I haven't checked the other branches yet. My image doesn't appear in site_files, the site_files directory is empty.

          Here is the html from the zip file.

          <table class="glossarypost dictionary" cellspacing="0">
          <tr valign="top">
          <td class="entry">
          <div class="concept"><h3 class="main">word</h3></div> 
          <p>asdf<img src="site_files/nep tune.png" alt="" width="31" height="31"/></p></td></tr>
          </table>
          

          This looks correct.

          The file name is "nep tune.png" (without the quotes). I had a file called neptune.png and just put a space in the middle of the name. It displays fine within the glossary in Moodle but something is going wrong during the export process.

          Show
          Andrew Davis added a comment - This doesn't appear to work in master. I haven't checked the other branches yet. My image doesn't appear in site_files, the site_files directory is empty. Here is the html from the zip file. <table class= "glossarypost dictionary" cellspacing= "0" > <tr valign= "top" > <td class= "entry" > <div class= "concept" ><h3 class= "main" >word</h3></div> <p>asdf<img src= "site_files/nep tune.png" alt= "" width=" 31 " height=" 31"/></p></td></tr> </table> This looks correct. The file name is "nep tune.png" (without the quotes). I had a file called neptune.png and just put a space in the middle of the name. It displays fine within the glossary in Moodle but something is going wrong during the export process.
          Hide
          Andrew Davis added a comment -

          Attaching the image in question in case that's helpful.

          Show
          Andrew Davis added a comment - Attaching the image in question in case that's helpful.
          Hide
          Jason Platts added a comment -

          Andrew,

          I've tried the test with your attached image and it worked for me, this was on 2.4.
          So I'm not sure what the difference is and why you're not getting the image.

          Did you check the zip using winzip?

          Do you have developer debugging enabled - were there any warnings/errors during the export process?

          Show
          Jason Platts added a comment - Andrew, I've tried the test with your attached image and it worked for me, this was on 2.4. So I'm not sure what the difference is and why you're not getting the image. Did you check the zip using winzip? Do you have developer debugging enabled - were there any warnings/errors during the export process?
          Hide
          Andrew Davis added a comment -

          I'm not getting the image in the zip in 2.4 either.

          I'm using Ubuntu (Linux) so winzip isn't an option. I'm using Gnome Archive Manager.

          I do have developer debugging enabled. No warnings or errors unfortunately apart from those reported in MDLQA-5392.

          I'll see if there is anyone else around running Windows who can try this out. If it works for them it's probably something funny that's platform specific.

          Show
          Andrew Davis added a comment - I'm not getting the image in the zip in 2.4 either. I'm using Ubuntu (Linux) so winzip isn't an option. I'm using Gnome Archive Manager. I do have developer debugging enabled. No warnings or errors unfortunately apart from those reported in MDLQA-5392 . I'll see if there is anyone else around running Windows who can try this out. If it works for them it's probably something funny that's platform specific.
          Hide
          Jason Platts added a comment -

          Can you attach the zip to the tracker issue, then I can check that aspect.
          Thanks.

          Show
          Jason Platts added a comment - Can you attach the zip to the tracker issue, then I can check that aspect. Thanks.
          Hide
          Andrew Davis added a comment -

          Attaching the portfolio export.

          Show
          Andrew Davis added a comment - Attaching the portfolio export.
          Hide
          Damyon Wiese added a comment -

          Andrew - see if there has been any debugging added to the start of the zip file?

          I have seen some zip programs fail on files like this but others ignored it and just worked.

          (You can open the zip in vim or run "head blah.zip" and you should see the text and/or the start of zip header).

          Show
          Damyon Wiese added a comment - Andrew - see if there has been any debugging added to the start of the zip file? I have seen some zip programs fail on files like this but others ignored it and just worked. (You can open the zip in vim or run "head blah.zip" and you should see the text and/or the start of zip header).
          Hide
          Andrew Davis added a comment -

          There doesn't seem to be any debug there. The zip itself appears to be valid. It contains other files. Just not the image file.

          Show
          Andrew Davis added a comment - There doesn't seem to be any debug there. The zip itself appears to be valid. It contains other files. Just not the image file.
          Hide
          Jason Platts added a comment - - edited

          Using WinZip (Windows 7) I see the image and site_files folder in the zip attached to this issue. (If you look at the file size of the zip it is 2kb - same as the image).

          Could this be related to MDL-37332 - so because the directory in the zip has been created incorrectly you can't see it, whereas WinZip seems to be able to cope with this.

          Show
          Jason Platts added a comment - - edited Using WinZip (Windows 7) I see the image and site_files folder in the zip attached to this issue. (If you look at the file size of the zip it is 2kb - same as the image). Could this be related to MDL-37332 - so because the directory in the zip has been created incorrectly you can't see it, whereas WinZip seems to be able to cope with this.
          Hide
          Dan Poltawski added a comment -

          Yes, it sounds like it and it doens't sound like this is related to this issue, so i'm going to pass this test.

          (I can also open the zip on osx)

          Show
          Dan Poltawski added a comment - Yes, it sounds like it and it doens't sound like this is related to this issue, so i'm going to pass this test. (I can also open the zip on osx)
          Hide
          Dan Poltawski added a comment -

          Passing, based on the information we have I think this is safe.

          Show
          Dan Poltawski added a comment - Passing, based on the information we have I think this is safe.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          I feel myself really alone tonight! So was time to push your fixes upstream!

          "Lest we forget. We will remember them."

          Thanks and ciao!

          Show
          Eloy Lafuente (stronk7) added a comment - I feel myself really alone tonight! So was time to push your fixes upstream! "Lest we forget. We will remember them." Thanks and ciao!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: