Skip to content

YULMS-2: Fix to Prevent fatal error in get_custom_error_message when …#60

Open
leverforce wants to merge 1 commit intobycs-lp:mainfrom
leverforce:fix/safer-getresponse
Open

YULMS-2: Fix to Prevent fatal error in get_custom_error_message when …#60
leverforce wants to merge 1 commit intobycs-lp:mainfrom
leverforce:fix/safer-getresponse

Conversation

@leverforce
Copy link
Copy Markdown

Problem
In the ChatGPT connector of the moodle-local_ai_manager plugin, a fatal error occurs during failed API requests when the thrown exception is a ConnectException.

The code attempts to call $exception->getResponse() without checking if the method exists. Unfortunately, ConnectException does not implement getResponse(), leading to:

PHP Fatal error: Uncaught Error: Call to undefined method GuzzleHttp\Exception\ConnectException::getResponse()

Root Cause
The connector was making multiple direct calls to getResponse() on any exception object passed to the error handler. Some exceptions (e.g., ConnectException) do not implement this method, resulting in a fatal crash.

Solution
Introduced a safe check using method_exists() to verify the existence of getResponse() before calling it. The response is now stored in a variable to avoid redundant method calls and improve readability.

Benefits

  • Prevents fatal errors in AI evaluation workflows during network or API outages.
  • Makes error handling in get_custom_error_message() fault-tolerant.

Affected file
local/ai_manager/tools/chatgpt/classes/connector.php

Notes
This issue surfaced when the AI service was unreachable (Because of a negative account balance) and the Moodle instance had local AI Manager enabled with the ChatGPT connector. This patch ensures safe fallback behavior in all such cases.
An added complication is that the error reported appears in the UI of the plugin being used. In my case, this was https://github.com/marcusgreen/moodle-qtype_aitext. Users might think the issue is with the qtype_aitext plugin but it is really with local_ai_manager

…getResponse() is undefined

- Safely checks if getResponse exists and only calls it once
- Prevents crash when handling ConnectException in ChatGPT connector
case 400:
if (method_exists($exception, 'getResponse') && !empty($exception->getResponse())) {
$responsebody = json_decode($exception->getResponse()->getBody()->getContents());
$response = method_exists($exception, 'getResponse') ? $exception->getResponse() : null;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure I get the point of the fix...

Current code:

  • Checks if getResponse() exists
  • if it exists it also checks if the getResponse() is not empty, so also if not null etc.
  • if not the rest of the code will not be called

Your code:

  • Checks if getResponse() exists
  • if it exists it will check, if will check if $exception->getResponse() is not empty like the current code
  • if not it will be set to zero and empty($reponse) will also evaluate to true

Maybe I'm missing out on a tiny detail, but could you please explain in which case your code is different from the current one? Thanks!

Copy link
Copy Markdown
Author

@leverforce leverforce Apr 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Memmel. This is a great plugin...thanks so much.

I think the issue is that Guzzle can throw a ConnectException not just a RequestException and this is the type of exception that occurs when I try to connect to openAI with a negative account balance.
When Guzzle throws a ConnectException, it means the connection failed before even reaching the server. Since no response ever came back, there's nothing to wrap — so there's no response object at all.
$response in a RequestException can be null, but in the case of ConnectException, the method getResponse() doesn’t even exist — because it’s not part of that class

(check https://github.com/guzzle/guzzle/blob/7.9/UPGRADING.md#function-guzzlehttpexceptionconnectexceptiongetresponse-is-removed)

Currentcode: checks method_exists() to see if getResponse() exists — but then it calls $exception->getResponse() again later, multiple times:
...once inside empty(...)
...and again when reading the response body for json_decode(...)

Calling getResponse() again like that is what causes the fatal error — because it’s not defined in ConnectException, and PHP crashes when trying to call a method that doesn’t exist on the actual exception object.

Proposed code only calls $exception->getResponse() once, and only if it actually exists (checked with method_exists()).
It stores the result in a $response variable. Then, later parts of the code use that variable — not the method call again.
This is the important part: it means getResponse() is never called at all if it doesn’t exist, so the fatal error cannot happen.

About the error and testing:

Initial issue: I had a negative balance with OpenAI → the server refused connections → Guzzle threw a ConnectException → That was expected and legitimate.

After restoring balance:
I topped up my OpenAI account. Now the connection worked again, but the error kept happening in Moodle even after fixing my account.

The problem wasn't that OpenAI was rejecting requests anymore — it was that Moodle was still trying to process an old exception and wasn't correctly handling retries or queued requests.

The original code did this: second and third call to $exception->getResponse() assumed the method still exists. So even if the original problem (connection failure) was gone, any attempt to process a leftover ConnectException would still crash Moodle.

That’s because the plugin didn’t just fail when it hit a connection issue — it kept trying to handle the exception in a way that assumed there was a proper response object to work with.
With the fix, if the method doesn't exist, it’s never called again. So even if a stale ConnectException is floating around, it won’t crash anything anymore.
Moodle skips over it.

I will try to find a test routine that does not involve touching the account balance in OpenAI

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for explaining. So just to be clear: The problem is that by calling getResponse() on the exception object, I kind of "consume" the response. After that it will not be available by calling getResponse() again, right?

That makes sense and I've already stumbling over this behavior while processing guzzle requests.

If you can provide a clear way of testing this, this would be great (maybe it's even possible to mock something in a unit test?)

Also please make sure codechecker is happy.

Again thank you very much!

@PhMemmel
Copy link
Copy Markdown
Member

Hi @leverforce ,

thank you very much for providing the patch.

While your description totally makes sense I am wondering how your code is different from mine in detail. I commented in the code.

Could you also maybe provide a way to test the behavior properly?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants