search

akrabat / ip-address-middleware

PSR-7 Middleware that determines the client IP address and stores it as an ServerRequest attribute
Other
168 stars 37 forks source link

Specify types for the constructor parameters #35

Closed kudashevs closed 2 years ago

kudashevs commented 2 years ago

Hello again,

Another proposition is to specify types for all the constructor parameters. However, there is a little problem with the $attributeName parameter which is null by default (but it is considered to be of the string type according to the docblock information). So, specifying its type will result in a breaking change. In my opinion, it is worth doing because it will lead to a more consistent and cleaner parameters list.

kudashevs commented 2 years ago

This is a draft of my proposition about the constructor parameters types.

If it looks more or less appropriate, please let me know. I can/want to improve it a little bit and add some additional tests to increase coverage.

kudashevs commented 2 years ago

I've just realized how many projects use it. Wow! Changing the constructor signature was a bad idea.

akrabat commented 2 years ago

I've just realized how many projects use it. Wow! Changing the constructor signature was a bad idea.

Yeah. It's the sort of thing to do when there's a new feature that requires a BC break anyway, so as we're doing a major version, we can consider doing general tidy-up things.