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

proof of concept illuminate http #3865

Draft
wants to merge 22 commits into
base: 2.x
Choose a base branch
from
Draft

proof of concept illuminate http #3865

wants to merge 22 commits into from

Conversation

SychO9
Copy link
Member

@SychO9 SychO9 commented Aug 14, 2023

I've stopped here where we have successful tests and a mostly working app.

What is left

  • adapt uploaded files to use new request api
  • fix maintenance handler (has to be done through a middleware now)
  • cache routes (haven't tested performance yet)
  • fix updater routes and installer routes

Nice to have

  • set illuminate request session & actor resolvers

Chores

  • remove psr7 middlewares from dependencies

Architectural changes
Here is a diagram that explains how this changes the base architecture

boot

Review
To make it easier reviewing the changes, here is what you should look at and ignore

Ignore: All Tests changes

Focus On: Application, SiteInterface, Server, Middlewares, Controllers, Router, RequestUtil, ExceptionHandler, HandleErrors, UrlGenerator, routes.php files. Service Providers (Forum, Admin, API).

Do we want these changes

The case for no

  • The mutable request and binding it to the container is not ideal, for instance, the client has to temporarily bind a new request and rebind the old one after the response is resolved. It could lead to more similar issues.
  • This closes us to an ecosystem of PSR-7 standards, while PSR7 Request handlers still work (controllers), other things don't (like psr7 middleware).
  • This introduces a lot of breaking changes, primarily related to the use of the new request implementation which propagates to a lot of areas (like tests).
  • Whatever features we seek from laravel that requires its request handling, we might just recreate some of it instead.

The case for Yes

  • Gets us closer to a more Laravel development experience and could attract more contributors in the future, which we haven't had for a couple years+. (This goes for both core, and ecosystem).
  • Makes integrations with the Laravel ecosystem easier.
  • ?

@tankerkiller125
Copy link
Contributor

There is a PSR-7 middleware abstraction, but it's not ideal. https://github.com/jshannon63/psr7middleware but that's the only "potential workaround for a NO item".

@luceos
Copy link
Member

luceos commented Nov 13, 2024

@SychO9 I am assuming this is off the table for now and when we revisit this, it needs to be based off of 2.x anyway? If so, can we close this, the branch can probably be left archived.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants