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

mark classes as final and methods and properties as private #123

Open
wants to merge 3 commits into
base: 2.x
from

Conversation

Projects
None yet
5 participants
@dbu
Contributor

dbu commented Dec 2, 2018

Q A
Bug fix? no
New feature? no
BC breaks? yes
Deprecations? no
Related tickets partially #20
Documentation php-http/documentation#244
License MIT

What's in this PR?

Mark the remaining things final.

Why?

See #20

Checklist

  • Updated CHANGELOG.md to describe BC breaks / deprecations | new feature | bugfix

@dbu dbu force-pushed the mark-as-final branch from 222cac6 to e2dfaeb Dec 2, 2018

@dbu

This comment has been minimized.

Contributor

dbu commented Dec 2, 2018

ignore styleci here. we switched to pretty-ci, i disabled styleci for this repository now.

Show resolved Hide resolved src/BatchClient.php Outdated
Show resolved Hide resolved src/HttpClientPoolItem.php Outdated
@gmponos

This comment has been minimized.

gmponos commented Dec 2, 2018

I know that you want to enforce composition instead of inheritance but in real life scenarios there are cases that you will need to depend on implementation instead of abstraction.

This way it will make mocking impossible. Especially when clients like BatchClient do not have an abstraction layer for all of their methods.

Don't get me wrong I am all in for final but not for packages like this. If it was like guzzle having only one implementation I think it would make sense. But for packages like this one that contain multiple implementations of the same abstraction I think it's not the best thing to do...

For instance take a look at this.. it has no finals. Although I can't be sure if this was made on purpose. The point is that there will be real life scenarios that I will need to depend specifically in ArrayCache.. Same goes for clients inside this package.

That's my opinion for what is worth it.

@dbu dbu force-pushed the mark-as-final branch from e2dfaeb to 4af4acf Dec 2, 2018

@dbu dbu force-pushed the mark-as-final branch from 4af4acf to 6a15990 Dec 2, 2018

@dbu

This comment has been minimized.

Contributor

dbu commented Dec 2, 2018

hm, i see what you mean, and with the utility clients like batch or method client, you are certainly right. but i think we still want to prevent people from extending the specific clients. our options are:

  • mark them as @internal and if people rely on being able to extend the class, tell them "told you"
  • provide interfaces for those clients

i think i prefer the second option, as that is cleaner.

@php-http/owners wdyt?

@sagikazarmark and if you agree to make the clients final, can you check if its sane what i did with php spec?

@dbu dbu force-pushed the mark-as-final branch from 6a15990 to ce5e183 Dec 2, 2018

@Nyholm

This comment has been minimized.

Member

Nyholm commented Dec 4, 2018

I agree with @gmponos. If we should be really correct, we should use an interface for all classes we mark as final.

Interfaces could be added after 2.0.0 though. But (of course) we cannot add final.

@Nyholm

Nyholm approved these changes Dec 4, 2018

@sagikazarmark

This comment has been minimized.

Member

sagikazarmark commented Dec 4, 2018

TBH I don't really like the Impl suffix. Unfortunately, I don't really have an alternative. :\

@joelwurtz

This comment has been minimized.

Member

joelwurtz commented Dec 4, 2018

We could use the @final annotation ATM and then use an interface latter ?

Show resolved Hide resolved src/HttpMethodsClient.php Outdated

@dbu dbu referenced this pull request Dec 8, 2018

Merged

Make RedirectPlugin final #127

1 of 1 task complete

@dbu dbu force-pushed the mark-as-final branch from ce5e183 to 71e787d Dec 8, 2018

@dbu

This comment has been minimized.

Contributor

dbu commented Dec 8, 2018

i added the interfaces. i opted to change the classes to Impl and keep the name for the interface. this way, everywhere the class was typehinted, this will now point to the interface. the only change needed is in the bootstrap where classes are implemented.

i think the interfaces are a clean thing here and we want them. if somebody has a better idea for the naming, i am happy to change it, but i don't know one. we could have a subnamespace for the implementations, but that does not seem better :-)

if the other @php-http/owners agree, i will do a pull request on the documentation as well.

@dbu dbu force-pushed the mark-as-final branch from 71e787d to a177902 Dec 8, 2018

@dbu dbu referenced this pull request Dec 8, 2018

Closed

emulated clients #124

@dbu dbu force-pushed the mark-as-final branch from 421c371 to 67a7b24 Dec 13, 2018

@dbu dbu force-pushed the mark-as-final branch from 67a7b24 to 86b2d1a Dec 13, 2018

@dbu dbu referenced this pull request Dec 13, 2018

Open

support client-common 2.0 #291

1 of 1 task complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment