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

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

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.

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.

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.

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

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.

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

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

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.

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

(amended issue title)

Integrated, thanks! (23 and master)

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

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.