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] Integrate Laravel CORS into framework #41137

Merged
merged 8 commits into from
Feb 21, 2022
Merged

[9.x] Integrate Laravel CORS into framework #41137

merged 8 commits into from
Feb 21, 2022

Conversation

driesvints
Copy link
Member

@driesvints driesvints commented Feb 21, 2022

This PR ports the middleware from https://github.com/fruitcake/laravel-cors over to the core Laravel framework. The main reason is that we want to remove a circular dependency we rely on additionally to the fact that we eliminate another dependency of the skeleton.

All credits for the code go to @barryvdh of @fruitcake. Thanks for maintaining that package for so long!

After this PR has been merged and tagged I'll undraft the PR that contains the changes needed for the Laravel skeleton: laravel/laravel#5825

@taylorotwell taylorotwell merged commit fc186a6 into 9.x Feb 21, 2022
@taylorotwell taylorotwell deleted the laravel-cors branch February 21, 2022 15:52
@browner12
Copy link
Contributor

Should we remove the fruitcake/laravel-cors dependency in our projects, and update our kernel middleware?

Is the intent that people upgrade this now, or wait for v10?

@barryvdh
Copy link
Contributor

You can just apply the changes listed here directly, if you are on 9 already: https://github.com/laravel/laravel/pull/5825/files

@browner12
Copy link
Contributor

Thanks @barryvdh. In addition to those changes I'm assuming we'll have to remove our local composer requirement of fruitcake/laravel-cors as well.

I guess the question is more if and how the team is going to disseminate this change to the public, since it's occurring on a minor version. I only came across this by chance after running a composer outdated. Or if the thought is people are fine as is until v10.

@barryvdh
Copy link
Contributor

It's fine either way, but you can remove the local package, if you also replace the middleware (same as the changes in Laravel/laravel)

@driesvints
Copy link
Member Author

There's no breaking change. You're totally free to leave the current package in place or move to the new one.

@jonnott
Copy link
Contributor

jonnott commented Apr 1, 2022

@driesvints Should this possibly now be mentioned in the Laravel 9 upgrade guide, as an optional change to the skeleton?

@driesvints
Copy link
Member Author

@jonnott not really since, like you say, it's optional. It's not required to upgrade.

@jonnott
Copy link
Contributor

jonnott commented Apr 1, 2022

@jonnott not really since, like you say, it's optional. It's not required to upgrade.

True, but a lot of these kind of changes have at least been mentioned-in-passing before in upgrade guides, I guess with the intention to help people maintain as 'clean' a codebase as possible as they upgrade through the versions..?

@driesvints
Copy link
Member Author

We already have a changelog for the skeleton: https://github.com/laravel/laravel/blob/9.x/CHANGELOG.md

And often announce these on Twitter: https://twitter.com/driesvints/status/1497146821491281922

The upgrade guide really is meant to only contain the bare minimum to upgrade to a new version. A better place would be the release notes maybe: https://laravel.com/docs/9.x/releases#laravel-9. But the feature is so small I don't think it's worth adding.

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.

6 participants