fix: play a plain SCORM package, and keep the source where the editor needs it

[erseco]

Refs exelearning/exelearning#2415 · pairs with exelearning/exelearning#2417

Two separate things, because content.xml was being asked to do two jobs at once.

1. A SCORM package without content.xml should just play

exescorm/mandatoryfileslist defaulted to /^content(v\d+)?\.xml$/, so any package without an eXeLearning source was rejected on upload with badexelearningpackage — a stock Articulate/iSpring SCORM, and now eXeLearning's own output once it honours the Editable export property.

That rule was gatekeeping, not validating:

• nothing in this plugin reads content.xml at runtime — there is not one reference to it in PHP outside this check;
• exescorm_validate_package() already enforces SCORM validity independently, requiring a root imsmanifest.xml or an AICC .cst (lib.php:1382), with its own nomanifest / badimsmanifestlocation messages;
• an uploaded package is never editable anyway — view.php:181 offers "Edit in eXeLearning" only for EXESCORM_TYPE_EMBEDDED or an .elpx.

So the default is now empty. Such a package uploads, plays, tracks, and simply has no edit button. The setting stays for sites that want eXeLearning-authored packages only — they re-add the expression. db/upgrade.php clears the stored value only when it is still the old default byte for byte, so a customised list is left alone.

forbiddenfileslist (/.*\.php$/) is untouched — that one is security, not policy.

2. Require the source where it actually matters

Two endpoints store a package that will be re-opened for editing, and both now refuse one without an eXeLearning source:

• editor/save.php is where the embedded editor saves (editor/index.php hands it to bridge.js as saveUrl), and bridge.js re-opens that exact package on the next edit. It used to write the upload straight into the package filearea with no validation at all, and mod_form.php deliberately skips validation for the embedded type, so this was the only gate on that path and it was open. It now stages the upload in temppackage (the same sequence set_ode.php uses), runs exescorm_validate_package() and has_editable_source() on it, and on failure deletes the staged file and answers HTTP 400 {success: false, error: <reason>}. Nothing is written to package, reference is untouched and exescorm_parse() is not called, so the activity keeps the package it had.
• set_ode.php is the callback of the external eXeOnline flow (exescorm_redirector sends the author to exeonlinebaseuri, and get_ode.php hands the stored package back to eXeLearning on the next edit). The same check applies there; that flow was already rejected by the old mandatoryfileslist default, only the message changes.

A source-less package in either place is unrecoverable, so it is refused explicitly instead of inherited from a global RE list that also governs unrelated uploads.

editor/index.php gets the same check. The view page never offers the edit button for an uploaded package, but the URL was reachable with only a capability check, and the editor would have loaded and then failed at import with no explanation. It now shows a reason.

