Re-instate 'Fix aws library conflicts in Totara'

[sammarshallou]

PR #64 seemed to get delayed because @sarahjcotton didn't have time to make the required simple code fix.

I agree with the requested change and in case it's useful, this PR is just Sarah's change modified to change the operator as requested by @mark-webster-catalyst - see #64 for all information

We need this because we're developing on 4.2 now and it doesn't work at all without it...

(There might be some clever way to add a branch to an existing pull request but I don't know it so I filed a new one, apologies - obviously feel free to close this if appropriate.)

[sammarshallou]

Hmmm.. I've now actually tested this on our test infrastructure and although (with the fix to use || instead of &&) it doesn't give any errors, it also doesn't fix the problem (on Moodle 4.2, not Totara). Still getting an exception like this:

Exception - Undefined constant GuzzleHttp\Client::VERSION

line 136 of /local/aws/sdk/GuzzleHttp/functions.php: Error thrown line 40 of /local/aws/sdk/Aws/Handler/GuzzleV6/GuzzleHandler.php: call to GuzzleHttp\default_user_agent() line 88 of /local/aws/sdk/Aws/WrappedHttpHandler.php: call to Aws\Handler\GuzzleV6\GuzzleHandler->__invoke()

Now why is that... (looking)

[sammarshallou]

OK so I did this:

require_once($CFG->dirroot . '/local/aws/sdk/aws-autoloader.php'); $cls = new ReflectionClass('GuzzleHttp\Client'); print_object($cls->getFileName());

So the error actually occurs because the core version of GuzzleHttp in lib/guzzlehttp is the newer version 7, which removes the version constant, which in the version included here was present with a comment saying it was deprecated and would be removed in version 6.5.

I don't know offhand how to change the autoloader so that the one in local_aws takes priority over the one in core, but I'll look it up...

[sarahjcotton]

@sammarshallou Have you seen the conversation in this issue? Not sure if there's something in there that might help: https://github.com/catalyst/moodle-local_aws/issues/56 Thanks so much for picking this up

[sammarshallou]

Thanks @sarahjcotton - I did skim it but admit I didn't understand most of the comments :) I've made a change in my second commit so the autoloader prioritises the classes in local_aws rather than the ones in core moodle - this is a bit sketchy since if there is ever a case where somebody already loaded the core Guzzle before loading the aws-autoloader.php then it would break [and conversely if somebody loads aws-autoloader then does something which relies on the core version of Guzzle], but then it's always going to be sketchy if there are two (incompatible) copies of the same library. Am now deploying this to a test server to see if it actually works.

[mark-webster-catalyst]

@sammarshallou #58 sorts the issue with Moodle 4.2 (and I believe will fix totara, too)

[sammarshallou]

@mark-webster-catalyst OK - I have confirmed on our test server that the changes in this branch do fix it (at least for our situation) in Moodle 4.2 (no idea about Totara as I just took Sarah's change for that, it's possible I might have broken it there again with my second commit).

I'll revert my changes on our test server and put in the changes from #58 and confirm that fixes it as well, then I'll close this one!

[mark-webster-catalyst]

@sammarshallou maybe don't be so hasty. If yours does fix both it looks like it might be the neater solution

[sammarshallou]

@mark-webster-catalyst Test has deployed and the change in #58 does also work for our situation. I thought that one might have been more widely tested which could be an advantage vs. this one, but obviously I don't mind if this PR is used instead - as long as something fixes it. :) I shan't close this one then, will leave it to you or others to decide.

[sarahjcotton]

Hi @sammarshallou my colleague @SimonThornett is going to post a suggestion on using some of my code from the original PR in 58 with yours. This has come up again due to this tracker issue: https://tracker.moodle.org/browse/MDL-72935

Would you be kind enough to perhaps add that to this PR so we can get things moving again please?

Once that's done, would you mind having a look please @danmarsden .

Thanks so much for your efforts with this.

[SimonThornett]

Hi @sammarshallou,

I've been having a look at this as it related to some testing issues in the tracker ticket Sarah mentioned above. Looking at the two pulls #66 and #58 I think that a combination of both would be good.

Using yours as a base adding Sarah's checks for 402 and the conditional loading of /lib/guzzlehttp/guzzle/src/functions_include.php would mean that we can keep with a singular branch that works with both newer and older Moodle's as well as Totara.

Additionally, switching the requires to require_onces will mitigate an issue that we saw where it had attempted to load the same file multiple times. It's possible that this might be mitigated already with your prepend flag on the spl_autoload_register however I have not tested that myself.

Hope this helps.

[sammarshallou]

@SimonThornett Sorry it took me so long to get back to this.

I've had a think about it and I think the problem with my code is that it is designed to ensure that the embedded verison of Guzzle (6.5) is used. That's what the autoloader parameter change does.

But, #58 makes it use the core version of Guzzle, if available. This is sort of the opposite. Since the core version is newer, that seems like a more sensible approach.

I think the bug without these is that it uses a mixture - the functions.php from local_aws and the rest of it from core - which is what breaks.

One thing that does still concern me with #58 is that I kind of think the PSR7 and Promise things within GuzzleHtttp maybe should also use the core version...