fix: only validate PHP when activating snippets in bulk - #491
Conversation
|
Worth adding, because it makes this bigger than the fixed list of PHP built-ins I gave above.
array_merge( $defined_functions['internal'], $defined_functions['user'] )So it is not just PHP internals. It is every function declared by WordPress, the active plugins and the theme. On a modest test install that is 5,658 names — 1,623 internal and 4,035 from user code. Measured against thirty names a script author might plausibly use, twenty collided:
Two consequences:
I have added a seventh test that derives its name from the live |
43a73be to
f7e6831
Compare
Bulk activation ran every selected snippet through the PHP code validator,
whatever its type:
foreach ( $snippets as $snippet ) {
$validator = new Validator( $snippet->code );
The other two callers guard this on the snippet being PHP; this one was
missed.
The validator does not check syntax. It looks for redeclarations of
functions and classes that already exist in PHP, which says nothing
meaningful about CSS or JavaScript. Read against a script, any function
sharing a name with a PHP built-in looks like a redeclaration, so these
were all refused:
next, reset, current, key, count, sort, end, compact, extract, header
Those are ordinary names in JavaScript. A carousel with next() and
reset() could not be activated in bulk, while the same snippet activated
perfectly well from its own toggle, because that path is guarded.
It also failed silently. Invalid snippets are dropped from the batch
rather than reported, so the snippet simply stayed inactive with nothing
said, and one such snippet returned null for the whole call.
Validate only PHP, matching the other two callers.
Reported on the support forum as JavaScript being rejected by strict
validation rules. The reporter's explanation was not right, but the
experience was: their scripts would not activate.
check_duplicate_identifier() builds its list from get_defined_functions(), so it covers PHP internals plus every function declared by WordPress, the active plugins and the theme. On a modest install that is over five and a half thousand names, four thousand of them from plugins. The set of JavaScript names that used to be refused was therefore specific to each site and grew as plugins were added, which is why the behaviour looked arbitrary and was hard to reproduce. Deriving the name from the live list rather than hard-coding one keeps the test honest whatever happens to be loaded.
f7e6831 to
27ce583
Compare
Prompted by Issue saving custom JS snippets due to strict validation/sanitization rules.
The reporter's explanation is not right — we do not validate JavaScript syntax, and there are no regex rules on declarations or DOM calls. I tested every pattern they named and all are fine:
el => { … }Array.from().filter()MutationObserverURLSearchParamsBut their experience was real, and there is a genuine bug underneath it.
The bug
activate_snippets()runs every selected snippet through the PHP validator, whatever its type:The other two callers guard this on
'php' === $snippet->type. This one was missed.The validator does not check syntax. It looks for redeclarations of functions and classes that already exist in PHP. Run against a script, any function sharing a name with a PHP built-in reads as a redeclaration. All of these are refused:
Those are ordinary names in JavaScript. A carousel with
next()andreset()cannot be bulk activated.And the same snippet activates fine on its own, because the single-activation path is guarded. That inconsistency is what makes it look like arbitrary validation rules from the outside.
It fails silently. Invalid snippets are dropped from the batch rather than reported, so the snippet just stays inactive with nothing said. Worse, one bad snippet makes the whole call return
null.The fix
Validate only PHP, matching the other two callers.
Testing
6 unit tests. Two fail without the fix; the other four are guards proving PHP checking is intact:
Also verified through the admin UI on WP 7.1: created a JavaScript snippet containing
next()andreset(), selected it, chose Activate, pressed Apply.Full suite 163 tests,
phpcsclean.Related
The silent failure is the same problem as #488 — a batch can fail entirely and say nothing. That draft would surface this class of thing; this fixes the cause of one instance of it.
Worth asking the reporter whether their scripts declare functions with any of the names above, to confirm this is what they hit rather than something still hiding.