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

Url repository: remove duplicate images, resolve image path correctly

    Details

    • Type: Bug
    • Status: Closed
    • Priority: 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

          Attachments

            Activity

            Hide
            dongsheng Dongsheng Cai added a comment -
            Show
            dongsheng 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 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 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 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 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 Dongsheng Cai added a comment -

            Submitting to integration review.

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

            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
            poltawski 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
            poltawski 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
            poltawski 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
            abgreeve 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
            abgreeve 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
            stronk7 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
            stronk7 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:
                  Fix Release Date:
                  25/Jun/12