Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Fixes LRS usage with proxy to close #238. #239

Closed
wants to merge 3 commits into from
Closed

Conversation

ryasmi
Copy link
Member

@ryasmi ryasmi commented Aug 31, 2018

@peterspicer-catalyst thanks for reporting this #238, I'm unable to test this right now. Are you able to test this by following the instructions below?

  1. Navigate to the root directory of your Moodle installation in your terminal.
  2. Run the commands below in sequence to get this branch of the plugin.
    cd `admin/tool/log/store`
    rm -rf xapi
    git clone git@github.com:xAPI-vle/moodle-logstore_xapi.git xapi
    cd xapi
    php -r "readfile('https://getcomposer.org/installer');" | php; rm -rf vendor; php composer.phar install
    git checkout origin/fix-proxy
  3. Test that statements are sent from Moodle with a proxy configuration.
  4. Test that statements are sent from Moodle without a proxy configuration.

@ryasmi ryasmi self-assigned this Aug 31, 2018
@ryasmi ryasmi added the fix label Aug 31, 2018
@ryasmi ryasmi requested a review from ijeffro August 31, 2018 11:50
@peterspicer-catalyst
Copy link

Hi @ryansmith94

This change would work for our use case specifically - but ideally reusing the setup in Moodle's curl class might be better overall: it covers for proxy host, with or without a port number, and with or without a username/password to authenticate on the proxy.

I guess it depends how far down the rabbit hole to go - I would assume that an LRS would typically not be on the bypass list as well, so that would normally be something to not worry about.

I'm not sure how feasible it is to rewrite the sending function to actually use Moodle's curl class, but it'd make it a maintenance-free prospect in future. In the interim, it might be worth adding a check against the $CFG->proxyport setting too.

Peter

@ryasmi
Copy link
Member Author

ryasmi commented Aug 31, 2018

Hi @peterspicer-catalyst, thanks for the advice on the Moodle Curl Class, do you have a link to the source code for that? I'm having trouble finding it with Google. Once I've had a look at it, I'll determine how feasible it is to use the class itself rather than re-implementing aspects of it. For now it's likely that we'll just re-implement the proxy aspect and refactor to use the entire class later.

@peterspicer-catalyst
Copy link

Hi @ryansmith94

Moodle's curl class lives in lib/filelib.php - https://github.com/moodle/moodle/blob/master/lib/filelib.php#L2952 for the class itself in Moodle master.

It's mostly a case of instantiating the curl class, and its post method, where the first parameter is the URL to POST to, the second parameter should be an array of the parameters put into the POST body, and the third parameter should be an array of any extra options you want to pass into cURL itself. Should also give you back status codes etc. for determining if successful.

Peter

@ryasmi
Copy link
Member Author

ryasmi commented Aug 31, 2018

Thanks @peterspicer-catalyst I'm investigating this now. Thanks for the details on how to use the class as well, the only issue I can see with us using that class is that during our automated testing we don't actually have access to the class. Having said that, we don't usually run our tests against the LRS loader, we usually run it against a loader that just returns the statement so that we can check it matches what we'd expect and that it's valid. Using the class for the LRS loader would prevent us from testing the LRS loader locally though when needed, we might be able to create a separate loader that uses the curl class though.

@ryasmi
Copy link
Member Author

ryasmi commented Aug 31, 2018

Just tested that statements are sent from Moodle without a proxy configuration. Will try to find a way to test that statements are sent from Moodle with a proxy configuration on Monday, then merge this. I also have the fix-proxy-moodlecurl branch working, it adds a new loader that uses the Moodle curl, will test that further on Monday too.

@ryasmi
Copy link
Member Author

ryasmi commented Sep 3, 2018

Hi @peterspicer-catalyst, do you have a link to any documentation for configuring the proxy in Moodle?

@peterspicer-catalyst
Copy link

Hi @ryansmith94 unfortunately Moodle's docs page doesn't really talk about proxyhost or proxyport, and I think the most comprehensive information to be had on it is actually on the page where you put the values in for Moodle's configuration itself! (If in doubt, use the search function for "proxyhost")

https://docs.moodle.org/35/en/HTTP for the relevant manual page though.

Proxies are something that I'm not particularly versed in, I'm afraid - our sysadmins tell me that we have proxies in front of some clients based on their needs and configurations and I put the relevant details into Moodle and then everything works. Beyond that, hard to say - however as long as the relevant options are passed into cURL, my experience has been that it's pretty transparent.

Peter

@ryasmi
Copy link
Member Author

ryasmi commented Sep 3, 2018

Hi @peterspicer-catalyst, ah that's unfortunate. Thanks for the search tip, I usually use that and Google, just couldn't find anything this time. Thanks for the manual page.

It sounds like by testing that this works without the proxy settings, it should be enough to verify that it should work with the proxy settings since we're using Moodle's curl class.

Moving forward I'll close this PR in favour of #241, since these changes are no longer required to fix #238.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants