Move the block to Block API version 3, and two fixes from the #88 review - #89
Merged
Conversation
Every run printed one "file_get_contents(): Read of N bytes failed with errno=21 Is a directory" notice per empty-of-JavaScript directory, in CI logs as well as locally. The filter callback returns true for a directory to mean "recurse into it", but RecursiveIteratorIterator defaults to LEAVES_ONLY, and a directory whose children the filter all rejected has no leaves -- so the directory itself is yielded as one. Skip directories in the loop. No behaviour change: those entries produced false from file_get_contents and were already skipped by the check below, just noisily.
PHPMD flagged handle_upload() at an NPath complexity of 1152 against a threshold of 500 (code scanning alert #36). The method is a flat chain of guards, but each one calls redirect_with_notice() rather than returning, and static analysis cannot know that ends the request -- so every guard counts as a branch that might fall through and the paths multiply. The four checks on the posted file move into accept_uploaded_archive(), which returns the validated paths or ends the request. handle_upload() is left with the capability check, the nonce, the install and the notice. $_FILES is still read in handle_upload() and passed in, rather than read in the callee. WordPress.Security.NonceVerification works per function scope, so reading the superglobal in a method that does not itself call check_admin_referer() trips the sniff -- and the honest fix is to keep the nonce check and the read together, not to suppress the warning. No behaviour change: the same checks run in the same order with the same messages. PHPMD reports no violation for the file now, PHPCS is clean, and the nine existing ExeLearning_Admin_Styles tests pass unchanged.
exelearning/elp-upload declared no apiVersion, so it registered as version 1, which WordPress 6.9 deprecates. That was not a local problem: a single API-version-1 block forces the whole post editor onto the non-iframe path, so the plugin was holding every editor it was installed in back from a change WordPress is completing. Three things stood in the way, and only the first is the one-line part. The version is now declared in both places -- apiVersion in the JS, api_version in register_block_type() -- with a test asserting they agree, since they sit far apart and disagreeing is silent. The visible output of both edit() branches is wrapped in one element carrying useBlockProps(), with the hook called before the early return; InspectorControls and BlockControls stay outside it, being Slot/Fill that render into the editor's own chrome. The stylesheets move onto the block type. Enqueued from enqueue_block_editor_assets they land in the outer admin document, which is not where an API version 3 block renders; declared as the block's style/editor_style, WordPress injects them into the canvas iframe. exelearning-frontend is still enqueued globally for the shortcode and deduplicates by handle. The fullscreen button is now wired by the component. elp-upload-fullscreen.js attached a click listener and a MutationObserver to the admin document and matched buttons to previews by DOM proximity -- none of which can see a block that lives in another document. The edit component already held a ref to the preview iframe, so the button gets a real onClick and a disabled state for a file with no preview. The script, its enqueue and its two test files are deleted: it existed only for the editor, and there is no version of it that can watch a document it is not in. The frontend button is unaffected; it is wired by inline JS emitted from PHP. The E2E test is what proves this. It failed first at `[data-type="exelearning/elp-upload"] .exelearning-block-preview iframe` resolving to 0 elements in the main document, which is exactly the migration working. It now looks inside iframe[name="editor-canvas"], toggles the real inspector control in the outer sidebar, and asserts the button appears in the canvas and takes the preview fullscreen. Its fullscreen stub moved from the element to the prototype: pinned to one node it vanished whenever the editor re-rendered, which made the assertion flaky in a full-suite run while passing in isolation. Designed in SDD-0005.
Contributor
Test in WordPress PlaygroundTest the plugin with the code from this branch:
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #89 +/- ##
============================================
- Coverage 96.99% 96.87% -0.12%
- Complexity 862 864 +2
============================================
Files 40 39 -1
Lines 4357 4323 -34
============================================
- Hits 4226 4188 -38
- Misses 131 135 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
The download and fullscreen buttons rendered as blank boxes in the editor while looking correct on the published page. Moving the block to API version 3 in the previous commit moved its rendering into the editor canvas iframe, which receives the block's declared styles and their dependencies and nothing else. wp-admin loads dashicons into the outer document, which used to be where the block was; it is not any more. The frontend was unaffected because dashicons is enqueued there separately. Declaring dashicons as a dependency of exelearning-frontend is enough: WordPress then carries the font wherever that sheet goes, canvas included. The failure mode here is quiet -- no error, no missing file, just icons that do not draw -- so the E2E asserts the computed font-family of an icon inside the canvas rather than that a stylesheet link exists. Confirmed to fail without the dependency: the font falls back to the system sans-serif, which is exactly the blank box.
The comment explaining why dashicons is a dependency shifted the line numbers that the POT and PO files record as source references. Only the `#:` comments change -- 55 lines in, 55 out, no msgid or msgstr touched.
erseco
added a commit
that referenced
this pull request
Aug 4, 2026
…-sandbox Brings in the coverage work (#88), the editor bootstrap refactor and the block's move to Block API version 3 (#89). Conflicts, and how they were taken: admin/views/editor-bootstrap.php -- both sides changed how the <base> tag is injected. This branch also injects a cache-purge script at the same point; main had fixed the pattern that finds the head element, because `<head[^>]*>` also matches the editor's own `<header id="head">`. Kept this branch's purge script with main's `\b` and the replace limit: without them the purge script was being injected, and run, twice. languages/* -- regenerated rather than hand-merged. main added no msgid, so this branch's translations carry over untouched and only the `#:` source references move. Tests that main added and this branch had already superseded: - vitest.config.mts: main disabled happy-dom iframe page loading for the whole suite, which breaks exe_embed.test.js -- that suite drives real iframes. Scoped the setting to wp_exe_download.test.js, the one file that needs it, with a @vitest-environment-options docblock. - elp_upload.test.js: the teacher-mode CSS-injection hack this branch replaced with `?exe-teacher=1` on the preview URL. Dropped; the replacement is already covered by elp_upload_preview.test.js. - exelearning_media_modal.test.js: the native-attachment-UI refactor removed the bespoke metadata panel, the "preview in new tab" link and the whole two-column actions row -- runAllUpdates() no longer calls addEditButtonToAttachmentInfo(). Those tests are gone. The details-panel ones are rewritten against what the panel does now: the preview replaces the thumbnail, and a single "Edit in eXeLearning" link sits below it carrying the class exelearning-editor.js binds to. - ContentProxyServeTest: one assertion compared the whole served document byte for byte, which no longer holds now that the proxy appends the embed shim. It asserts what it was about -- the absolute inline-style URL surviving untouched. 1007 PHP tests, 253 JS tests, PHPCS clean, translations deterministic.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #88. Three independent commits: the block migration, plus the two
smaller items that came out of that review.
1. Block API version 3
exelearning/elp-uploaddeclared noapiVersion, so it registered as version 1,which WordPress 6.9 deprecates. That was not a local problem: a single
API-version-1 block forces the whole post editor onto the non-iframe path, so
the plugin was holding every editor it was installed in back from a change
WordPress is completing.
No linter reported it. It surfaced only because #88 started registering the block
against the real
@wordpress/blockspackage instead of a stub.Only the first of three parts is the one-line change:
The version, declared twice.
apiVersion: 3in the JS andapi_version => 3in
register_block_type(), with a test asserting they agree — they sit far apartand disagreeing is silent.
The canvas wrapper. Both
edit()branches now wrap their visible output inone element carrying
useBlockProps(), with the hook called before the earlyreturn so it runs on every render.
InspectorControlsandBlockControlsstayoutside it: they are Slot/Fill and render into the editor's own chrome.
The stylesheets. Enqueued from
enqueue_block_editor_assetsthey land in theouter admin document, which is not where an API version 3 block renders. They are
now declared as the block type's
style/editor_style, which is whatWordPress injects into the canvas iframe.
exelearning-frontendis stillenqueued globally for the shortcode and deduplicates by handle.
The part that was a real regression
assets/js/elp-upload-fullscreen.jswired the editor's fullscreen button byattaching a
clicklistener and aMutationObserverto the admin documentand matching buttons to previews by DOM proximity. None of that can see a block
that lives in another document.
The
editcomponent already held a ref to the preview iframe, so the button nowgets a real
onClickand adisabledstate for a file with no preview. Thescript, its enqueue in
ExeLearning_Viewer_Enhancementsand its two test filesare deleted — it existed only for the editor, and there is no version of it that
can watch a document it is not in. The frontend button is untouched; it is
wired by inline JavaScript emitted from PHP.
How this was verified
The E2E suite is what proves the migration, and it earned its keep twice.
It failed first at
[data-type="exelearning/elp-upload"] .exelearning-block-preview iframeresolving to 0 elements in the main document — which is precisely the
migration working, and the evidence that the fullscreen script had died. The
test's own comment had recorded the old assumption: "Its presence in the main
document (not an iframed canvas) is what lets the editor script find and sync
the button."
It now looks inside
iframe[name="editor-canvas"], toggles the real inspectorcontrol in the outer sidebar, and asserts the button appears in the canvas
and takes the preview fullscreen.
Then it flaked: passing 3/3 in isolation, failing in a full-suite run. The stub
for
requestFullscreenwas pinned to the iframe element, and the editorre-renders the block freely, so the stub vanished with the node. It now goes on
HTMLIFrameElement.prototypeinside the canvas and records whether the iframesits in
.exelearning-block-preview, so the assertion is still about thepreview and not just "some iframe went fullscreen".
2. PHPMD NPathComplexity (code scanning alert #36)
handle_upload()measured 1152 against a threshold of 500. It is a flat chain ofguards, but each calls
redirect_with_notice()instead of returning, and staticanalysis cannot know that ends the request — so every guard counts as a branch
that might fall through and the paths multiply.
The four checks on the posted file move into
accept_uploaded_archive(). PHPMDreports no violation for the file now and the nine existing tests pass unchanged.
$_FILESis still read inhandle_upload()and passed in, rather than read inthe callee:
WordPress.Security.NonceVerificationworks per function scope, soreading the superglobal in a method that does not itself call
check_admin_referer()trips the sniff — and the honest fix is to keep the noncecheck and the read together, not to suppress the warning.
3. The translation validator's directory notice
Every run printed one
file_get_contents(): ... Is a directorynotice perJavaScript-free directory, in CI logs as well as locally. The filter callback
returns
truefor a directory to mean "recurse into it", butRecursiveIteratorIteratordefaults toLEAVES_ONLY, and a directory whosechildren the filter all rejected has no leaves — so the directory itself is
yielded as one.
Documentation
SDD-0005, indexed.
docs/SHORTCODES.mdneeds no change: the block's saved attributes and itsserver-side render are untouched, so this is presentation-only for existing
posts.
Verification
make test— 942 tests, 2073 assertionsnpm run test:js— 242 testsmake test-e2e— 38 tests, chromium + firefox (was 40; the two deleted are theisolation tests for the deleted script)
make check-translations— regenerated; the diff is#:reference commentsonly, no msgid or msgstr touched
phpcsandphpmd— cleanWorth a reviewer's eye
The preview iframe is now an iframe inside the canvas iframe. Same-origin
still holds — both are this site — which is what the teacher-mode stylesheet
injection depends on, and the E2E exercises it end to end. It is the part of this
change most likely to behave differently in an environment I have not tried.