search

php-http / message

HTTP Message related tools
http://php-http.org
MIT License
1.29k stars 41 forks source link

Added deprecation notification on our factories #106

Closed Nyholm closed 4 years ago

Nyholm commented 5 years ago
Q A
Bug fix? no
New feature? no
BC breaks? no
Deprecations? yes
Related tickets fixes #105
Documentation
License MIT

What's in this PR?

PSR-17 is out. Our factories are not BC compatible with PSR-17 because of PHP7 return types. I think we should remove our factories in version 2.

Note. I do not suggest we plan for a version 2 release.

dbu commented 5 years ago

should we additionally deprecate our factory interfaces? i'd say so.

also, symfony does a trigger_error('...', E_USER_DEPRECATED); to make the problem more visible. should we do that here too?

Nyholm commented 5 years ago

I think we should deprecate our interfaces yes.

About trigger_error. I was considering that. I don't think it is needed. Since we currently have not plan of ever release a new major version I don't think we need to make it very visible. This annotation will show that you should never create a new implementation with these factories

dbu commented 5 years ago

agreed, so lets not trigger. can you please update this with deprecation on the interface and fix the code style issue (blank line around @deprecated line) ?

Nyholm commented 5 years ago

I fixed the code style issues.

Here is a PR for the interfaces: https://github.com/php-http/message-factory/pull/39

gmponos commented 5 years ago

There is also one more reason not to have trigger_error and that is that libraries like guzzle do not have factories implemented yet.

Otherwise my opinion would've been that it would be better to raise the error.

sagikazarmark commented 5 years ago

I would say this PR should be the last one merged and we could even wait for guzzle to release PSR-17

dbu commented 5 years ago

would this now be ready to merge? is guzzle psr-17 released?

dbu commented 5 years ago

@Nyholm what is the state of this?

dbu commented 4 years ago

@Nyholm do we want to merge this?

Nyholm commented 4 years ago

Yes, Im 👍 for merging.

I've updated the warning and rebased. The CS issues are unrelated.