Skip to content

Conversation

@christoph-kluge
Copy link
Contributor

Split-off from #217 and extension to #40. This PR adds the ability to pass a custom RequestHeaderParser to the server.

  • Adds option to pass a RequestHeaderParserFactoryInterface (someone up for a shorter name?)
  • If none is passed, we take the default one shipped by the server
  • If one is passed, it's taking the custom one

As soon as #241 gets merged we could also extend this factory with a custom parameter to enrich the RequestHeaderParser with for a custom maxSize. This could then complete #214.

Copy link
Member

@clue clue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for filing this PR! I like the direction this is heading, however I do not think that a factory pattern makes much sense here. As an alternative, I would suggest making sure the RequestHeaderParser is no longer stateful and can be re-used for any number of requests. What do you think about this?

@christoph-kluge
Copy link
Contributor Author

I think I would change from the factory to an RequestHeaderParserInterface instead of a concrete. As you suggest it makes sense to have the RequestHeaderParser stateless. I think will follow the PSR7-MessageInterface and implement some kind of withConnection() (or similar) which will return a cloned instance of the RequestHeaderParser to achieve immutability and stateless usage. What do you think of it @clue ?

@WyriHaximus WyriHaximus added this to the v0.8.1 milestone Nov 21, 2017
@WyriHaximus WyriHaximus modified the milestones: v0.8.1, v0.8.2 Jan 4, 2018
@WyriHaximus
Copy link
Member

Moved this to v0.8.2 as we haven't heard back from you. Are there any updates?

@christoph-kluge
Copy link
Contributor Author

@WyriHaximus waiting for @clue's feedback before I touch it again.

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.

3 participants