Skip to content

Issue #116: fix php versions api rate limit. Issue #117: allow theme … #118

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.

Sign up for GitHub

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

Open
gthomas2 wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Issue #116: fix php versions api rate limit. Issue #117: allow theme … #118

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
882a493
Issue #116: fix php versions api rate limit. Issue #117: allow theme …
gthomas2 Mar 11, 2026
df1075c
Update templates/moodle/config.php.twig
gthomas2 Mar 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/Model/MoodleConfig.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ public function __construct(
public string $lang = self::UNSET,
public string $timezone = self::UNSET,
public string $defaultblocks = self::UNSET,
public string $theme = self::UNSET,
public bool $sslproxy = false
Comment on lines 11 to 14
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
) { }
}
8 changes: 7 additions & 1 deletion src/Service/PHPVersions.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,8 @@ public function listVersions(): array {
'8.1',
'8.2',
'8.3',
'8.4'
'8.4',
'8.5'
];

try {
Expand All @@ -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;
}
Comment on lines 42 to +57
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
Comment on lines +54 to +57
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
Comment on lines +54 to +57
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

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

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).

Copilot uses AI. Check for mistakes.

// Loop through the branches and extract the PHP version from the branch name
try {
$phpVersions = [];
Expand Down
4 changes: 4 additions & 0 deletions templates/moodle/config.php.twig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,10 @@ $CFG->behat_profiles['host-chrome'] = [
$CFG->lang = '{{config.lang}}';
{% endif %}

{% if (config.theme != UNSET) %}
$CFG->theme = '{{config.theme}}';
{% endif %}
Comment on lines +83 to +85
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.

{% if (config.timezone != UNSET) %}
date_default_timezone_set('{{config.timezone}}');
{% endif %}
Expand Down
Loading