Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PHP 8.1 Support #353

Merged
merged 10 commits into from
Apr 20, 2021
Merged

PHP 8.1 Support #353

merged 10 commits into from
Apr 20, 2021

Conversation

dshoreman
Copy link
Owner

@dshoreman dshoreman commented Feb 24, 2021

Fixues a bunch of "null as string" deprecation errors in PHP 8.1, and updates dependencies to latest.

Temporary Exception Overrides

Both laravel/framework and laravel/sanctum throw deprecation warnings that get transformed into exceptions in tests. Forked and fixed, but PRs currently pending.

Laravel

Triggers the warning when messages for a validation rule are formatted, and getDisplayableValue() returns null when it's passed to str_replace().

Sanctum

Triggers it in the EnsureFrontendRequestsAreStateful middleware when a request lacks both a Referer and an Origin header. This breaks all tests that hit frontend routes.


Laravel also threw warnings on 6.x (or just 7? need to double check) for any routes defined without a prefix, which included the default routes used by Passport. This no longer applies here, but it's unclear whether that's because Passport was switched for Sanctum, or because Symfony packages were also updated.

Deprecation Warnings

Less important, since these can largely be ignored and won't prevent things from running.

JSON Schema

Calls strlen() in JsonSchema\Constraints\Constraint. Required by Composer, fix pending in jsonrainbow/json-schema#657.

Composer

Calls preg_replace() in Composer\Autoload\ClassMapGenerator at line 251

@dshoreman dshoreman added the enhancement New feature or request label Feb 24, 2021
@dshoreman dshoreman self-assigned this Feb 24, 2021
@codecov
Copy link

codecov bot commented Feb 24, 2021

Codecov Report

Merging #353 (ee43d0b) into develop (622be73) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             develop     #353   +/-   ##
==========================================
  Coverage      99.46%   99.46%           
  Complexity       359      359           
==========================================
  Files             68       68           
  Lines            927      929    +2     
==========================================
+ Hits             922      924    +2     
  Misses             5        5           
Impacted Files Coverage Δ Complexity Δ
app/Database.php 100.00% <100.00%> (ø) 6.00 <0.00> (ø)
app/Http/Requests/System/CreateUser.php 100.00% <100.00%> (ø) 1.00 <0.00> (ø)
app/System/Group.php 100.00% <0.00%> (ø) 30.00% <0.00%> (ø%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 622be73...ee43d0b. Read the comment docs.

@dshoreman dshoreman changed the title Ignore PHP platform requirement in 8.1 CI runs PHP 8.1 Support Feb 24, 2021
@dshoreman dshoreman marked this pull request as draft February 24, 2021 17:08
@dshoreman
Copy link
Owner Author

Checking the full stacktrace (formatted below) reveals startsWith() is being called by Illuminate\Queue\QueueServiceProvider::registerOpisSecurityKey():

https://github.com/laravel/framework/blob/c7c9d4ea5529f2f4025dff73263a8c77cde6e97b/src/Illuminate/Queue/QueueServiceProvider.php#L259:

protected function registerOpisSecurityKey()
{
    if (Str::startsWith($key = $this->app['config']->get('app.key'), 'base64:')) {
        $key = base64_decode(substr($key, 7));
    }


    SerializableClosure::setSecretKey($key);
}

This appears to have been removed since 7.x, but the root cause of the issue is env('APP_KEY') in our app config which is currently defaulting to null, not ''.


Stacktrace from the Laravel log:

strncmp(): Passing null to parameter #1 ($string1) of type string is deprecated
 at vendor/laravel/framework/src/Illuminate/Support/Str.php:650)

[stacktrace]
#0 [internal function] Illuminate\\Foundation\\Bootstrap\\HandleExceptions->handleError()
#1 vendor/laravel/framework/src/Illuminate/Support/Str.php(650)
      strncmp()
#2 vendor/laravel/framework/src/Illuminate/Queue/QueueServiceProvider.php(259)
      Illuminate\\Support\\Str::startsWith()
#3 vendor/laravel/framework/src/Illuminate/Queue/QueueServiceProvider.php(36)
      Illuminate\\Queue\\QueueServiceProvider->registerOpisSecurityKey()
#4 vendor/laravel/framework/src/Illuminate/Foundation/Application.php(627)
      Illuminate\\Queue\\QueueServiceProvider->register()
#5 vendor/laravel/framework/src/Illuminate/Foundation/Application.php(761)
      Illuminate\\Foundation\\Application->register()
#6 vendor/laravel/framework/src/Illuminate/Foundation/Application.php(741)
      Illuminate\\Foundation\\Application->registerDeferredProvider()
#7 vendor/laravel/framework/src/Illuminate/Foundation/Application.php(808)
      Illuminate\\Foundation\\Application->loadDeferredProvider()
#8 vendor/laravel/framework/src/Illuminate/Foundation/Application.php(794)
      Illuminate\\Foundation\\Application->loadDeferredProviderIfNeeded()
#9 vendor/laravel/framework/src/Illuminate/Container/Container.php(646)
      Illuminate\\Foundation\\Application->resolve()
