Skip to content

fix: catch duplicate declarations within a batch of snippets - #492

Merged
imantsk merged 1 commit into
fix/bulk-activate-validates-php-only/corefrom
fix/batch-duplicate-identifiers/core
Aug 31, 2026
Merged

fix: catch duplicate declarations within a batch of snippets#492
imantsk merged 1 commit into
fix/bulk-activate-validates-php-only/corefrom
fix/batch-duplicate-identifiers/core

Conversation

@TallblokeUK

Copy link
Copy Markdown
Contributor

Stacked on #491. That branch is the base, so review it first — this diff shows only the batch change once #491 lands.

Addresses part of #230, and it is worse than that issue describes: this does not merely allow duplicate names, it takes the site down and locks you out of the admin.

The bug

Select two snippets that declare the same function, choose Activate, press Apply. Both activate. The next request fatals.

Each snippet is validated on its own, against whatever PHP has declared at that moment. That set does not include the other snippets in the same batch, so neither sees the other and both pass.

Fatal error: Cannot redeclare function cs_duplicate_helper()
… in snippet-ops.php(778) : eval()'d code on line 1

Why this is not a warning-level problem

The redeclaration happens at runtime inside eval(), where it is a fatal error, not a catchable one. execute_snippet() wraps execution in try/catch ( Throwable ) and the catch never fires — I checked.

Measured on WP 7.1:

before after
Front page 500 200
wp-admin 500 302
Snippets screen 500 reachable

So you cannot get to the snippets screen to switch one off. And ?snippets-safe-mode=1, the documented escape hatch, is itself broken until #484 lands — the two compound.

The fix

Carry the identifiers claimed by accepted snippets through the batch. The second snippet to claim a name is held back rather than activated alongside the first.

Deliberately conservative:

  • Guarded declarations still activate. A function_exists() check is already recorded as an exception and cannot fatal.
  • Anonymous functions claim nothing, so add_filter( …, function () {} ) is unaffected.
  • Scripts and stylesheets are not considered, sharing no namespace with PHP.
  • No code is rewritten. Nothing is renamed or namespaced; a snippet is simply not activated.

Testing

6 unit tests. Two fail without the fix, four are guards:

test without fix
Two snippets declaring the same function fails
Two snippets declaring the same class fails
Different names both activate passes (guard)
Guarded declarations both activate passes (guard)
A script is not blocked by a PHP name passes (guard)
Anonymous functions do not collide passes (guard)

Verified end to end as above. Full suite 170 tests, phpcs clean.

What this does not solve

The protection remains point in time. A snippet that is safe today still fatals if a plugin installed next month declares the same name, and nothing re-checks. On a modest install there are already over 5,600 declared function names, most of them from plugins, so that is a live risk rather than a theoretical one.

Options discussed, none free:

  • Re-validate on plugin activation and hold back snippets that would now fatal. Closes the gap, but silently disabling someone's code needs care.
  • Encourage function_exists() guards, which the validator already honours. Converts a fatal into a snippet that quietly does not redeclare.
  • Prefix or namespace declarations automatically. Attractive, but rewriting names cannot be done safely: a function name appearing in a string is indistinguishable from data, so add_action( 'init', 'my_func' ) and $label = 'my_func'; cannot be told apart. I tested this and it silently corrupts working snippets, which is worse than the loud failure it replaces.

Worth deciding on #230 rather than here.

@imantsk
imantsk changed the base branch from core to fix/bulk-activate-validates-php-only/core August 31, 2026 17:21
@imantsk
imantsk force-pushed the fix/bulk-activate-validates-php-only/core branch from 43a73be to f7e6831 Compare August 31, 2026 17:37
@imantsk
imantsk force-pushed the fix/batch-duplicate-identifiers/core branch from c8cc985 to 11a86a0 Compare August 31, 2026 17:37
Two snippets declaring the same function could both be activated in one
action, and the site fataled on the very next request.

Each snippet was validated on its own, against whatever PHP had declared
at that moment. That set does not include the other snippets in the same
batch, so neither saw the other and both passed. The redeclaration then
happened at runtime, inside eval(), where it is a fatal error rather than
a catchable one: the front end and the admin both returned 500, leaving
no way to reach the snippets screen and switch one off.

Carry the identifiers claimed by accepted snippets through the batch, so
the second snippet to claim a name is held back rather than activated
alongside the first.

Guarded declarations are unaffected, since a function_exists() check is
already recorded as an exception and cannot fatal. Anonymous functions
claim no name. Scripts and stylesheets are not considered at all, as they
share no namespace with PHP.

Relates to #230.
@imantsk
imantsk force-pushed the fix/bulk-activate-validates-php-only/core branch from f7e6831 to 27ce583 Compare August 31, 2026 17:44
@imantsk
imantsk force-pushed the fix/batch-duplicate-identifiers/core branch from 11a86a0 to 538e4dd Compare August 31, 2026 17:44
@imantsk
imantsk merged commit 391f619 into fix/bulk-activate-validates-php-only/core Aug 31, 2026
13 of 14 checks passed
@imantsk
imantsk deleted the fix/batch-duplicate-identifiers/core branch August 31, 2026 17:46
@imantsk imantsk mentioned this pull request Aug 31, 2026
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants