Issue #116: fix php versions api rate limit. Issue #117: allow theme …#118
Issue #116: fix php versions api rate limit. Issue #117: allow theme …#118gthomas2 wants to merge 2 commits into
Conversation
…to be configured via config prop
There was a problem hiding this comment.
Pull request overview
This PR addresses two user-facing areas: (1) making PHP version discovery resilient to GitHub API rate limiting, and (2) allowing Moodle’s $CFG->theme to be configured via the recipe config object.
Changes:
- Add
config.themesupport to the recipe model and Moodleconfig.phpTwig template. - Extend the hard-coded PHP version fallback list and add a rate-limit fallback path when querying GitHub branches.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
templates/moodle/config.php.twig |
Emits $CFG->theme when config.theme is set. |
src/Service/PHPVersions.php |
Adds 8.5 to fallback versions and returns fallback list when GitHub API rate limit is hit. |
src/Model/MoodleConfig.php |
Adds theme property to the MoodleConfig model. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public string $timezone = self::UNSET, | ||
| public string $defaultblocks = self::UNSET, | ||
| public string $theme = self::UNSET, | ||
| public bool $sslproxy = false |
There was a problem hiding this comment.
This PR adds a new config.theme option, but it isn’t documented alongside the other config properties. Please update the recipe/config documentation (e.g., README config property table) so users know this option exists and what values are expected.
| @@ -50,6 +51,11 @@ public function listVersions(): array { | |||
| return $hardCodedVersions; | |||
| } | |||
|
|
|||
| // Handle api rate limit issue. | |||
| if (!empty($branches['message']) && strpos($branches['message'], 'API rate limit exceeded') !== false) { | |||
| return $hardCodedVersions; | |||
| } | |||
There was a problem hiding this comment.
listVersions() now has another early-return path (rate limit), but the cURL handle is never closed on any return path. Please ensure curl_close($ch) always runs (e.g., via try/finally) so repeated calls don’t leak resources.
| // Handle api rate limit issue. | ||
| if (!empty($branches['message']) && strpos($branches['message'], 'API rate limit exceeded') !== false) { | ||
| return $hardCodedVersions; | ||
| } |
There was a problem hiding this comment.
The rate-limit check handles one specific GitHub error payload, but other API error responses are also JSON objects with a message field (not an array of branches). In those cases, the foreach ($branches as $branch) loop will hit a TypeError (not caught by catch (\Exception)), causing a fatal. Consider validating that $branches is a list of arrays with a name key before iterating, and/or catching \Throwable in the parsing block.
| // Handle api rate limit issue. | ||
| if (!empty($branches['message']) && strpos($branches['message'], 'API rate limit exceeded') !== false) { | ||
| return $hardCodedVersions; | ||
| } |
There was a problem hiding this comment.
This adds new behavior (fallback to hard-coded versions when the GitHub API rate limit is exceeded), but there’s no unit test coverage asserting the fallback behavior. Since the repo already uses PHPUnit for services, consider adding a test that simulates an API response containing a rate-limit message and verifies listVersions() returns the hard-coded list (likely by extracting the HTTP call behind a mockable method/service).
| {% if (config.theme != UNSET) %} | ||
| $CFG->theme = '{{config.theme}}'; | ||
| {% endif %} |
There was a problem hiding this comment.
A new config.theme field is now supported, but there’s no test asserting it is rendered into the generated Moodle config.php when set. Consider extending the existing MoodleConfigServiceTest to set $recipe->config->theme and assert the resulting config.php contains the expected $CFG->theme line.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…to be configured via config prop