Skip to content

Throw on helper name collisions - #94

Merged
harikt merged 1 commit into
6.xfrom
helper-collision
Jul 23, 2026
Merged

Throw on helper name collisions#94
harikt merged 1 commit into
6.xfrom
helper-collision

Conversation

@harikt

@harikt harikt commented Jul 23, 2026

Copy link
Copy Markdown
Member

HelperRegistry is a flat name => callable map and set() overwrote silently, so two packages both registering 'url' meant last-one-wins with no warning and no way to tell it had happened.

set() now takes a third parameter, $override, and throws Exception\HelperAlreadyRegistered when the name is taken and override is false. Passing override: true on an unregistered name is deliberately not an error: it means "I accept replacing whatever is there", not "something must be there", so an application can assert its own helper without first probing for a module's.

Registering the identical callable twice also throws. Two packages that share an implementation still have to say which one owns the name.

The constructor map now routes through set() so there is one way in.

Summary by CodeRabbit

  • Breaking Changes
    • Registering a helper under an existing name now raises an error instead of silently replacing it.
    • Use the override: true option to intentionally replace an existing helper.
  • New Features
    • Added a dedicated error for duplicate helper registrations.
  • Documentation
    • Updated helper and interface documentation with collision handling, override usage, and conditional registration guidance.

HelperRegistry is a flat name => callable map and set() overwrote silently, so
two packages both registering 'url' meant last-one-wins with no warning and no
way to tell it had happened.

set() now takes a third parameter, $override, and throws
Exception\HelperAlreadyRegistered when the name is taken and override is false.
Passing override: true on an unregistered name is deliberately not an error: it
means "I accept replacing whatever is there", not "something must be there", so
an application can assert its own helper without first probing for a module's.

Registering the identical callable twice also throws. Two packages that share
an implementation still have to say which one owns the name.

The constructor map now routes through set() so there is one way in.
@coderabbitai

coderabbitai Bot commented Jul 23, 2026

Copy link
Copy Markdown

Review Change Stack

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 9c66b33b-1f3d-4c0e-a760-9180473b367e

📥 Commits

Reviewing files that changed from the base of the PR and between 5814b7a and a104b97.

📒 Files selected for processing (7)
  • CHANGELOG.md
  • docs/helpers.md
  • docs/interfaces.md
  • src/Exception/HelperAlreadyRegistered.php
  • src/HelperRegistry.php
  • src/HelperRegistryInterface.php
  • tests/HelperRegistryTest.php

Walkthrough

Helper registration now rejects duplicate names by default, exposes an override parameter for deliberate replacement, adds a dedicated exception, routes constructor entries through set(), and updates tests, documentation, and changelog entries.

Changes

Helper registration behavior

Layer / File(s) Summary
Registration contract and exception
src/Exception/HelperAlreadyRegistered.php, src/HelperRegistryInterface.php
Adds the duplicate-registration exception and extends set() with an optional override flag.
Registry collision and override behavior
src/HelperRegistry.php, tests/HelperRegistryTest.php
Constructor registration uses set(). Duplicate names throw unless overridden, with tests covering replacement, preservation, and collisions.
Documentation and release notes
docs/helpers.md, docs/interfaces.md, CHANGELOG.md
Documents duplicate-name errors, explicit overrides, conditional registration, and the new public exception.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Caller
  participant HelperRegistry
  participant HelperAlreadyRegistered
  Caller->>HelperRegistry: set(name, callable, override)
  alt duplicate name without override
    HelperRegistry->>HelperAlreadyRegistered: create exception
    HelperAlreadyRegistered-->>Caller: throw exception
  else new name or override enabled
    HelperRegistry-->>Caller: register callable
  end
Loading

Poem

A bunny taps set() with care,
No duplicate helpers hiding there.
“Override!” goes the carrot sign,
Or keep the first one safely fine.
New exceptions hop into the line!

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch helper-collision

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@harikt
harikt merged commit 767da4a into 6.x Jul 23, 2026
10 checks passed
@harikt
harikt deleted the helper-collision branch July 23, 2026 14:19
@harikt
harikt restored the helper-collision branch July 23, 2026 14:51
@harikt

harikt commented Jul 23, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review .

@coderabbitai

coderabbitai Bot commented Jul 23, 2026

Copy link
Copy Markdown

@harikt: I’ll review the changes in #94.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@harikt

harikt commented Jul 23, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai fullreview

@coderabbitai

coderabbitai Bot commented Jul 23, 2026

Copy link
Copy Markdown

@harikt: Starting a full review of #94.

✅ Action performed

Full review finished.

@harikt
harikt deleted the helper-collision branch July 23, 2026 16:39
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.

1 participant