Only parse APP_URL for default stateful domains when it's set
#279
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
If APP_URL is missing then null is passed to parse_url(), which breaks on PHP 8.1 even when you're only running composer install. On PHP <==8 it wouldn't error, but you'd have an empty entry in sanctum.stateful. Setting SANCTUM_STATEFUL_DOMAINS in the project's .env isn't enough because the default config is still merged in the service provider as long as config isn't cached (which it won't be during composer install). --- Also removes APP_URL from phpunit.xml, setting in the test that needs it instead. By using getEnvironmentSetUp instead of @environment-setup, we don't need putenv everywhere and it more closely matches other tests.
dshoreman
commented
May 19, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
If
APP_URLis not defined in the .env, the config breaks on PHP 8.1 with a null string deprecation. Since it has to load in the service provider, artisan and composer are broken too (not sure about new installs though—I only have 8.1 via GH actions).For older versions, it's less of an issue - it merely adds
''to thesanctum.statefularray. Looks scary from a distance, but there's anarray_filter()in the middleware so it's harmless... I think.Introduced in v2.9.3 via #264.
Notes
A simple
env('APP_URL', '')would suffice for the 8.1 issue, but it still leaves the extra empty entry insanctum.stateful. To remove any chance of worry/risk/ambiguity, this PR makes the whole thing conditional onAPP_URLbeing defined.