Moodle
  1. Moodle
  2. MDL-31928

Url repository: remove duplicate images, resolve image path correctly

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3
    • Fix Version/s: 2.3
    • Component/s: Repositories
    • Labels:
    • Testing Instructions:
      Hide

      1. Enable URL repository
      2. Open file picker (from any text editor) and try to type URL addresses, make sure that all images are displayed and are not duplicated. Note that images found in CSS files are displayed but they may not be present on the page.
      3. Test picking file from repository with Javascript disabled

      Show
      1. Enable URL repository 2. Open file picker (from any text editor) and try to type URL addresses, make sure that all images are displayed and are not duplicated. Note that images found in CSS files are displayed but they may not be present on the page. 3. Test picking file from repository with Javascript disabled
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull Master Branch:
      wip-MDL-31928-master

      Description

      URL repository:

      • lists the same image several times if it occurs several times on the page
      • does not correctly resolve image path if it has a query string
      • images in included CSS files are not shown
      • non-JS file picker ignores thumbnail width and height attributes

        Gliffy Diagrams

          Activity

          Hide
          Dongsheng Cai added a comment -
          Show
          Dongsheng Cai added a comment - https://github.com/marinaglancy/moodle/compare/master...wip-MDL-31928-master#L2R455 What's this function for? It doesn't seem to be used
          Hide
          Marina Glancy added a comment -

          Hi Dongsheng, this was part of an external library that I updated. Do you think we should remove the function?

          Show
          Marina Glancy added a comment - Hi Dongsheng, this was part of an external library that I updated. Do you think we should remove the function?
          Hide
          Dongsheng Cai added a comment -

          Ha, it comes with the library, I thought you added it and not using it, just curious why it's there. Sure, we can keep it.

          It looks all good to me, Thanks.

          Show
          Dongsheng Cai added a comment - Ha, it comes with the library, I thought you added it and not using it, just curious why it's there. Sure, we can keep it. It looks all good to me, Thanks.
          Hide
          Dongsheng Cai added a comment -

          Submitting to integration review.

          Show
          Dongsheng Cai added a comment - Submitting to integration review.
          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 -

          Hmm, I have a bit of a concern about locallib.php (I know its an existing library!).

          The function names are fairly generic and I could imagine at risk of being redefined

          Show
          Dan Poltawski added a comment - Hmm, I have a bit of a concern about locallib.php (I know its an existing library!). The function names are fairly generic and I could imagine at risk of being redefined
          Hide
          Dan Poltawski added a comment -

          I, i've integrated this now.

          Just a note that i'm concerned by the library in repository/url/locallib.php because the 'namespace' its using is very very generic.

          It would not be hard to imagine another part of moodle introducing a encode_url() function and making this go bang.

          But for now it seems to be OK, if doing work on this in the future it would be good to address that.

          Show
          Dan Poltawski added a comment - I, i've integrated this now. Just a note that i'm concerned by the library in repository/url/locallib.php because the 'namespace' its using is very very generic. It would not be hard to imagine another part of moodle introducing a encode_url() function and making this go bang. But for now it seems to be OK, if doing work on this in the future it would be good to address that.
          Hide
          Adrian Greeve added a comment -

          Tested in master, I managed to find a site that had images and downloaded them with and without javascript enabled.
          Thanks.

          Show
          Adrian Greeve added a comment - Tested in master, I managed to find a site that had images and downloaded them with and without javascript enabled. Thanks.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          This has been near becoming rejected, because it's not the best code you are able to produce.

          But, luckily, at the end, it has landed and has been spread to all repos out there.

          Many thanks and, don't forget it, keep improving your skills, you can!

          Closing, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - This has been near becoming rejected, because it's not the best code you are able to produce. But, luckily, at the end, it has landed and has been spread to all repos out there. Many thanks and, don't forget it, keep improving your skills, you can! Closing, ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: