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

Replace server request via inheritance #359

Closed
wants to merge 5 commits into from

Conversation

alexpts
Copy link

@alexpts alexpts commented Apr 5, 2020

Added the ability to replace server request via inheritance.

This will make it possible to use a different psr7 implementation for ServerRequestInterface

@clue
Copy link
Member

clue commented Apr 5, 2020

@alexpts Thanks for filing this PR!

I'm not a big fan of exposing these classes and using inheritance to override internal behavior, but perhaps I'm missing some context. The motivation is currently unclear, what exactly is gained by using a different PSR-7 implementation here?

It's currently my understanding a middleware implementation would be the preferred approach if you need a specific implementation.

@alexpts
Copy link
Author

alexpts commented Apr 5, 2020

Nothing changes architecturally.

But this allows you to use another implementation that suits the project more.I do not need to make a bridge from default request to other request. It possible configure react/http for different preferences.

I think it’s not necessary to use inheritance. I can propose an option without inheritance that will allow me to set a parser.

Example:

$server = new StreamingServer($app);
$server->setParser($customRequestParser);
$server->listen(new \React\Socket\Server(9000, $loop));

Think it is will be better. What do you think about?

@ghost
Copy link

ghost commented Apr 6, 2020

I think the preferred way would be to use a ServerRequestFactory along these lines. IMO both presented options break isolation (although I prefer final to be gone 😉).

You'd preferable expose the option to set a factory in the constructor, which would be passed down to the parser.

@WyriHaximus
Copy link
Member

Nothing changes architecturally.

But this allows you to use another implementation that suits the project more.I do not need to make a bridge from default request to other request. It possible configure react/http for different preferences.

Did you run into specific issues that you want to go for this?

I think it’s not necessary to use inheritance. I can propose an option without inheritance that will allow me to set a parser.

Example:

$server = new StreamingServer($app);
$server->setParser($customRequestParser);
$server->listen(new \React\Socket\Server(9000, $loop));

Think it is will be better. What do you think about?

Was personally thinking along the lines of:

$server = StreamingServer::createWithRequestParser($customRequestParser, $app);
$server->listen(new \React\Socket\Server(9000, $loop));

I think the preferred way would be to use a ServerRequestFactory along these lines. IMO both presented options break isolation (although I prefer final to be gone ).

You'd preferable expose the option to set a factory in the constructor, which would be passed down to the parser.

This would have my preference to be honest, loving the idea of custom request parsers, but that seems overkill for the implied issue at hid

@SimonFrings
Copy link
Member

It seems like this pull request is open for quite a while now and didn't receive any updates since. Last comment is over a year old and I want to avoid pull requests laying around for too long without any further communication, so I'd suggest we close this for now. If this topic is still relevant and needs some further investigation, we can always reopen it in the future and take another look 👍

If there are no objections I will come back and close this after a few days.

@alexpts alexpts closed this Oct 28, 2022
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.

None yet

4 participants