Respect Warning settings before calling custom handler - #1393
Conversation
…dler # Conflicts: # CHANGELOG.md # tests/Executor/ExecutorLazySchemaTest.php
…dler # Conflicts: # CHANGELOG.md # src/Error/Warning.php
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.50.
| Benchmark suite | Current: e920ef9 | Previous: 203d11e | Ratio |
|---|---|---|---|
DeferredBench::benchManyNestedDeferreds |
21.952 ms |
12.472 ms |
1.76 |
This comment was automatically generated by workflow using github-action-benchmark.
There was a problem hiding this comment.
Pull request overview
This PR updates the library’s warning emission logic so custom warning handlers respect Warning::enable() / Warning::suppress() settings, matching the default trigger_error behavior.
Changes:
- Gate custom warning handler invocation behind the same
enableWarningsbitmask check used fortrigger_error. - Refactor warning emission to centralize “should warn” logic and streamline
warnOnce()behavior. - Update the lazy schema executor test to assert warning handling behavior (currently needs fixes to actually execute).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
tests/Executor/ExecutorLazySchemaTest.php |
Adds assertions for warning interception via a custom warning handler during lazy schema execution. |
src/Error/Warning.php |
Ensures warnings are only emitted/handled when enabled; refactors warnOnce()/warn() and introduces shouldWarn(). |
CHANGELOG.md |
Documents the behavior change for custom warning handlers respecting suppression. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public static function suppress($suppress = true): void | ||
| { | ||
| if ($suppress === true) { | ||
| self::$enableWarnings = 0; | ||
| self::$enableWarnings = self::NONE; | ||
| } elseif ($suppress === false) { |
| { | ||
| if ($enable === true) { | ||
| self::$enableWarnings = self::ALL; | ||
| } elseif ($enable === false) { | ||
| self::$enableWarnings = 0; | ||
| self::$enableWarnings = self::NONE; |
| $warnings = []; | ||
| Warning::setWarningHandler(function ($warning) use (&$warnings): void { | ||
| $warnings[] = $warning; | ||
| }); | ||
| Warning::enable(Warning::WARNING_FULL_SCHEMA_SCAN); |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
tests/Executor/ExecutorLazySchemaTest.php:140
markTestIncomplete()aborts the test, so the new assertions and$warningsverification below it are unreachable. It also prevents resetting the global warning handler, which can leak into other tests.
Warning::enable(Warning::WARNING_FULL_SCHEMA_SCAN);
$result = Executor::execute($schema, Parser::parse($query));
self::markTestIncomplete('No longer works with PHPUnit 10, reintroduce with https://github.com/webonyx/graphql-php/pull/1393');
self::assertEquals($expected, $result);
src/Error/Warning.php:62
- The
suppress()docblock says it has no effect when a custom warning handler is set, but this PR changes behavior to respect suppression/enablement before invoking the custom handler. The doc comment should be updated to match.
public static function suppress($suppress = true): void
{
if ($suppress === true) {
self::$enableWarnings = self::NONE;
src/Error/Warning.php:90
- The
enable()docblock says it has no effect when a custom warning handler is set, but this PR makes enablement affect custom handlers too. Update the doc comment so it matches the new behavior.
public static function enable($enable = true): void
{
if ($enable === true) {
self::$enableWarnings = self::ALL;
} elseif ($enable === false) {
self::$enableWarnings = self::NONE;
…dler # Conflicts: # CHANGELOG.md
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
tests/Executor/ExecutorLazySchemaTest.php:138
markTestIncomplete()makes the new assertions unreachable and also leaves the staticWarninghandler enabled for the rest of the test suite. This can cause cascading side effects in subsequent tests. Remove the incomplete marker and ensure warning settings/handler are restored in afinallyblock (also align the handler signature withWarningHandler).
$warnings[] = $warning;
});
Warning::enable(Warning::WARNING_FULL_SCHEMA_SCAN);
$result = Executor::execute($schema, Parser::parse($query));
self::markTestIncomplete('No longer works with PHPUnit 10, reintroduce with https://github.com/webonyx/graphql-php/pull/1393');
src/Error/Warning.php:62
- The docblock says suppressing warnings has “no effect when custom warning handler is set”, but
warn()now checksshouldWarn()before calling the handler. Please update the docblock to match the new behavior.
public static function suppress($suppress = true): void
{
if ($suppress === true) {
self::$enableWarnings = self::NONE;
src/Error/Warning.php:90
- The docblock says re-enabling warnings has “no effect when custom warning handler is set”, but
warn()now checksshouldWarn()before calling the handler. Please update the docblock to match the new behavior.
public static function enable($enable = true): void
{
if ($enable === true) {
self::$enableWarnings = self::ALL;
} elseif ($enable === false) {
self::$enableWarnings = self::NONE;
…dler # Conflicts: # CHANGELOG.md
…dler # Conflicts: # CHANGELOG.md
Respect settings in GraphQL\Error\Warning before calling custom warningHandler.