And the export request sets options: {forceEditableSource: true} (exelearning/exelearning#2417), so an author who turns the property off can still use the embedded editor: the package it stores keeps its source, while the property still governs every export the author triggers themself.

Drive-by

tests/validatepackage_test.php asked for validexescorm.zip and badexescorm.zip. Neither has ever existed in this repository — the fixtures are validscorm.zip and badscorm.zip — so test_validate_package errored in create_file_from_pathname before asserting anything. Fixed, since the new tests live in that file.

Verification

Moodle 4.5.10 / PHP 8.3.15 / MariaDB 12.3.3, docker compose up + vendor/bin/phpunit.

• mod/exescorm/tests/validatepackage_test.php — OK (5 tests, 21 assertions). On the unmodified tree the same file is Tests: 1, Errors: 1 (the missing fixture).
• Full mod_exescorm_testsuite — before: Tests: 203, Errors: 45, Failures: 8; after: Tests: 207, Errors: 44, Failures: 8. The 4 extra tests are the new ones and the error that disappeared is the fixture name. No new failures; the remaining ones are pre-existing PHP 8.3 dynamic-property deprecations.
• phpcs --standard=moodle-extra on every changed file: error counts identical to the unmodified tree, except settings.php which drops one (a tab-indented line removed). editor/index.php, editor/save.php and lang/en/exescorm.php are clean.

New tests: a plain SCORM package validates; a site that re-adds the mandatory rule still rejects it; has_editable_source() across root/nested/legacy contentvN.xml/near-miss names; and a real archive with imsmanifest.xml + content.xml passes both save-time checks.

Ordering

Safe to merge in either order. The bundled editor under dist/ is built from a pinned eXeLearning checkout (make build-editor); until that checkout includes exelearning/exelearning#2417, forceEditableSource is an unknown key the editor ignores and the current editor still always writes content.xml. Part 1 of this PR stands on its own regardless.

Longer term

The cleaner shape is still to keep the .elpx source in its own file area beside the published SCORM package, so the author's exportSource choice would apply to what learners receive while the source stays private to the plugin. That needs a new file area plus backup, restore and upgrade paths — follow-up, not a precondition. Recorded in ADR-2415-01 on the core PR.

---

Moodle Playground Preview

The changes in this pull request can be previewed and tested using a Moodle Playground instance.

ℹ️ The eXeLearning editor is fetched from the shared release and unpacked into the plugin when the playground boots, so the first load may take a few extra seconds. ELPX upload, viewer and preview work normally.

[ignaciogros]

Thanks for this, @erseco.

The diagnosis on part 1 is right, and the fixture fix in validatepackage_test.php was overdue. Two things before merging:

Blocking: the save-time check is in the wrong endpoint

The PR says "set_ode.php is the embedded editor saving, and editor/bridge.js re-opens that exact package on the next edit". I think that is not the case:

• set_ode.php is the callback for the external eXeOnline flow (exeonlinebaseuri, see classes/exeonline/exescorm_redirector.php:122).
• The embedded editor saves through editor/save.php (editor/index.php:88, lib.php:316). That file calls neither exescorm_validate_package() nor has_editable_source(): it writes the ZIP straight into the package filearea, updates reference and calls exescorm_parse().

So once the bundled editor picks up exelearning/exelearning#2417, if forceEditableSource is ever not honoured, editor/save.php will happily store a source-less package and the activity ends up in exactly the unrecoverable state this PR wants to prevent. The safety net covers the old flow, not the one bridge.js uses.

Suggested fix: add the same check to editor/save.php — create the stored_file, list_files() + has_editable_source(), and fail (success: false, nothing written to reference, no exescorm_parse()) when the source is missing. Please also fix the comment in set_ode.php and the PR description. Keeping the check in set_ode.php is fine: that flow was already rejected by mandatoryfileslist before, only the message changes.

Minor (non-blocking)

editor/save.php does not call exescorm_validate_package() at all. Pre-existing, but since you will be touching that file anyway, it is the natural place to run both checks in the same pass.

Could you please take a look at those points?

Thank you.

[erseco]

Addressed in c98a2a1.

Save-time check. Confirmed: the embedded editor saves through editor/save.php (editor/index.php:106 → bridge.js:249), and set_ode.php is the eXeOnline callback. I had the two flows crossed. editor/save.php now:

• stages the upload in temppackage, the sequence set_ode.php already uses, so the current package is untouched while the new one is checked;
• runs exescorm_validate_package() and then has_editable_source() on the staged file;
• on failure deletes the staged file and answers HTTP 400 {"success": false, "error": <reason>} — nothing written to package, reference untouched, no exescorm_parse(); the activity keeps its previous package;
• only then wipes package, moves the staged file in, updates reference and parses, as before.

The set_ode.php check stays; its comment now describes the eXeOnline flow it actually serves, and the bridge.js comment no longer claims exescorm_validate_package() would catch a source-less export (with the default rule gone it would not — the new check is what does).

Minor. Covered by the same pass: exescorm_validate_package() runs first, so a missing manifest, a manifest in a subfolder or a forbidden file gets the existing nomanifest / badimsmanifestlocation / badexelearningpackage message. The missing-source case gets a new string, nosourcetosave.

Tests. test_has_editable_source docblock corrected, and test_editor_export_with_source_passes_save_checks builds a real ZIP (imsmanifest.xml + content.xml + index.html) and asserts it passes both checks, so the root-only match is exercised against actual list_files() pathnames rather than only synthetic entries.

Verification (Moodle 4.5.10 / PHP 8.3.15 / MariaDB 12.3.3):

• tests/validatepackage_test.php: OK (5 tests, 21 assertions).
• Full mod_exescorm_testsuite: Tests: 207, Errors: 44, Failures: 8 — the previous 206/44/8 plus the new test; no new failures.
• phpcs --standard=moodle-extra (moodle-cs 3.7.0): editor/save.php and lang/en/exescorm.php clean; set_ode.php and tests/validatepackage_test.php report exactly the same counts as on main, all on pre-existing lines.

PR description updated to match.

One thing I noticed and left alone: amd/src/editor_modal.js handles save-error by logging to the console and re-enabling the Save button, so a rejected save is silent for the author. Pre-existing, and it applies to every save failure, not just this one — I'd rather open a separate issue than grow this PR.

Addressed in c98a2a1 (editor/save.php now runs both checks); re-requesting review.

[ignaciogros]

Thanks, @erseco.

I translated the source-check strings and added the change in the CHANGELOG.

I hope those changes are OK too.

I agree about opening a separate issue for the amd/src/editor_modal.js problem.