diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f7a09fd4..112525e6 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -2,13 +2,16 @@ name: Codechecker CI on: [push, pull_request] +env: + TZ: Australia/Sydney + jobs: test: runs-on: ubuntu-22.04 services: postgres: - image: postgres:15 + image: postgres:16 env: POSTGRES_USER: 'postgres' POSTGRES_HOST_AUTH_METHOD: 'trust' @@ -35,35 +38,33 @@ jobs: - php: 8.3 moodle-branch: main database: mariadb + - php: 8.4 + moodle-branch: MOODLE_502_STABLE + database: pgsql + - php: 8.4 + moodle-branch: MOODLE_501_STABLE + database: pgsql - php: 8.3 moodle-branch: MOODLE_500_STABLE database: pgsql - php: 8.3 moodle-branch: MOODLE_405_STABLE database: pgsql - - php: 8.3 - moodle-branch: MOODLE_404_STABLE - database: pgsql - - - php: 8.2 - moodle-branch: MOODLE_403_STABLE - database: pgsql - - php: 8.2 - moodle-branch: MOODLE_402_STABLE - database: pgsql - - - php: 8.1 - moodle-branch: MOODLE_401_STABLE - database: pgsql # Lowest php versions supported by each branch (with main always being tested twice). - - php: 8.2 + - php: 8.3 moodle-branch: main database: pgsql - - php: 8.2 + - php: 8.3 moodle-branch: main database: mariadb + - php: 8.3 + moodle-branch: MOODLE_502_STABLE + database: pgsql + - php: 8.2 + moodle-branch: MOODLE_501_STABLE + database: pgsql - php: 8.2 moodle-branch: MOODLE_500_STABLE database: pgsql @@ -71,20 +72,6 @@ jobs: - php: 8.1 moodle-branch: MOODLE_405_STABLE database: pgsql - - php: 8.1 - moodle-branch: MOODLE_404_STABLE - database: pgsql - - - php: 8.0 - moodle-branch: MOODLE_403_STABLE - database: pgsql - - php: 8.0 - moodle-branch: MOODLE_402_STABLE - database: pgsql - - - php: 7.4 - moodle-branch: MOODLE_401_STABLE - database: pgsql steps: - name: Check out repository code uses: actions/checkout@v4 diff --git a/CHANGES.md b/CHANGES.md index 77fe1578..7c2e9a79 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -1,3 +1,60 @@ +Changes in version 5.2.3 (20260515) - Worked examples +------------------------------------------------------- +- Expanded the on-screen path help with worked examples covering a + plugin, a subplugin (e.g. the tiny_bootstrap TinyMCE editor plugin), + an activity module and a single file, and clarified that a component + name maps to its directory. + +Changes in version 5.2.2 (20260515) - Mind the group +------------------------------------------------------- +- Submit the web check form to the explicit /local/codechecker/index.php + endpoint instead of the bare /local/codechecker/ directory URL. The + plugin already registers and loads the page as index.php, so posting + to the directory forced the web server to resolve the directory index + on a different, stricter-permission code path - returning a bare 403 + Forbidden on "Check" (while the page itself loaded fine) whenever the + plugin directory was not traversable by the web server user. The form + now uses the same endpoint as the rest of the plugin, so it no longer + depends on server-side directory permissions. +- Documented the related file-permission guidance in the README. + +Changes in version 5.2.1 (20260515) - Forbidden no more +------------------------------------------------------- +- Process the web check submission in-place instead of doing a + Post/Redirect/Get. The PRG redirect placed the filesystem path in the + URL query string, which nginx LFI/path-traversal WAF rules (e.g. + YunoHost defaults) reject with a 403 before the request reaches + Moodle. The path now stays in the POST body; a GET fallback is kept + for direct links. +- Pointed the README CI badge at this repository instead of the + upstream moodlehq one, so forks show their own pipeline status. +- Note: the WAF/query-string explanation above was the initial + diagnosis. The 403 was later traced to the form posting to the bare + directory URL on a stricter web-server permission path; the + definitive fix is in 5.2.2. This change is retained as a sensible + improvement but was not the actual cure. + +Changes in version 5.2.0 (20260515) - G'day, 5.2 +------------------------------------------------------- +- Confirmed compatibility with Moodle 5.1 and 5.2. +- Fixed the find_other_files test data provider keys (notmatches -> + nomatches) so the tests pass under PHPUnit 11 (Moodle 5.0+), which + maps associative data-provider keys to named parameters. +- Reformatted the plugin's own source to pass the bundled Moodle CS + v3.7.0 standard (phpcbf, formatting only - no behaviour change). +- Added GitHub Actions CI coverage for Moodle 5.1 (MOODLE_501_STABLE, + PHP 8.2 to 8.4) and Moodle 5.2 (MOODLE_502_STABLE, PHP 8.3 to 8.4). +- Fixed thirdpartylibs.xml: removed the stale PHPCompatibility entry (the + library was unbundled in 5.1.0) that broke the grunt ignorefiles task, + and synced the remaining versions with composer.lock. +- Bumped the CI PostgreSQL service to 16, required by Moodle 5.2 and main. +- Raised the CI main-branch jobs to PHP 8.3 (Moodle main now requires + PHP 8.3.0 and no longer composer-installs on 8.2). +- Removed CI testing for Moodle versions before 4.5 (4.1 to 4.4). +- Added the Australia/Sydney timezone to the CI environment. +- Bundled the Moodle CS v3.7.0 coding style rule sets + (https://github.com/moodlehq/moodle-cs). + Changes in version 5.1.0 (20251202) - No longer compatible ------------------------------------------------------- - Bundled dependencies updated: diff --git a/CLAUDE.md b/CLAUDE.md new file mode 100644 index 00000000..5b14abdd --- /dev/null +++ b/CLAUDE.md @@ -0,0 +1,501 @@ +# CLAUDE.md — Moodle Plugin Development + +Guidance for Claude when developing Moodle plugins for Australian deployments. +These rules are drawn from real mistakes and CI failures across the +`verzog/moodle-*` plugin suite — treat them as a pre-flight checklist, not +background reading. + +**Requirements:** PHP 8.2 – 8.4, Moodle 5.0 – 5.2, plugin with `version.php` and +`$plugin->component`. CI via GitHub Actions only. + +> **Project baseline:** this codebase targets **Moodle 5.0 – 5.2**. Moodle 5.0 +> drops PHP 8.1, so the PHP floor is **8.2**. Moodle 5.2 supports PHP 8.3 – 8.4. +> No Moodle 4.x or PHP 8.1 compatibility is built, tested, or supported. + +--- + +## 1. CI — GitHub Actions Setup + +GitHub Actions (GHA) is the only supported CI provider. No Travis CI references. + +### Configuration + +- **Runner image:** `ubuntu-24.04`. `ubuntu-22.04` has been retired and every + job will fail with `moodle-plugin-ci: command not found`. +- **Workflow file:** Copy `gha.dist.yml` from `moodle-plugin-ci` and save as + `.github/workflows/moodle-ci.yml`. +- **AU timezone:** Add `TZ: Australia/Sydney` to the workflow `env` so date/time + tests respect AEST/AWST offsets. +- **`max_input_vars`:** Set `PHP_INI_VALUES: max_input_vars=5000` (or equivalent + `php.ini` step) — Moodle's PHPUnit bootstrap requires it. +- **Matrix:** PHP 8.2, 8.3, 8.4 × `pgsql` + `mysqli`, branches + `MOODLE_500_STABLE`, `MOODLE_501_STABLE`, `MOODLE_502_STABLE`, and `main` + (covers 5.3-dev until cut). Moodle 5.1 supports PHP 8.2 – 8.4; 5.2 supports + PHP 8.3 – 8.4 — gate matrix combos accordingly. +- **`mysqli` driver:** run against `mariadb:10.11` (moodle-plugin-ci's own + `gha.dist` choice). Avoids the `mysql:8.0` init-health race. +- **Service images:** pin `postgres:16.6` and `mysql:8.4.3` (or `mariadb:10.11`) + by tag — `:latest` drift caused silent breakage. +- **Install resiliency:** wrap `composer create-project` and `moodle-plugin-ci + install` in a 3-attempt retry with linear backoff; give the install step + `id: install` and gate later steps on `steps.install.conclusion == 'success'` + so a flake doesn't cascade into `command not found` noise. + +### CI Commands + +| Command | Purpose | AU Compliance Note | +|----------------|--------------------------------|-------------------------------------------------------| +| `phplint` | PHP syntax errors | PHP 8.2 – 8.4 compatibility | +| `codechecker` | Moodle Coding Standards | 4-space indent; new line before `{` on classes/fns | +| `phpdoc` | PHPDoc checker | Requires `@package`, `@copyright`, `@license` | +| `validate` | Plugin structure | Checks `version.php` and Frankenstyle component name | +| `savepoints` | Upgrade step validation | `db/upgrade.php` version increments | +| `mustache` | Mustache template lint | No hardcoded text; use `{{#str}}` tags | +| `grunt` | Compile/lint JS & CSS | SCSS lint; AMD modules in `amd/src/` | +| `phpunit` | Back-end unit tests | Extends `advanced_testcase`; `$this->resetAfterTest()`| +| `behat` | Acceptance tests | Use DD/MM/YYYY in all scenarios; `--auto-rerun 3` | + +### CI Failure Diagnosis + +If **every** matrix job fails identically within a few seconds (before the +install step), suspect infrastructure — not code. Check for a retired runner +image, an org Actions policy, or a GitHub billing/spending limit. Diagnose +before pushing workflow edits; a blind runner-image bump wastes a cycle. + +### Optimisation + +- Remove unused steps (e.g. skip Mobile App testing for desktop-only plugins). +- Use Behat tags (`--tags="@local_myplugin"`) to focus tests during development. +- Use `--auto-rerun 3` in Behat to absorb flaky failures. + +### `$plugin->supported` and `main` + +Don't add `$plugin->supported` if the matrix also tests `main`. An upper bound +fails `validate` on `main` jobs (the dev branch reports a higher version than +the cap). Use the `requires` floor + README compatibility strapline instead. + +--- + +## 2. Australian Locale & Coding Style + +### Language & Spelling + +Use AU/UK English exclusively in `lang/en/` files. + +| Forbidden | Required | +|------------|------------| +| Customize | Customise | +| Organize | Organise | +| Color | Colour | +| Behavior | Behaviour | +| Enrollment | Enrolment | + +### Dates & Currency + +- Render all user-facing dates via `userdate()` — defaults to DD/MM/YYYY. +- Currency defaults to AUD with 2-decimal precision. +- For locale-formatted decimal input in forms, parse via `unformat_float()`. +- When offering a currency picker, source it from + `\core_payment\helper::get_supported_currencies()` so it only lists + currencies a configured gateway will accept. + +### Privacy — Australian Privacy Principles (APP) + +Implement `privacy/classes/provider.php` using the Moodle Privacy API to +comply with the APP. Plugins that store no personal data of their own +(themes, presentation blocks, façades over another plugin's data) should +implement `\core_privacy\local\metadata\null_provider` rather than skipping. + +See §4 Security Defaults for the `MOODLE_INTERNAL` rule. + +--- + +## 3. Hard-Won Rules (Real CI Failures) + +### 3.1 Language Files: Strict Ordering, No Section Comments + +`phpcs` runs as `moodle-plugin-ci phpcs --max-warnings 0` — warnings fail +the build. The `moodle.Files.LangFilesOrdering` sniff requires: + +- Every `$string['key']` in **ascending byte order** (`strcmp` / `LC_ALL=C sort` + — case-sensitive; `:` < `A` < `_` < `a`). +- **No comments between strings.** The `// Navigation.`-style dividers are + flagged as `UnexpectedComment`. Only the license header comment is allowed. + +Add strings anywhere, then re-sort the whole file. Verify with: + +```bash +grep -oP "^\$string\['\K[^']+" lang/en/local_myplugin.php > /tmp/k +LC_ALL=C sort /tmp/k | diff - /tmp/k && echo "ORDER OK" +``` + +Lang files do **not** get a `defined('MOODLE_INTERNAL') || die();` guard — +`phpcs` flags it as unnecessary there. + +### 3.2 `fullname()` Needs the Full Name Field Set + +Never hand-pick `u.firstname, u.lastname` for a record passed to `fullname()` — +it raises an `E_USER_NOTICE` about missing phonetic/alternate fields. Use: + +```php +$namefields = \core_user\fields::for_name()->get_sql('u', true)->selects; +$sql = "SELECT DISTINCT u.id{$namefields} FROM {user} u ..."; +``` + +With `SELECT DISTINCT`, every `ORDER BY` column must appear in the select list +— PostgreSQL enforces this and CI will catch it. + +### 3.3 Rewrite `@@PLUGINFILE@@` Before Formatting + +Editor content saved via `file_postupdate_standard_editor()` stores embedded +images as `@@PLUGINFILE@@` placeholders. They will 404 on the rendered page +unless you rewrite before calling `format_text()`: + +```php +format_text( + file_rewrite_pluginfile_urls( + $html, 'pluginfile.php', + $context->id, 'local_myplugin', MY_FILEAREA, $record->id + ), + $format, ['context' => $context] +); +``` + +Every file area you serve must also be whitelisted in the plugin's +`local_myplugin_pluginfile()` callback, or it 404s even after rewriting. + +### 3.4 AMD: Edit src, Rebuild build via Moodle's Grunt, Bump Version + +`amd/src/*.js` is not what runs — Moodle loads `amd/build/*.min.js`. After +editing source, regenerate the minified bundle using **Moodle's own grunt +pipeline** (`grunt amd` from the moodleroot), not a standalone bundler — the +wrapper, source maps, and AMD shim it produces are what `mustache` and the +loader expect. If grunt is unavailable, produce a hand-minified build matching +the existing wrapper byte-for-byte. Then bump `version.php` and purge all +caches. + +### 3.5 Bump `version.php` for Any Cached Asset Change + +Templates, AMD, lang strings, DB schema, capabilities, and Mustache helpers +all require a higher `$plugin->version` to take effect on an existing install. +When parallel PRs are in flight, give each a **distinct** version number to +avoid merge collisions. For static JS/CSS loaded outside AMD, append +`?v=$plugin->version` to cache-bust browsers. + +### 3.6 Optional File Areas Need an Explicit Clear-on-Disable + +`file_prepare_draft_area()` always populates the draft, so a hidden filemanager +still round-trips its existing files. Delete the area on save when a toggle is +off: + +```php +if (!empty($data->haspanorama)) { + file_save_draft_area_files(...); +} else { + get_file_storage()->delete_area_files( + $context->id, 'local_myplugin', FILEAREA_PANORAMA, $record->id + ); +} +``` + +Use `advcheckbox` + `$mform->hideIf('panorama_image', 'haspanorama', 'notchecked')` +for the reveal. + +### 3.7 Form Actions: Point to Explicit `index.php`, Not the Directory + +A `moodleform` action of `new moodle_url('/local/myplugin/')` (bare directory) +forces the web server to resolve the directory index on submit — a different, +stricter-permission path than the page load. If the plugin directory isn't +traversable by the web server user, the page loads fine but **Submit returns a +bare 403**. Always target the explicit endpoint the plugin registers: + +```php +$mform = new myplugin_form(new moodle_url('/local/myplugin/index.php')); +``` + +Same lesson, GET variant: don't shove filesystem-looking values into the query +string. nginx/WAF LFI rules (e.g. YunoHost defaults) reject those with a bare +403 before they reach Moodle. Handle the POST in-place; keep the admin page +URL clean. + +### 3.8 Moodle 5.1+ `public/` Directory Layout + +On Moodle 5.1+ the docroot is `/public/`; plugins live at +`public/local/...`, `$CFG->dirroot` points at the `public/` dir, and +`config.php` may sit in `public/` (back-compat) or the project root. Account +for both layouts when reasoning about paths or bootstrap +(`require '../../config.php'`). Note: `moodle-plugin-ci` does not exercise the +`public/` split — passing CI says nothing about path resolution on 5.1+. + +### 3.9 PHP 8.4: No Implicit-Nullable Parameters + +PHP 8.4 deprecates `function foo(array $x = null)`. `phpunit --fail-on-warning` +fails on 8.4 while 8.2 / 8.3 pass silently. Mark every nullable parameter +explicitly: + +```php +public function add_instance($course, ?array $fields = null) { ... } +``` + +Audit for `lcg_value`, deprecated `mysqli_*`, and removed curly-brace string +offsets at the same time. + +### 3.10 Enrol-instance Forms: Use `get_default_enrol_roles()` + +The default-role select on an enrol-instance edit form uses +`get_default_enrol_roles($context, $defaultroleid)` — the helper that +`enrol_fee` / `enrol_self` / `enrol_paypal` use. `extend_assignable_roles()` +does **not** exist on `enrol_plugin` and calling it fatals on form open. + +### 3.11 Enrol Cost/Currency Live on `mdl_enrol` + +The enrolment subsystem already owns `mdl_enrol.cost` and `mdl_enrol.currency` +(shared with `enrol_fee` / `enrol_paypal`). Add the fields to the +instance-edit form and write to those columns — **no schema change needed**. +Validate with `unformat_float()` and reject negatives. + +### 3.12 Cross-plugin Dependencies: Pin to a Numeric Version + +`$plugin->dependencies = ['local_educheckout' => 2026060200];` — don't use +`ANY_VERSION` once the suite is shipping together. CI dependency checkouts +(`actions/checkout` with `repository: verzog/moodle-foo`) should use an +**explicit `ref:`** (after a `master` → `main` rename, the GitHub redirect +silently keeps things working until it doesn't — be explicit). + +### 3.13 Block Migration: `{block}.name` Unique Violation + +Block plugin install registers a row in `{block}` for the new component +**before** `db/install.php` runs. If you `set_field('block', 'name', +'newname', ['name' => 'oldname'])` during migration, you hit the unique +`name` index and the install aborts. Detect the new-name row first and **drop +the leftover old row** in that case; only rename when the new row isn't there +yet. + +### 3.14 N+1 Queries in Cron / Migration Loops + +`get_record(...)` inside a per-row loop is the classic N+1 trap (e.g. +`sync_enrolments()` lazy-loading the enrol instance for every user-enrolment +row). Pre-load with a single `get_records(..., '', 'id, ...')` keyed by id +**before** the loop; `continue` past stale ids defensively. For bulk renames +across DB engines, use a single SQL `UPDATE ... SET col = REPLACE(col, 'a', +'b')` — `REPLACE()` is supported on MySQL/MariaDB and PostgreSQL. + +### 3.15 Server-side Validation of JSON Payloads + +External functions that accept JSON-encoded data (e.g. rubric scores) must +validate after `json_decode`: array shape, expected length, every value in +the allowed set. Throw `invalid_parameter_exception` before any DB write. +Don't trust the front-end to enforce structural constraints. + +### 3.16 Stranded Commits After Merge + +If a PR is merged and then more commits are pushed to the (now-deleted) head +branch, those commits are **stranded** — no open PR points at them and `main` +never receives them. Always confirm the PR diff contains the intended change +**before** merging, or open a follow-up PR for additional commits. + +### 3.17 Revert PRs: Verify the Root Cause First + +Don't immediately revert a merged PR because it superficially resembles a +suspected regression. Verify the cause first; an unnecessary revert + re-land +cycle (PR → revert → re-land) wastes a release version and pollutes history. + +### 3.18 CI Badge: Point at the Fork + +A README badge hardcoded to `moodlehq/moodle-…` renders **upstream's** status +on a fork (typically red). Repoint to `verzog/`, pin to `main`. + +### 3.19 Marketplace Hygiene + +Before first directory submission: + +- `LICENSE` file at plugin root (GPLv3 stub) — Marketplace blocker. +- `$plugin->maturity = MATURITY_BETA;` on the first submission of a suite. + Promote to `MATURITY_STABLE` only after a clean fresh-install + upgrade + pass in production. +- `TERMS.md` if the plugin will be sold via the Marketplace — adapt the + template from `moodle-tool_installfromgithub`, scope §8 (Acceptable use) + and §9 (Privacy) to the plugin. +- Document install-time migration behaviour (`db/install.php`) in the README + so reviewers understand what touches the DB on first install. + +### 3.20 Modals: Use `core/modal`, Not Editor `windowManager` + +For TinyMCE plugins and the like, build dialogs on `core/modal` rather than +fighting TinyMCE's `windowManager` for z-index. Filepicker overlay z-index +hacks (`yui3-widget-mask`, `moodle-dialogue-lightbox`, `!important`) are a +smell — switch to `core/modal` instead. Scope any `file_picker_callback` to +your own dialogs. + +### 3.21 Payment Flow Hardening + +For `core_payment`-driven flows: enforce idempotency on the callback (same +`paymentid` must not double-enrol), guard against logged-out / mismatched +accounts, and write structured logs for every state transition. Server-side +capacity / sold-out checks must live in **every** add-to-cart entry point, +not just the UI button. + +--- + +## 4. Security Defaults + +- **`MOODLE_INTERNAL` guard:** Add `defined('MOODLE_INTERNAL') || die();` to + every PHP file **except** lang files and `lib.php` files that contain only + function declarations (phpcs flags the guard as unnecessary in those cases). +- **SQL:** Always use Moodle's query placeholders (`?` or named params) — never + interpolate user input into SQL strings. +- **Capabilities:** check `require_capability()` at the entry point of every + external function and admin action. Don't rely on UI gating alone. +- **JS:** Set user/content text with `textContent` / + `document.createTextNode`, never `innerHTML`. +- **Inline style/script from settings:** Strip the closing tag + (`str_ireplace('dependencies` pinned to a numeric version, not `ANY_VERSION`. +- [ ] User-facing strings in `lang/en/` — no hardcoded text anywhere. +- [ ] Dates rendered via `userdate()`; currency defaults to AUD. +- [ ] Cost/currency forms validate with `unformat_float()`; currency picker + sourced from `\core_payment\helper::get_supported_currencies()`. +- [ ] No `innerHTML` with untrusted/user data. +- [ ] Capability check at the top of every external function and admin entry. +- [ ] Privacy API implemented (or `null_provider`). +- [ ] `LICENSE` present at plugin root (and `TERMS.md` if Marketplace-bound). +- [ ] Workflow file is GitHub Actions only — no Travis CI references. +- [ ] Runner is `ubuntu-24.04`; `TZ: Australia/Sydney`; `max_input_vars=5000`. +- [ ] Service images pinned; install step retried; later steps gated on it. +- [ ] CI matrix: PHP 8.2/8.3/8.4 × pgsql/mysqli × Moodle 5.0/5.1/5.2/main. +- [ ] `$plugin->supported` absent if `main` is in the matrix. +- [ ] Form actions point to explicit `index.php`, not a bare directory URL. +- [ ] Block migrations check for the new-component `{block}` row before + renaming the old one. +- [ ] No N+1 loops; bulk renames use a single `UPDATE … REPLACE()`. +- [ ] PR diff confirmed to contain the intended change before merging. + +--- + +## 8. Workflow + +- One concern per PR; branch off the latest `main`. +- Default branch is `main`. After a `master` → `main` rename, update the + workflow's push trigger and any cross-repo `actions/checkout` `ref:` values + — the GitHub redirect handles refs but not file contents. +- If a branch falls behind merged work, rebase onto `origin/main` and + force-with-lease. +- Open PRs **ready for review** (not draft); keep PR bodies to a summary + + test plan. +- CI runs: `phplint`, `phpcs --max-warnings 0`, `phpdoc`, `validate`, + `savepoints`, `phpunit --fail-on-warning`, `behat` across a PHP × Moodle- + branch matrix. **Warnings are failures.** +- Don't merge until the fix commits are on the branch. Commits pushed after a + merge are stranded with no open PR (§3.16). +- Don't revert a merged PR without verifying root cause (§3.17). + +--- + +## 9. Deployment & Live-Site Diagnosis + +CI green and PR merged does **not** mean the change is live. Most "still broken +after the fix" reports are deployment issues, not code. + +### Merge ≠ Deploy + +`main` on GitHub never touches a running site. After merging: redeploy the +files, then run the Moodle upgrade (*Site admin → Notifications* or +`php admin/cli/upgrade.php`). `version.php` must be bumped (§3.5) or no +upgrade/cache-purge fires. + +### Verify the Bytes on Disk + +Grep a known marker from the new code in the deployed file before trusting it: + +```bash +grep -n "MARKER_FROM_THE_FIX" /path/to/moodle/local/myplugin/index.php +``` + +Don't assume a deploy worked — confirm the file changed. + +### Deploy Ownership + +Deploying as a non-web user (rsync/git pull as a deploy account) sets the +wrong group, so the web server can't traverse the plugin directory → 403 on +directory-resolved requests. Match owner:group to the rest of the Moodle tree: + +```bash +rsync -a --chown=appuser:www-data ... +``` + +Re-apply after every deploy if needed. + +### Get Server Evidence Before Theorising + +For "works in CI / fails on server", read the per-vhost nginx logs — not the +global ones: + +```bash +sudo tail -n 50 /var/log/nginx/-error.log +sudo grep ' 403 ' /var/log/nginx/-access.log | tail +``` + +The access log line shows exact method and URL. `(13: Permission denied)` means +filesystem permissions; a WAF or nginx `deny` logs differently. Get this +first — hours were lost theorising (WAF, redirects, framework layout) before +the log named the actual cause. diff --git a/README.md b/README.md index 400e559e..18201655 100644 --- a/README.md +++ b/README.md @@ -1,7 +1,7 @@ Moodle Code Checker =================== -[![Codechecker CI](https://github.com/moodlehq/moodle-local_codechecker/actions/workflows/ci.yml/badge.svg)](https://github.com/moodlehq/moodle-local_codechecker/actions/workflows/ci.yml) +[![Codechecker CI](https://github.com/verzog/moodle-local_codechecker/actions/workflows/ci.yml/badge.svg?branch=main)](https://github.com/verzog/moodle-local_codechecker/actions/workflows/ci.yml) Information ----------- @@ -34,6 +34,18 @@ should see a new option in the settings block: > Site administration -> Development -> Code checker +Web server file permissions +--------------------------- + +The form submits to ``/local/codechecker/index.php`` so the request is +always handled by PHP directly, matching how the page itself is served. +If you still see a bare ``403 Forbidden`` only when pressing "Check" +(while the form page loads fine), check that the ``codechecker`` +directory is readable and traversable by your web server user or group, +matching the rest of your Moodle tree. This can happen when the plugin +is deployed as a non-web account (for example ``rsync`` or ``git pull`` +run as a deploy user). + We hope you find this tool useful. Feel free to enhance it! Also, you can report any idea or bug using GitHub's issues and pull requests, thanks! diff --git a/classes/privacy/provider.php b/classes/privacy/provider.php index 0a4e6fe5..e5f5ee9a 100644 --- a/classes/privacy/provider.php +++ b/classes/privacy/provider.php @@ -24,7 +24,6 @@ * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ class provider implements \core_privacy\local\metadata\null_provider { - /** * Get the language string identifier with the component's language * file to explain why this plugin stores no data. diff --git a/classes/report.php b/classes/report.php index 01b9d2db..f746bfc4 100644 --- a/classes/report.php +++ b/classes/report.php @@ -36,7 +36,6 @@ * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ class report extends \PHP_CodeSniffer\Reports\Xml { - /** * Generate a partial report for a single processed file. * @@ -58,7 +57,7 @@ public function generateFileReport($report, \PHP_CodeSniffer\Files\File $phpcsFi } // Here we are, with a file with 0 errors and warnings. - $out = new \XMLWriter; + $out = new \XMLWriter(); $out->openMemory(); $out->setIndent(true); @@ -71,6 +70,5 @@ public function generateFileReport($report, \PHP_CodeSniffer\Files\File $phpcsFi echo $out->flush(); return true; - } } diff --git a/classes/runner.php b/classes/runner.php index 5247d6f9..de64f2c7 100644 --- a/classes/runner.php +++ b/classes/runner.php @@ -35,7 +35,6 @@ * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ class runner extends \PHP_CodeSniffer\Runner { - /** * Create an instance of the runner. */ @@ -144,7 +143,7 @@ public function run() { echo $e->getMessage(); return $e->getCode(); } catch (\Exception $e) { - $error = 'Problem during processing; checking has been aborted. The error message was: '.$e->getMessage(); + $error = 'Problem during processing; checking has been aborted. The error message was: ' . $e->getMessage(); $file->addErrorOnLine($error, 1, 'Internal.Exception'); } $file->cleanUp(); diff --git a/index.php b/index.php index 41cfa9c7..8e752348 100644 --- a/index.php +++ b/index.php @@ -33,33 +33,48 @@ throw new coding_exception('Unable to find the "vendor" directory within the plugin. Please, install the plugin again.'); } -$pathlist = optional_param('path', '', PARAM_RAW); -$exclude = optional_param('exclude', '', PARAM_NOTAGS); -$includewarnings = optional_param('includewarnings', true, PARAM_BOOL); -$showstandard = optional_param('showstandard', false, PARAM_BOOL); - -$pageparams = []; -if ($pathlist) { - $pageparams['path'] = $pathlist; -} -if ($exclude) { - $pageparams['exclude'] = $exclude; -} -$pageparams['includewarnings'] = $includewarnings; -$pageparams['showstandard'] = $showstandard; - -admin_externalpage_setup('local_codechecker', '', $pageparams); +// The admin page URL is deliberately kept clean (no filesystem path in the +// query string): some web-server WAF rules (e.g. nginx LFI / path-traversal +// filters, like YunoHost's defaults) reject such requests with a 403. +admin_externalpage_setup('local_codechecker'); // We are going to need lots of memory and time. raise_memory_limit(MEMORY_HUGE); set_time_limit(600); -$mform = new local_codechecker_form(new moodle_url('/local/codechecker/')); -$mform->set_data((object)$pageparams); +// Submit to the explicit index.php endpoint (the same URL the admin menu +// and page registration use), not the bare directory. Posting to the +// directory would force the web server to resolve the directory index +// itself - a different path with stricter permission requirements that +// returns a 403 on submit if the plugin dir is not traversable by the +// web server user, even though the page itself loads fine. +$mform = new local_codechecker_form(new moodle_url('/local/codechecker/index.php')); + if ($data = $mform->get_data()) { - redirect(new moodle_url('/local/codechecker/', $pageparams)); + // Handle the submission in-place instead of doing a Post/Redirect/Get. + // A PRG redirect would expose the filesystem path in the URL query + // string, which the WAF rules mentioned above block before the request + // ever reaches Moodle. Keeping the path in the POST body avoids that. + $pathlist = $data->path; + $exclude = $data->exclude; + $includewarnings = !empty($data->includewarnings); + $showstandard = !empty($data->showstandard); +} else { + // Backwards compatibility for direct GET links. These may still be + // blocked by the same WAF rules when the path is present in the query. + $pathlist = optional_param('path', '', PARAM_RAW); + $exclude = optional_param('exclude', '', PARAM_NOTAGS); + $includewarnings = optional_param('includewarnings', true, PARAM_BOOL); + $showstandard = optional_param('showstandard', false, PARAM_BOOL); } +$mform->set_data((object)[ + 'path' => $pathlist, + 'exclude' => $exclude, + 'includewarnings' => $includewarnings, + 'showstandard' => $showstandard, +]); + $output = $PAGE->get_renderer('local_codechecker'); echo $OUTPUT->header(); @@ -110,7 +125,7 @@ // Look for other problems, not handled by codesniffer. Use same list of ignored (originally in keys, now in values). local_codechecker_check_other_files(local_codechecker_clean_path($fullpath), $xml, array_keys($ignores)); - list($numerrors, $numwarnings) = local_codechecker_count_problems($xml); + [$numerrors, $numwarnings] = local_codechecker_count_problems($xml); // Output the results report. echo $output->report($xml, $numerrors, $numwarnings, $showstandard); diff --git a/lang/en/local_codechecker.php b/lang/en/local_codechecker.php index 6d3ee723..84c33c0d 100644 --- a/lang/en/local_codechecker.php +++ b/lang/en/local_codechecker.php @@ -40,8 +40,9 @@ $string['filesummary'] = '{$a->path} - {$a->count}'; $string['includewarnings'] = 'Include warnings'; $string['info'] = '

