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
    • Rank:
      38578

      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

        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: