Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-37942

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

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.4.1
    • Fix Version/s: 2.3.7, 2.4.4
    • Component/s: Portfolio
    • 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 Master Branch:
      wip-MDL-37942-master

      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.

        Gliffy Diagrams

        1. nep tune.png
          2 kB

          Issue Links

            Activity

            Hide
            phalacee 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
            phalacee 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
            jp76 Jason Platts added a comment -

            Added 23 stable branch fix.

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

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

            Show
            cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
            Hide
            jp76 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
            jp76 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
            jp76 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
            jp76 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
            phalacee 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
            phalacee 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
            jp76 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
            jp76 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
            poltawski Dan Poltawski added a comment -

            Sending back to integration to consider this

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

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

            Show
            poltawski Dan Poltawski added a comment - Thanks Jason, your argument makes sense. I've integrated to master, 24 and 23.
            Hide
            andyjdavis 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
            andyjdavis 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
            andyjdavis Andrew Davis added a comment -

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

            Show
            andyjdavis Andrew Davis added a comment - Attaching the image in question in case that's helpful.
            Hide
            jp76 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
            jp76 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
            andyjdavis 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
            andyjdavis 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
            jp76 Jason Platts added a comment -

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

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

            Attaching the portfolio export.

            Show
            andyjdavis Andrew Davis added a comment - Attaching the portfolio export.
            Hide
            damyon 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 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
            andyjdavis 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
            andyjdavis 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
            jp76 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
            jp76 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
            poltawski 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
            poltawski 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
            poltawski Dan Poltawski added a comment -

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

            Show
            poltawski Dan Poltawski added a comment - Passing, based on the information we have I think this is safe.
            Hide
            stronk7 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
            stronk7 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:
                  Fix Release Date:
                  13/May/13