Skip to content

fix(sdk): omit defaulted screen_hint from auth URLs#396

Merged
gjtorikian merged 1 commit into
mainfrom
restore-choice
May 28, 2026
Merged

fix(sdk): omit defaulted screen_hint from auth URLs#396
gjtorikian merged 1 commit into
mainfrom
restore-choice

Conversation

@gjtorikian
Copy link
Copy Markdown
Contributor

Closes #395

@gjtorikian gjtorikian requested review from a team as code owners May 28, 2026 14:59
@gjtorikian gjtorikian requested a review from nicknisi May 28, 2026 14:59
@gjtorikian gjtorikian merged commit d583c7c into main May 28, 2026
8 of 9 checks passed
@gjtorikian gjtorikian deleted the restore-choice branch May 28, 2026 15:00
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 28, 2026

Greptile Summary

Fixes getAuthorizationUrl so screen_hint is only included in the generated URL when the caller explicitly provides it, rather than always defaulting to sign-in.

  • $screenHint is now nullable with a null default; the null-safe ?->value access lets the existing array_filter strip the key cleanly.
  • A new runtime behaviour test (testAuthorizationUrlOmitsScreenHintUnlessProvided) asserts the query string is free of screen_hint when no value is passed.

Confidence Score: 5/5

Safe to merge — the change is minimal, correctly scoped, and directly tested.

The fix is a one-liner in the method signature and a single null-safe operator addition; the existing array_filter already handles the omission correctly, and the new test confirms the end-to-end behaviour. No other code paths are affected.

No files require special attention.

Important Files Changed

Filename Overview
lib/Service/UserManagement.php Makes $screenHint nullable (default null) and guards ->value access with the null-safe operator; array_filter already strips null entries so the field is correctly omitted when absent.
tests/Service/RuntimeBehaviorTest.php Adds testAuthorizationUrlOmitsScreenHintUnlessProvided which asserts screen_hint is absent from the generated URL when the parameter is not supplied.

Sequence Diagram

sequenceDiagram
    participant Caller
    participant getAuthorizationUrl
    participant array_filter
    participant buildUrl

    Caller->>getAuthorizationUrl: "call (screenHint = null, default)"
    getAuthorizationUrl->>array_filter: "query map with screen_hint => null"
    array_filter-->>getAuthorizationUrl: screen_hint key removed
    getAuthorizationUrl->>buildUrl: query without screen_hint
    buildUrl-->>Caller: URL (no screen_hint param)

    Caller->>getAuthorizationUrl: "call (screenHint = SignIn explicitly)"
    getAuthorizationUrl->>array_filter: "query map with screen_hint => sign-in"
    array_filter-->>getAuthorizationUrl: screen_hint retained
    getAuthorizationUrl->>buildUrl: "query with screen_hint=sign-in"
    buildUrl-->>Caller: "URL (screen_hint=sign-in present)"
Loading

Reviews (1): Last reviewed commit: "fix(sdk): omit defaulted screen_hint fro..." | Re-trigger Greptile

Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration Bot left a comment

Choose a reason for hiding this comment

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

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 2 additional findings.

Open in Devin Review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

UserManagement::getAuthorizationUrl() hardcodes screen_hint=sign-in, breaks all OAuth provider flows

1 participant