Skip to content

feat: guard Deployer singleton before Host construction#4189

Merged
antonmedv merged 3 commits intodeployphp:masterfrom
m1rm:feat/deployer-has-instance
Apr 12, 2026
Merged

feat: guard Deployer singleton before Host construction#4189
antonmedv merged 3 commits intodeployphp:masterfrom
m1rm:feat/deployer-has-instance

Conversation

@m1rm
Copy link
Copy Markdown
Contributor

@m1rm m1rm commented Apr 11, 2026

  • Bug fix #…?
  • New feature?
  • BC breaks? -> maybe, because the exception changed
  • Tests added?
  • Docs added? -> comments in the code

Background

Host::__construct used Deployer::get() to decide whether to inherit the global recipe config. With a typed, uninitialized static $instance, PHP 8+ throws when get() reads the property before new Deployer(...) has run. That can break HostTest (and any code path that builds a Host without a bootstrapped singleton).

Solution

Deployer

  • Store the singleton as ?self (default null). get() throws a clear RuntimeException if not initialized.
  • Add resetInstance() (@internal) for tests that need a clean singleton between cases.

Host

Always wire the parent Configuration from Deployer::get()->config. If Deployer is not initialized, get() throws — there is no “host without deployer” path anymore (as discussed in review).

Tests

  • Tests that construct Host bootstrap Deployer first (e.g. new Deployer(new Application())), and tearDown() still calls Deployer::resetInstance() where needed for isolation.
  • Where invoke() / logger paths run, the test Deployer also has input and output set on the container (same idea as other tests that touch the full stack).

Tooling

  • PHPStan baseline: remove the obsolete ignore that applied to the old Host conditional.

Behavior / compatibility

  • Normal runtime (recipe always creates Deployer before hosts): unchanged.
  • Calling Deployer::get() with no instance: invalid; error is a RuntimeException instead of a PHP engine error on an uninitialized typed static.

New

new Host(...) without a prior Deployer now always fails fast with that same initialization error instead of silently using a Configuration with no deployer parent.

@m1rm
Copy link
Copy Markdown
Contributor Author

m1rm commented Apr 11, 2026

Additional info/motivation of this PR: I came across the issue when implementing setDefaultSelector as thin typed wrapper for set('default_selector', ...) (which will be a subsequent PR).

$parent = null;
if (Deployer::get()) {
if (Deployer::hasInstance()) {
$parent = Deployer::get()->config;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What a use case where we create a Host instance before creating Deployer?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should throw an error here instead?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What a use case where we create a Host instance before creating Deployer?

I ran into an error when trying to implement a thin typed wrapper for set('default_selector', ...), the wip implementation is here:

master...m1rm:deployer:feat/add-setDefaultSelector-function

maybe I also took a wrong turn, I'd be fine with throwing an error if that is more appropriate

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so. Can't imagine why host without deployer may be needed.

Also please check phpstan errors.

m1rm added 2 commits April 12, 2026 18:56
…('Deployer is not initialized.') when the singleton is missing, so the constructor now always uses the deployer config as the parent and relies on that throw (no separate if (!Deployer::hasInstance()) block needed
@antonmedv antonmedv merged commit 2cb259a into deployphp:master Apr 12, 2026
8 of 9 checks passed
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