Checks code against some aspects of the {$a->link}.

-

Enter a path relative to the Moodle code root, for example: {$a->path}.

-

You can enter either a specific PHP file, or to a folder to check all the files it contains. +

Enter one or more paths (one per line) relative to the Moodle code root. The component name maps to its directory, for example:

+{$a->path} +

You can enter either a specific PHP file, or a folder to check all the files it contains. Multiple entries are supported (files or folders), one per line.

To exclude files, a comma separated list of substr matching paths can be used, for example: {$a->excludeexample}. Asterisks are allowed as wildchars at any place.

'; $string['invalidpath'] = 'Invalid path {$a}'; diff --git a/locallib.php b/locallib.php index f2c7c3e3..5187d7e6 100644 --- a/locallib.php +++ b/locallib.php @@ -50,9 +50,17 @@ protected function definition() { $mform = $this->_form; $a = new stdClass(); - $a->link = html_writer::link('https://moodledev.io/general/development/policies/codingstyle', - get_string('moodlecodingguidelines', 'local_codechecker')); - $a->path = html_writer::tag('tt', 'local/codechecker'); + $a->link = html_writer::link( + 'https://moodledev.io/general/development/policies/codingstyle', + get_string('moodlecodingguidelines', 'local_codechecker') + ); + $a->path = html_writer::alist([ + html_writer::tag('tt', 'local/codechecker') . ' - a plugin', + html_writer::tag('tt', 'lib/editor/tiny/plugins/bootstrap') . + ' - a subplugin (here the tiny_bootstrap TinyMCE editor plugin)', + html_writer::tag('tt', 'mod/forum') . ' - an activity module', + html_writer::tag('tt', 'local/codechecker/locallib.php') . ' - a single file', + ]); $a->excludeexample = html_writer::tag('tt', 'db, backup/*1, *lib*'); $mform->addElement('static', '', '', get_string('info', 'local_codechecker', $a)); @@ -84,15 +92,19 @@ function autoload_tools(): bool { $vendordir = ''; $phpcsdir = ''; // Verify that we have the vendor directory at hand (it contains all the tools). - if (is_dir(__DIR__ . '/vendor') && - file_exists(__DIR__ . '/vendor/autoload.php')) { + if ( + is_dir(__DIR__ . '/vendor') && + file_exists(__DIR__ . '/vendor/autoload.php') + ) { $vendordir = __DIR__ . '/vendor'; } else { return false; } // Verify that the vendor dir has phpcs installed. - if (is_dir($vendordir . '/squizlabs/php_codesniffer') && - file_exists($vendordir . '/squizlabs/php_codesniffer/autoload.php')) { + if ( + is_dir($vendordir . '/squizlabs/php_codesniffer') && + file_exists($vendordir . '/squizlabs/php_codesniffer/autoload.php') + ) { $phpcsdir = $vendordir . '/squizlabs/php_codesniffer'; } else { return false; @@ -135,7 +147,7 @@ function local_codesniffer_get_ignores($extraignorelist = '') { foreach ($plugintypes as $type => $ignored) { $plugins = core_component::get_plugin_list_with_file($type, 'thirdpartylibs.xml', false); foreach ($plugins as $plugin => $path) { - $files[$type.'_'.$plugin] = $path; + $files[$type . '_' . $plugin] = $path; } } } @@ -171,7 +183,8 @@ function local_codesniffer_get_ignores($extraignorelist = '') { // Manually add our own phpcs stuff to be excluded. $paths[] = preg_quote(local_codechecker_clean_path( - '/local/codechecker' . DIRECTORY_SEPARATOR . 'phpcs')); + '/local/codechecker' . DIRECTORY_SEPARATOR . 'phpcs' + )); // Changed in PHP_CodeSniffer 1.4.4 and upwards, so we apply the // same here: Paths go to keys and mark all them as 'absolute'. @@ -324,7 +337,7 @@ function local_codechecker_find_other_files(&$arr, $folder, $ignores, $extension * @param string $key key within language file ('other_' will be prepended) * @param bool $warning if true is warning, otherwise error */ -function local_codechecker_add_problem($fileinxml, $file, $line, $key, $warning=false) { +function local_codechecker_add_problem($fileinxml, $file, $line, $key, $warning = false) { $type = $warning ? 'warning' : 'error'; $counter = $warning ? 'warnings' : 'errors'; diff --git a/renderer.php b/renderer.php index 45ce411e..1af55c88 100644 --- a/renderer.php +++ b/renderer.php @@ -50,10 +50,18 @@ public function summary_start($numfiles) { */ public function summary_line($fileindex, $prettypath, $numproblems) { if ($numproblems) { - return html_writer::tag('li', html_writer::link(new moodle_url('#file' . $fileindex), - get_string('filesummary', 'local_codechecker', - ['path' => s($prettypath), 'count' => $numproblems])), - ['class' => 'fail']); + return html_writer::tag( + 'li', + html_writer::link( + new moodle_url('#file' . $fileindex), + get_string( + 'filesummary', + 'local_codechecker', + ['path' => s($prettypath), 'count' => $numproblems] + ) + ), + ['class' => 'fail'] + ); } else { return html_writer::tag('li', s($prettypath), ['class' => 'good']); } @@ -70,11 +78,17 @@ public function summary_line($fileindex, $prettypath, $numproblems) { public function summary_end($numfiles, $summary, $type) { $output = html_writer::end_tag('ul'); if ($summary) { - $output .= html_writer::tag('h2', get_string('summary', 'local_codechecker', - $summary), ['class' => $type]); + $output .= html_writer::tag('h2', get_string( + 'summary', + 'local_codechecker', + $summary + ), ['class' => $type]); } else { - $output .= html_writer::tag('h2', get_string('success', 'local_codechecker'), - ['class' => 'good']); + $output .= html_writer::tag( + 'h2', + get_string('success', 'local_codechecker'), + ['class' => 'good'] + ); } return $output; } @@ -86,7 +100,10 @@ public function summary_end($numfiles, $summary, $type) { */ public function invald_path_message($path) { return $this->output->notification(get_string( - 'invalidpath', 'local_codechecker', s($path))); + 'invalidpath', + 'local_codechecker', + s($path) + )); } /** @@ -104,8 +121,11 @@ public function report(SimpleXMLElement $xml, $numerrors, $numwarnings, $showsta $grandsummary = ''; $grandtype = ''; if ($numerrors + $numwarnings > 0) { - $grandsummary = get_string('numerrorswarnings', 'local_codechecker', - ['errors' => $numerrors, 'warnings' => $numwarnings]); + $grandsummary = get_string( + 'numerrorswarnings', + 'local_codechecker', + ['errors' => $numerrors, 'warnings' => $numwarnings] + ); if ($numerrors) { $grandtype = 'fail error'; } else { @@ -172,11 +192,15 @@ public function report(SimpleXMLElement $xml, $numerrors, $numwarnings, $showsta * @return string HTML to output. */ public function problems($fileindex, $fileinxml, $prettypath) { - $output = html_writer::start_tag('div', - ['class' => 'resultfile', 'id' => 'file' . $fileindex]); + $output = html_writer::start_tag( + 'div', + ['class' => 'resultfile', 'id' => 'file' . $fileindex] + ); $output .= html_writer::tag('h3', html_writer::link( - new moodle_url('/local/codechecker/', ['path' => $prettypath]), - s($prettypath), ['title' => get_string('recheckfile', 'local_codechecker')])); + new moodle_url('/local/codechecker/', ['path' => $prettypath]), + s($prettypath), + ['title' => get_string('recheckfile', 'local_codechecker')] + )); $output .= html_writer::start_tag('ul'); foreach ($fileinxml->xpath('error|warning') as $problem) { @@ -204,12 +228,16 @@ public function problem_message($problem, $prettypath) { $code = ''; if ($lastfileandline !== $prettypath . '#@#' . $line) { // We have moved to another line, output it. - $code = html_writer::tag('li', html_writer::tag('div', - html_writer::tag('pre', '#' . $line . ': ' . str_replace( - array_keys($this->replaces), - array_values($this->replaces), - s(local_codechecker_get_line_of_code($line, $prettypath)) - ))), + $code = html_writer::tag( + 'li', + html_writer::tag( + 'div', + html_writer::tag('pre', '#' . $line . ': ' . str_replace( + array_keys($this->replaces), + array_values($this->replaces), + s(local_codechecker_get_line_of_code($line, $prettypath)) + )) + ), ['class' => 'sourcecode'] ); $lastfileandline = $prettypath . '#@#' . $line; diff --git a/run.php b/run.php index a244aa3c..fce3499e 100644 --- a/run.php +++ b/run.php @@ -36,11 +36,13 @@ } // Get the command-line options. -list($options, $unrecognized) = cli_get_params( +[$options, $unrecognized] = cli_get_params( ['help' => false, 'interactive' => false, 'exclude' => ''], ['h' => 'help', 'i' => 'interactive', - 'e' => 'exclude', ]); + 'e' => 'exclude', + ] +); if (count($unrecognized) != 1) { $options['help'] = true; diff --git a/settings.php b/settings.php index 4baa1353..7847f7d4 100644 --- a/settings.php +++ b/settings.php @@ -25,7 +25,9 @@ defined('MOODLE_INTERNAL') || die; if ($hassiteconfig) { // Needs this condition or there is error on login page. - $ADMIN->add('development', new admin_externalpage('local_codechecker', - get_string('pluginname', 'local_codechecker'), - new moodle_url('/local/codechecker/index.php'))); + $ADMIN->add('development', new admin_externalpage( + 'local_codechecker', + get_string('pluginname', 'local_codechecker'), + new moodle_url('/local/codechecker/index.php') + )); } diff --git a/tests/locallib_test.php b/tests/locallib_test.php index 92bd0704..7983eb5e 100644 --- a/tests/locallib_test.php +++ b/tests/locallib_test.php @@ -25,7 +25,6 @@ * @license https://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ final class locallib_test extends \basic_testcase { - /** * Data provider for test_local_codechecker_find_other_files() */ @@ -37,28 +36,28 @@ public static function local_codechecker_find_other_files_provider(): array { 'ignores' => [], 'extensions' => $defaultextensions, 'matches' => [\moodle_exception::class], - 'notmatches' => [], + 'nomatches' => [], ], 'one wrong dir' => [ 'path' => 'local/codechecker/nononotests/', 'ignores' => [], 'extensions' => $defaultextensions, 'matches' => [\moodle_exception::class], - 'notmatches' => [], + 'nomatches' => [], ], 'one file' => [ 'path' => 'local/codechecker/tests/locallib_test.php', 'ignores' => [], 'extensions' => $defaultextensions, 'matches' => [], - 'notmatches' => [], + 'nomatches' => [], ], 'one php file' => [ 'path' => 'local/codechecker/tests/locallib_test.php', 'ignores' => [], 'extensions' => ['php'], 'matches' => ['local/codechecker/tests/locallib_test.php'], - 'notmatches' => [], + 'nomatches' => [], ], 'one dir' => [ 'path' => 'local/codechecker/tests', @@ -110,8 +109,13 @@ public static function local_codechecker_find_other_files_provider(): array { * @dataProvider local_codechecker_find_other_files_provider * @covers ::local_codechecker_find_other_files */ - public function test_local_codechecker_find_other_files(string $path, array $ignores, - array $extensions, array $matches, array $nomatches): void { + public function test_local_codechecker_find_other_files( + string $path, + array $ignores, + array $extensions, + array $matches, + array $nomatches + ): void { global $CFG; require_once(__DIR__ . '/../locallib.php'); diff --git a/thirdpartylibs.xml b/thirdpartylibs.xml index 1f685c45..9b8ae20b 100644 --- a/thirdpartylibs.xml +++ b/thirdpartylibs.xml @@ -3,35 +3,28 @@ vendor/squizlabs/php_codesniffer PHP_CodeSniffer - 3.10.1 + 3.13.5 BSD 3-Clause - - vendor/phpcompatibility/php-compatibility - PHP Compatibility Coding Standard - 10.0dev (96072c30) - LGPL - 3 - vendor/phpcsstandards/phpcsextra A collection of code standards for use with PHP_CodeSniffer - 1.2.1 + 1.5.0 LGPL 3 vendor/phpcsstandards/phpcsutils A suite of utility functions for use with PHP_CodeSniffer - 1.0.12 + 1.2.1 LGPL 3 vendor/moodlehq/moodle-cs Moodle Coding Style - v3.4.7 + v3.7.0 GPL 3 diff --git a/version.php b/version.php index 3fec8cb1..4beb7ff9 100644 --- a/version.php +++ b/version.php @@ -24,8 +24,8 @@ defined('MOODLE_INTERNAL') || die; -$plugin->version = 2025120200; -$plugin->release = '5.1.0'; +$plugin->version = 2026051503; +$plugin->release = '5.2.3'; $plugin->maturity = MATURITY_STABLE; $plugin->requires = 2019111803; // Moodle 3.8.3 release and upwards (first one supporting PHP 7.4). $plugin->component = 'local_codechecker';