Skip to content

Add getter to check if registry is enabled#54

Merged
JulianVennen merged 3 commits intomasterfrom
is-registry-enabled
Mar 27, 2026
Merged

Add getter to check if registry is enabled#54
JulianVennen merged 3 commits intomasterfrom
is-registry-enabled

Conversation

@JulianVennen
Copy link
Copy Markdown
Member

This could be useful to ensure you return the registry to its previous state after temporarily disabling it or to verify that your code doesn't accidentally leave the registry disabled in a test.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds a public getter for the GenericModel registry toggle so callers (especially tests) can check/restore the previous registry state after temporarily disabling it.

Changes:

  • Introduces GenericModel::isRegistryEnabled() to expose the current value of the internal registry flag.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@JulianVennen JulianVennen requested a review from Copilot March 27, 2026 22:25
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +12 to +16
$this->assertTrue(GenericModel::isRegistryEnabled());
GenericModel::disableRegistry();
$this->assertFalse(GenericModel::isRegistryEnabled());
GenericModel::enableRegistry();
$this->assertTrue(GenericModel::isRegistryEnabled());
Copy link

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

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

This test assumes the registry is enabled at the start and also risks leaving the global static registry state modified if an assertion fails after disableRegistry(). Since other tests in the suite call disableRegistry() without re-enabling it (e.g., TestDriverTest::testUpdate()), this can make the test order-dependent/flaky. Consider capturing the initial state with GenericModel::isRegistryEnabled() and restoring it in a try/finally (or tearDown()), and avoid asserting a hard-coded initial true unless you explicitly set it first.

Suggested change
$this->assertTrue(GenericModel::isRegistryEnabled());
GenericModel::disableRegistry();
$this->assertFalse(GenericModel::isRegistryEnabled());
GenericModel::enableRegistry();
$this->assertTrue(GenericModel::isRegistryEnabled());
$initialState = GenericModel::isRegistryEnabled();
try {
// Ensure registry is enabled so we can test disabling and re-enabling it
if (!$initialState) {
GenericModel::enableRegistry();
}
$this->assertTrue(GenericModel::isRegistryEnabled());
GenericModel::disableRegistry();
$this->assertFalse(GenericModel::isRegistryEnabled());
GenericModel::enableRegistry();
$this->assertTrue(GenericModel::isRegistryEnabled());
} finally {
// Restore original registry state to avoid leaking global changes
if ($initialState !== GenericModel::isRegistryEnabled()) {
if ($initialState) {
GenericModel::enableRegistry();
} else {
GenericModel::disableRegistry();
}
}
}

Copilot uses AI. Check for mistakes.
@JulianVennen JulianVennen merged commit c2b6db0 into master Mar 27, 2026
7 checks passed
@JulianVennen JulianVennen deleted the is-registry-enabled branch March 27, 2026 22:29
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.

3 participants