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

Fix upstream dependent test failure in Laravel 11.23 and later #1595

Merged

Conversation

KentarouTakeda
Copy link
Contributor

Summary

Fixed test failures in Laravel 11.23 and later. The cause is the following changes on the laravel/framework side:

I addressed the issue by changing the Laravel method that we were testing.

The following points were taken into consideration when selecting the target method:

  • It must be a method of the Eloquent Builder class, just like before the fix.
  • The content does not change between the CI options prefer-oldest and prefer-stable, and is unlikely to change in the future.
  • It must be a method that accepts multiple parameters.
  • Some of these parameters must have default values.

By satisfying these conditions, we can perform tests equivalent to with(), which was the target of tests before the fix. upsert met the conditions, so I selected it.

Note

This pull request, #1594, and #1591 can all be merged independently.

IMO, the following order is best:

  1. Merging 1594 should make all of this pull request's tests pass.
  2. If we merge this pull request, all of 1591's tests should pass.
  3. 1591 contains a breaking change. Merge it last.

Type of change

  • Misc. change (internal, infrastructure, maintenance, etc.)

Checklist

  • Existing tests have been adapted and/or new tests have been added

@barryvdh barryvdh merged commit ec61b82 into barryvdh:master Oct 17, 2024
16 of 26 checks passed
@barryvdh
Copy link
Owner

It seems to work now :)
Guess we'll just stick with the latest version for now.

@KentarouTakeda KentarouTakeda deleted the fix-upstream-dependent-test-failures branch October 17, 2024 22:58
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.

2 participants