Skip to content

Detect REQUIRED_ARG_ADDED when the old field had no prior arguments - #1976

Open
yaroslavGoncharuk wants to merge 2 commits into
webonyx:masterfrom
yaroslavGoncharuk:fix/required-arg-added-with-no-prior-args
Open

yaroslavGoncharuk wants to merge 2 commits into
webonyx:masterfrom
yaroslavGoncharuk:fix/required-arg-added-with-no-prior-args

Conversation

@yaroslavGoncharuk

Copy link
Copy Markdown

Problem

BreakingChangesFinder::findArgChanges() fails to report REQUIRED_ARG_ADDED when a
new required argument is added to a field that had zero arguments in the old
schema. The same kind of change is detected correctly the moment the field already
had at least one argument.

Root cause

The "check if arg was added to the field" loop was nested inside the loop over the
old field's existing args:

foreach ($oldField->args as $oldArgDef) {
    // ... existing-arg comparisons (ARG_REMOVED / ARG_CHANGED_KIND) ...

    // Check if arg was added to the field
    foreach ($newTypeFields[$fieldName]->args as $newTypeFieldArgDef) {
        // ...
    }
}

Since the "was an arg added" check only ran as a side effect of iterating
$oldField->args, an empty $oldField->args skipped it entirely — regardless of
how many args (required or not) the new field had.

// Reproduces on master:
$oldType = new ObjectType([
    'name' => 'Type1',
    'fields' => ['field1' => ['type' => Type::string(), 'args' => []]],
]);
$newType = new ObjectType([
    'name' => 'Type1',
    'fields' => ['field1' => [
        'type' => Type::string(),
        'args' => ['newRequiredArg' => Type::nonNull(Type::string())],
    ]],
]);
// findArgChanges($oldSchema, $newSchema)['breakingChanges'] === [] (should not be empty)

Fix

Moved the "was an arg added" loop out from under foreach ($oldField->args as $oldArgDef) so it runs once per field regardless of whether the old field had any
args — pure reordering, no behavior change for the already-working (non-empty
old-args) case. Mirrors how findFieldsThatChangedTypeOnInputObjectTypes() already
treats its own "was a field added" check as an independent loop.

Tests

Added testShouldDetectIfANonNullFieldArgumentWasAddedToAFieldWithNoPriorArgs()
alongside the existing testShouldDetectIfANonNullFieldArgumentWasAdded(), covering
the old-field-has-zero-args case.

Verification

composer test (2015 tests) and phpstan (full analysis) both pass; php-cs-fixer
reports 0 issues on the changed files. (composer check's rector step currently
errors on a clean, unmodified master too — an unrelated pre-existing tooling issue,
not run by CI.)

No public API changes; this only makes an existing detection function more complete.

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