Moodle
  1. Moodle
  2. MDL-33453

Clarify search_references[_count]() methods usage and add tests for them

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.3, 2.4
    • Fix Version/s: 2.3.1
    • Component/s: Files API
    • Labels:
      None
    • Testing Instructions:
      Hide

      Testing difficulty: trivial

      Make sure that the filemanager correctly reports the number and the list of aliases that refer to a given file.

      Show
      Testing difficulty: trivial Make sure that the filemanager correctly reports the number and the list of aliases that refer to a given file.
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull from Repository:
    • Rank:
      41349

      Description

      Methods like file_storage::search_references() or search_references_count() need to accept $repositoryid too. The reference field is supposed to be unique per repository instance, not world-wide.

        Issue Links

          Activity

          Hide
          David Mudrak added a comment -

          Information loss risk, API change required. Thus setting to MUST FIX for 2.3 release.

          Show
          David Mudrak added a comment - Information loss risk, API change required. Thus setting to MUST FIX for 2.3 release.
          Hide
          David Mudrak added a comment -

          Looking at this now in these 5-to-12 times

          Show
          David Mudrak added a comment - Looking at this now in these 5-to-12 times
          Hide
          David Mudrak added a comment -

          OK, it turned out that I was initially confused a bit by what these two methods are supposed to do. I did not realize that they are supposed to be used in the filemanager mainly, to get the list and the number of aliases that refer to a single stored_file.

          So I extended the PHPdocs to highlight this and added some unittests that illustrate the usage of these methods.

          Please note that this patch relies on 6feae1d256448b0a9867a010065deccfc3f54527 that has been already integrated.

          Show
          David Mudrak added a comment - OK, it turned out that I was initially confused a bit by what these two methods are supposed to do. I did not realize that they are supposed to be used in the filemanager mainly, to get the list and the number of aliases that refer to a single stored_file. So I extended the PHPdocs to highlight this and added some unittests that illustrate the usage of these methods. Please note that this patch relies on 6feae1d256448b0a9867a010065deccfc3f54527 that has been already integrated.
          Hide
          Aparup Banerjee added a comment -

          Hi David, is this still a MUST FIX for 2.3 ? i'm not seeing API changes really.

          • also , count_records_sql() already seems to return (int) , not sure why we're casting again.
          • this seems like a unintuitive way to run some checks. potentially misleading in the function name too.
          • perhaps unpack_reference() could do with a @throws in the phpdoc to make it seem like its running some checks.
          Show
          Aparup Banerjee added a comment - Hi David, is this still a MUST FIX for 2.3 ? i'm not seeing API changes really. also , count_records_sql() already seems to return (int) , not sure why we're casting again. this seems like a unintuitive way to run some checks. potentially misleading in the function name too. perhaps unpack_reference() could do with a @throws in the phpdoc to make it seem like its running some checks.
          Hide
          Aparup Banerjee added a comment -

          reopening, thanks for the phpdoc explanation verbosity but we can revisit this after 2.3 release perhaps.

          ps: not even sure what the bug is here that is being fixed with the current patch, but surely the checks are a good thing.

          Show
          Aparup Banerjee added a comment - reopening, thanks for the phpdoc explanation verbosity but we can revisit this after 2.3 release perhaps. ps: not even sure what the bug is here that is being fixed with the current patch, but surely the checks are a good thing.
          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
          David Mudrak added a comment -

          Thanks for the review and comments Apu. However, I must object.

          • The fact that I did not change the API does not IMHO mean that this could not be integrated. I tried to explain the patch in the commit message, I'm sorry if it was not clear enough. Anyway, I fully respect your decision to discuss this more. The patch was intended to make it clear what those two methods as supposed to be used for. And there are unittests attached to illustrate it, too. Previously, it was a bit confusing (at least for me - that is why I reported this issue).
          • Why do you think that count_records_sql() returns (int)? As I can say reading the code, it returns either integer 0 or numerical string. There is another issue for it. However, the patch fixing that has not been integrated yet and it was breaking my unittest. So explicit cast sounded like a good idea to me.
          • Well, calling a method to give it a chance to throw exception may be unintuitive but in this particular case it works pretty well. I added an explicit comment explaining the usage. If you can suggest a better way to check the valid format of the reference field, I'll be happy to amend the patch.
          • Throwing the exception and making a check like this is not the primary function of the unpack_reference() method. Anyway, I agree it may help to understand the situation so I added @throws into the patch.
          Show
          David Mudrak added a comment - Thanks for the review and comments Apu. However, I must object. The fact that I did not change the API does not IMHO mean that this could not be integrated. I tried to explain the patch in the commit message, I'm sorry if it was not clear enough. Anyway, I fully respect your decision to discuss this more. The patch was intended to make it clear what those two methods as supposed to be used for. And there are unittests attached to illustrate it, too. Previously, it was a bit confusing (at least for me - that is why I reported this issue). Why do you think that count_records_sql() returns (int)? As I can say reading the code, it returns either integer 0 or numerical string . There is another issue for it. However, the patch fixing that has not been integrated yet and it was breaking my unittest. So explicit cast sounded like a good idea to me. Well, calling a method to give it a chance to throw exception may be unintuitive but in this particular case it works pretty well. I added an explicit comment explaining the usage. If you can suggest a better way to check the valid format of the reference field, I'll be happy to amend the patch. Throwing the exception and making a check like this is not the primary function of the unpack_reference() method. Anyway, I agree it may help to understand the situation so I added @throws into the patch.
          Hide
          David Mudrak added a comment -

          Rebased against the fresh MOODLE_23_STABLE and amended according Apu's suggestions. Re-submitting for integration.

          Show
          David Mudrak added a comment - Rebased against the fresh MOODLE_23_STABLE and amended according Apu's suggestions. Re-submitting for integration.
          Hide
          David Mudrak added a comment -

          Linking MDL-33568 that is currently submitted into integration, too.

          Show
          David Mudrak added a comment - Linking MDL-33568 that is currently submitted into integration, too.
          Hide
          Aparup Banerjee added a comment -

          Hi David,

          • ah thanks for that, i wasn't sure what the bug was, its clear to me now its about removing confusion. thanks.
          • from phpdoc: count_records_sql() returns int ; thats where i saw it. drilling down i see in get_field_sql() a call to reset() , anyway thanks for linking this other issue for handling that, didn't see that before. atm tho its possible to get a false returned from count_records_sql(), so perhaps the issues should be related for integration at same time.
          • no better way, unless theres a refactor, i don't see any real need to atm, i guess its fine.
          • and atleast its a little more clearer now. thanks David.
          Show
          Aparup Banerjee added a comment - Hi David, ah thanks for that, i wasn't sure what the bug was, its clear to me now its about removing confusion. thanks. from phpdoc: count_records_sql() returns int ; thats where i saw it. drilling down i see in get_field_sql() a call to reset() , anyway thanks for linking this other issue for handling that, didn't see that before. atm tho its possible to get a false returned from count_records_sql(), so perhaps the issues should be related for integration at same time. no better way, unless theres a refactor, i don't see any real need to atm, i guess its fine. and atleast its a little more clearer now. thanks David.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          As far as MDL-33568 is going to be 2.4 only and this is for 2.3.x... I think it's ok to integrate this now... so I'm getting onto it, Aparup. Everything looks perfect, IMO.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - As far as MDL-33568 is going to be 2.4 only and this is for 2.3.x... I think it's ok to integrate this now... so I'm getting onto it, Aparup. Everything looks perfect, IMO. Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          (amended issue title)

          Show
          Eloy Lafuente (stronk7) added a comment - (amended issue title)
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated, thanks! (23 and master)

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated, thanks! (23 and master)
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Passing:

          • phpunit tests completed ok.
          • I created multiple aliases from server files and the count/details of aliases was shown perfectly in the original always.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Passing: phpunit tests completed ok. I created multiple aliases from server files and the count/details of aliases was shown perfectly in the original always. Ciao
          Hide
          Sam Hemelryk added a comment -

          Congratulations your code is upstream - gold star for you!

          This issue + 79 others made it in in time for the minor releases.
          Thank you everyone involved for your exuberant efforts.

          Show
          Sam Hemelryk added a comment - Congratulations your code is upstream - gold star for you! This issue + 79 others made it in in time for the minor releases. Thank you everyone involved for your exuberant efforts.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: