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

[9.x] Deprecation Test Improvements #45317

Merged
merged 38 commits into from Dec 16, 2022
Merged

Conversation

crynobone
Copy link
Member

@crynobone crynobone commented Dec 15, 2022

  • Bump fakerphp/faker from ^1.9.2 to ^1.21
  • Bump league/commonmark from ^2.2 to ^2.2.1

Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com>
@crynobone crynobone changed the title Improves Deprecation Tests [9.x] Deprecation Test Improvements Dec 15, 2022
@driesvints
Copy link
Member

@crynobone I thought we already were treating deprecations as exceptions: https://github.com/laravel/framework/blob/9.x/phpunit.xml.dist#L6

This can definitely just go into the current test suite. No need for a separate workflow 👍

Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com>
@crynobone crynobone closed this Dec 15, 2022
@crynobone crynobone reopened this Dec 15, 2022
@crynobone
Copy link
Member Author

Full write-up regarding the changes: orchestral/testbench-core#97

@nunomaduro
Copy link
Member

I thought we already were treating deprecations as exceptions

@driesvints Not in application tests that actually need to boot the framework. As Laravel boots, and HandleExceptions actually proxy those deprecations to a specific log channel, if any.

@crynobone Out of curiosity, can you point me to the exact code, on testbench, that modifies the behavior on HandleExceptions so deprecations actually get reported?

@crynobone
Copy link
Member Author

Testbench would bootstrap Orchestra\Testbench\Bootstrap\HandleExceptions insteads of Laravel.

@driesvints
Copy link
Member

Hey @crynobone. I'm gonna take this one over and see if I can solve all issues. I think the right way to tackle prefer-lowest is to bump minimum versions. For anything we cannot solve through Composer we can use the already provided step here:

https://github.com/laravel/framework/blob/9.x/.github/workflows/tests.yml#L84

@driesvints
Copy link
Member

@crynobone I feel like we're still missing things here. For example, the first failing test I look at is:

1) Illuminate\Tests\Integration\Auth\ForgotPasswordTest::it_can_send_forgot_password_email
str_starts_with(): Passing null to parameter #1 ($haystack) of type string is deprecated

/home/runner/work/framework/framework/src/Illuminate/Foundation/Bootstrap/HandleExceptions.php:71
/home/runner/work/framework/framework/src/Illuminate/Foundation/Bootstrap/HandleExceptions.php:268
/home/runner/work/framework/framework/src/Illuminate/Auth/Passwords/PasswordBrokerManager.php:86
/home/runner/work/framework/framework/src/Illuminate/Auth/Passwords/PasswordBrokerManager.php:71
/home/runner/work/framework/framework/src/Illuminate/Auth/Passwords/PasswordBrokerManager.php:48
/home/runner/work/framework/framework/src/Illuminate/Support/Facades/Facade.php:338
/home/runner/work/framework/framework/tests/Integration/Auth/ForgotPasswordTest.php:53

But there's no str_starts_with call on that line. Is the real stack trace hidden somewhere?

@driesvints
Copy link
Member

Seems like it's the case in most tests: https://github.com/laravel/framework/actions/runs/3707093739/jobs/6283094947#step:9:159

1) Illuminate\Tests\Integration\Auth\ForgotPasswordWithoutDefaultRoutesTest::it_can_send_forgot_password_email_via_create_url_using
Use of "static" in callables is deprecated

/home/runner/work/framework/framework/src/Illuminate/Foundation/Bootstrap/HandleExceptions.php:71
/home/runner/work/framework/framework/src/Illuminate/Foundation/Bootstrap/HandleExceptions.php:266
/home/runner/work/framework/framework/vendor/fakerphp/faker/src/Faker/Provider/Base.php:357
/home/runner/work/framework/framework/vendor/fakerphp/faker/src/Faker/Provider/Base.php:408
/home/runner/work/framework/framework/vendor/fakerphp/faker/src/Faker/Provider/Base.php:423
/home/runner/work/framework/framework/vendor/fakerphp/faker/src/Faker/Provider/Internet.php:94
/home/runner/work/framework/framework/vendor/fakerphp/faker/src/Faker/Provider/Internet.php:48
/home/runner/work/framework/framework/vendor/fakerphp/faker/src/Faker/Generator.php:230
/home/runner/work/framework/framework/vendor/fakerphp/faker/src/Faker/Generator.php:287
/home/runner/work/framework/framework/vendor/fakerphp/faker/src/Faker/UniqueGenerator.php:48
/home/runner/work/framework/framework/vendor/orchestra/testbench-core/src/Factories/UserFactory.php:31
/home/runner/work/framework/framework/src/Illuminate/Database/Eloquent/Factories/Factory.php:446
/home/runner/work/framework/framework/src/Illuminate/Database/Eloquent/Factories/Factory.php:425
/home/runner/work/framework/framework/src/Illuminate/Database/Eloquent/Factories/Factory.php:409
/home/runner/work/framework/framework/src/Illuminate/Database/Eloquent/Concerns/GuardsAttributes.php:155
/home/runner/work/framework/framework/src/Illuminate/Database/Eloquent/Factories/Factory.php:408
/home/runner/work/framework/framework/src/Illuminate/Database/Eloquent/Factories/Factory.php:382
/home/runner/work/framework/framework/src/Illuminate/Database/Eloquent/Factories/Factory.php:276
/home/runner/work/framework/framework/tests/Integration/Auth/ForgotPasswordWithoutDefaultRoutesTest.php:78

Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com>
Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com>
Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com>
Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com>
Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com>
Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com>
Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com>
Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com>
Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com>
Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com>
Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com>
Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com>
Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com>
Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com>
Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com>
Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com>
Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com>
Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com>
Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com>
Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com>
Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com>
Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com>
Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com>
@driesvints driesvints marked this pull request as draft December 16, 2022 10:11
@driesvints driesvints assigned driesvints and unassigned driesvints Dec 16, 2022
@driesvints driesvints marked this pull request as ready for review December 16, 2022 10:19
@taylorotwell taylorotwell merged commit 684a512 into laravel:9.x Dec 16, 2022
@crynobone crynobone deleted the patch-2 branch December 16, 2022 22:29
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.

None yet

4 participants