Allow report results to be retrieved via WS via /webservice/pluginfile

[jwalits]

This PR addresses the points raised in #105

• One change from the original spec. The dataformat paramter has been made as a required parameter. The reason being there is no straight forward way of getting top configured dataformat from the core Moodle API if no value is provided.
• The download.php endpoint is also no longer needed with this change, thus has been removed and code refactored on the view.php file

[brendanheywood]

@timhunt anything else we can do to get this over the line?

[timhunt]

@brendanheywood warp space-time, so I get 25 hours in a day? ;-) Thanks for the gentle nudge. I will see if I can get to this today. Just kicked off the CI build.

[brendanheywood]

hhmm, I think the behat failures are probably due the to moodle 4 nav changes

[timhunt]

Agreed. Bonus marks if you do a fix, but that won't stop this getting reviewed and landing.

[brendanheywood]

Another gentle bump

We've also got a bunch of plugins failing on 4.0 master and we'll likely have to just remove it from ci and cut new plugin branches to support it when it's actually stable. It's too much of a moving target at the moment.

[timhunt]

OK, this implementation is a lot better. Thanks for sticking with this. But ...

1. It is not normal to mix pluginfile URLs and GET parameters. Why do you think it is necessary here? It seems like an abuse of the indended design of Moodle file serving to me. (e.g. question_pluginfile in lib/questionlib.php does not do this in the if ($filearea === 'export') bit.)
2. Surely there are APIs for creating pluginfile URLs? Please use them, rather than hard-coding.
3. download_dataformat_selector will accept a moodle_url, so no point converting it to a string. (Also, I think you call to ->out() should have been ->out(false) to avoid double-escaping the &s.)
4. This should go without saying: any new feature requires automated tests. (I had to make this point in relation to the previous commit, so disappointing I have to say it again.)
5. The commit comment does not follow best practice.
6. The PHPdoc comments in lib.php convey no useful information. What is report_customsql_pluginfile doing? What does the file path need to be to make this work?
7. The whole point of this work is to make it possible to download report results with a web-service user. But, it feels a bit "if a tree falls over in the middle of a forest, does it make a sound?" Please document this in an appropriate place (probably internaldoc/functionality.txt).

So, to give an better aswer to @brendanheywood's question, "Anything else we can do to get this over the line?" You can review the code yourself, and work with your colleages to fix the issues before expecting me to spend more of my time on this.

[timhunt]

Re 1) I guess we could take the view that this code is not going into Moodle core, and therefore it is acceptable to use the "Mixing pluginfile URLs and GET parameters" hack here. But, I think it is a hack, and what is going on needs to be documented with comments in the code.

[brendanheywood]

1) Tim I'm not aware of any code standard for pluginfile urls forbidding get params, can you point me at the docs for this?

I'm not saying that any of these are correct but trawling the logs of a prod site shows many examples in core that do this already:

/pluginfile.php/###/course/section/###/essential-readings.png?time=###
/pluginfile.php/###/question/questiontext/###/Figure%###_##P.PNG?time=###
/pluginfile.php/###/mod_folder/content/#/Sample%##Video_Group%##Assignment_#.mp3?forcedownload=###
/pluginfile.php/###/mod_resource/content/###interactive/story.html?embed=###

[timhunt]

I am not aware of anything else which does do this.

forcedownload is a specific part of the file download system.

time puzzles me. I expect something is adding that to make absolutely sure the file is not serverd from any web cache. However, I cannot find the code responsibile - which would hopefully reveal more about the intent.

I don't see any time in our logs, Instead I see some rev=... which is again clearly just about defeating caching, so I guess that time is to do with your reverse proxy setup.

So, I am still feeling that this use of GET params is unprecidented - unless you keep searching and show me a real precident.

[jwalits]

Hi @timhunt

Thank for the in-depth feedback. To address the points raised, I have made a new commit with the changes.

1. The mixing of path and GET parameters, whilst not ideal, it does not seem to be forbidden in Moodle core. I was able to find a couple of examples using this. E.g https://github.com/moodle/moodle/blob/master/mod/folder/lib.php#L619-L623. This is also unavoidable in our case as the current code is using the in-built download_dataformat_selector method. This adds the dataformat as a GET variable to the url. We also need to have the ability to pass in arbitrary number of parameters as defined by the user in the sql report. The code cannot assume, which value matches which key in the sql report if the pluginfile path contains for example /foo/bar/random/param.
2. Thank you for pointing out the pluginfile API. I have updated the code to make use of the make_pluginfile_url method
3. Have updated to pass the moodle_url object directly rather than using ->out()
4. I have added some unit tests for testing the download urls
5. Once you are happy with the latest changes, I will squash the commits with a more suitable commit message
6. Have updated the PHPDocs for lib.php to add some more information, inc. example urls
7. Thanks for the link, I hadn't come across that text file previously. But I have added a new section in there for download

Please let us know if there is something else missing from the latest changes.

Thanks

[jwalits]

Hi @timhunt

Wish you a very happy new year, hope you had an enjoyable break over Christmas and new years.

Just a gentle bump on this if you had a chance to review the new set of changes?

Thanks.

[timhunt]

Finally. Sorry it too me so long to look at this (again!). But, all good now, and merged.