#10 vendor/facade/ignition/src/IgnitionServiceProvider.php(94)
      Illuminate\\Container\\Container->get()
#11 vendor/laravel/framework/src/Illuminate/Container/BoundMethod.php(36)
      Facade\\Ignition\\IgnitionServiceProvider->boot()
#12 vendor/laravel/framework/src/Illuminate/Container/Util.php(37)
      Illuminate\\Container\\BoundMethod::Illuminate\\Container\\{closure}()
#13 vendor/laravel/framework/src/Illuminate/Container/BoundMethod.php(93)
      Illuminate\\Container\\Util::unwrapIfClosure()
#14 vendor/laravel/framework/src/Illuminate/Container/BoundMethod.php(37)
      Illuminate\\Container\\BoundMethod::callBoundMethod()
#15 vendor/laravel/framework/src/Illuminate/Container/Container.php(596)
      Illuminate\\Container\\BoundMethod::call()
#16 vendor/laravel/framework/src/Illuminate/Foundation/Application.php(867)
      Illuminate\\Container\\Container->call()
#17 vendor/laravel/framework/src/Illuminate/Foundation/Application.php(850)
      Illuminate\\Foundation\\Application->bootProvider()
#18 [internal function]
      Illuminate\\Foundation\\Application->Illuminate\\Foundation\\{closure}()
#19 vendor/laravel/framework/src/Illuminate/Foundation/Application.php(851)
      array_walk()
#20 vendor/laravel/framework/src/Illuminate/Foundation/Bootstrap/BootProviders.php(17)
      Illuminate\\Foundation\\Application->boot()
#21 vendor/laravel/framework/src/Illuminate/Foundation/Application.php(230)
      Illuminate\\Foundation\\Bootstrap\\BootProviders->bootstrap()
#22 vendor/laravel/framework/src/Illuminate/Foundation/Console/Kernel.php(310)
      Illuminate\\Foundation\\Application->bootstrapWith()
#23 vendor/laravel/framework/src/Illuminate/Foundation/Console/Kernel.php(127)
      Illuminate\\Foundation\\Console\\Kernel->bootstrap()
#24 artisan(37)
      Illuminate\\Foundation\\Console\\Kernel->handle()
#25 {main}

@dshoreman dshoreman force-pushed the php8.1 branch 5 times, most recently from ddced2c to 2bb5d80 Compare February 26, 2021 15:33
@dshoreman dshoreman mentioned this pull request Feb 26, 2021
@dshoreman dshoreman force-pushed the php8.1 branch 11 times, most recently from 664252c to 8a55300 Compare March 7, 2021 13:33
This should fix an ErrorException in the `QueueServiceProvider`, which
attempts to use the `app.key` value from config as the first param of
its call to `Str::startsWith()`. Because it defaults to `null`, we get
a deprecation error on PHP 8.1 which Laravel transforms to an exception.
PHP CS Fixer doesn't officially support PHP 8.1 yet, so it throws a
warning instead. It appears to run fine however, so we tell it not to
pay attention to the PHP version if it's one it doesn't support.
Not sure why other versions aren't affected, but on PHP 8.1 all the
Servidor\Database (i.e. `Doctrine\DBAL`) tests fail because of an HY000
"No such file or directory" error, presumably due to the lack of a host.
* Bumps laravel for validation/rule-formatter fix in [8.34.0][1]
* Bumps Sanctum for stateful middleware fix in [2.9.1][2]

[1]: https://github.com/laravel/framework/releases/tag/v8.34.0
[2]: https://github.com/laravel/sanctum/releases/tag/v2.9.1
Version 2.9.3 includes laravel/sanctum#264 which introduces a new
null-passing error in the updated sanctum.stateful config value, which
assumes the `APP_URL` env var is set. We only set url in config, so even
though we have our own config/sanctum.php, the default file is included
in CI because config can't be cached before composer install was run.
Macroable v2 was being required because I'm running install on php8, but
it drops support for php 7 entirely thus breaking install on 7.4.

Although spatie/ray allows v1 *or* v2, composer doesn't seem to want to
try for v1 on 7.4 even when I explicitly add it to the composer.json so
instead of messing about this just locks it to 1.x, since php8 support
has been added since 1.0.1 anyway so it's probably fine.
@dshoreman dshoreman force-pushed the php8.1 branch 3 times, most recently from 543354a to b113007 Compare April 20, 2021 00:01
In v8.32.0, laravel/framework#36504 was merged which fixes null being
treated as true by the validator, but also enforces strict checking for
boolean values given to the required_unless rule. That's not necessarily
a bad thing, it just means we can't be passing `1` when we want `true`.

When we use `1`, user tests fail because while we pass `'user_group' =>
true` in the request data, the validator checks explicitly for `1` which
it doesn't see, so it says "gid is required unless user group is in 1".
We don't want that.
@dshoreman dshoreman marked this pull request as ready for review April 20, 2021 02:19
@dshoreman dshoreman merged commit 6186bde into develop Apr 20, 2021
@dshoreman dshoreman deleted the php8.1 branch April 20, 2021 02:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant