Set CURLOPT_TIMEOUT to 30 seconds

moodle-an-hochschulen / moodle-local_staticpage

Moodle plugin which displays static information pages which exist outside any course, imprint or faq pages for example, complete with Moodle navigation and theme

GNU General Public License v3.0

46 stars 28 forks source link

Set CURLOPT_TIMEOUT to 30 seconds #35

Closed ariefwt closed 5 years ago

ariefwt commented 5 years ago

Hello, I'd like to suggest setting CURLOPT_TIMEOUT to ensure curl doesn't prevent the page from loading.

A little backstory: In at least one of the deployments we handle, our customer's gateway is configured such that all outbound connections will go on indefinitely, and this in turn cause this function to keep waiting forever and ever.

In this pull request, the CURLOPT_TIMEOUT is set to a generous amount of 30 seconds (following the default PHP max_execution_time), but the number could come from configuration if needed be.

This will also help to alleviate #32, which is similar but not as extreme as our customer's situation.

abias commented 5 years ago

Hi @ariefwt ,

this is a reasonale request and I would be happy to integrate this improvement.

However, I have two change requests:

Please don't hardcode the timeout value to 30 seconds, create an admin setting on /admin/settings.php?section=local_staticpage_settings instead as you have already mentioned. This way, the timeout can be set to the admin's local needs.
Please evaluate if you can change local_staticpage_check_availability() in a way that it returns not only availability= true or false but also the information if a timeout has been reached. I could imagine that the function does not return a boolean anymore, but instead one of three contants
```
LOCAL_STATICPAGE_AVAILABILITY_YES
LOCAL_STATICPAGE_AVAILABILITY_NO
LOCAL_STATICPAGE_AVAILABILITY_TIMEOUT
```

Thanks, Alex

ariefwt commented 5 years ago

Sorry for delayed response, but I've added timeout configuration and a more fine-grained response check as requested, as well as a control whether to check availability or not

Please review 😄

ariefwt commented 5 years ago

Hello @abias, how are things going? :-)

abias commented 5 years ago

Hi @ariefwt,

I am sorry for the lack of feedback in this issue.

I will include your PR in our upcoming plugin release for Moodle 3.7. Please stay tuned for some more days.

Thanks, Alex

abias commented 5 years ago

Hi @ariefwt ,

many thanks for your patience with this issue.

I finally was able to review your well-crafted pull request, do some refinements to align the code to the existing codebase (https://github.com/moodleuulm/moodle-local_staticpage/commit/1a49f63482f1c4cdec8d0eb52f7d04b4861d04b7) and merge it into master in the end.

Please expect the 3.7 release of the plugin which will contain this improvement within the next few days.