Use of 'final' and 'private' in Psr18Adapter

[thomascorthals]

Is there any reason we can't use just class and protected for member variables and methods?

solarium/src/Core/Client/Adapter/Psr18Adapter.php

Line 16 in 559f8a8

final class Psr18Adapter implements AdapterInterface

The use of final and private contradicts our customisation philosophy.

solarium/docs/customizing-solarium.md

Line 8 in 559f8a8

3. and also still supporting the extending of classes (Solarium doesn't use 'private' or 'final' anywhere)

solarium/docs/customizing-solarium.md

Line 299 in 559f8a8

You can extend any class in Solarium, there is no use of 'private' or 'final'. However, Solarium does have several built-in class mappings for factory methods. Most important are the querytype mapping in Solarium\\Client and the component mapping in Solarium\\QueryType\\Select\\Query\\Query. In some cases you might need to alter this mappings for your classes to be used.

[mkalkbrenner]

See https://github.com/solariumphp/solarium/pull/754/files#r399572624

What is the reason for "final" (or the advantage)?

@dmaicher
I think we should not open this class for extension. Its does not make sense anyway as everything is private except public function execute(Request $request, Endpoint $endpoint): Response.

So we should not even encourage people to extend it. They should use composition if they need to customize stuff.

Or what do you think?

@mkalkbrenner
OK. Let's leave it like this and wait until someone complains ;-)

[mkalkbrenner]

I accepted the proposal for a new implementation in 5.2.0 but I agree that we could be less restrictive for 6.0.0.
@thomascorthals @dmaicher please discuss your opinions :-)

[thomascorthals]

I have missed that conversation about final. It happened to catch my eye because I was working on the docs. I have no idea how much sense it would make to extend it, it's more a matter of principle. If there's a good reason to make adapters final, we should state it in the docs. As far as unit and integration tests are concerned, it doesn't seem to matter.

A quick search of the code yields no other uses of final but quite a few privates. CONTRIBUTING.md doesn't mention it specifically so it just might be that new contributors miss this entirely.

[mkalkbrenner]

@wickedOne
@thomascorthals
@jsteggink
@thePanz
@timohund
@localheinz
@dmaicher

could we do a quick poll here?

[localheinz]

@mkalkbrenner

I am not a fan of inheritance and prefer composition instead, so for me, final classes (and consequently, private class members) work fine.

[thomascorthals]

@mkalkbrenner

I'm not opposed to using final and private if it's done consistently. E.g. make all adapters final and put something like "Solarium doesn't use final anywhere, except for adapters" in the docs.

Or if we make some classes final and others not, scratch that remark from the docs.

[wickedOne]

i'm with @localheinz and @thomascorthals on this one and would prefer our own adapters to be final. as end users are free to register their own adapter if needed, i don't see any limitations in that aspect.

of course that would mean we have to alter the remark in the customisation documentation

[dmaicher]

I would keep it private and final 😋 Reasoning see #754 (comment)

[thomascorthals]

@dmaicher I never intended to doubt your reasoning. PR #796 just happened to be less work than changing the other adapters and updating the docs. 😉

[mkalkbrenner]

I changed the docs in #803

[thomascorthals]

Should I go ahead and turn around PR #796 to make all three adapters final instead? That would break BC for anyone who has extended them, but we've already broken BC with the new Client constructor and the removal of the other adapters.

[mkalkbrenner]

I think we should not change the existing adapters.
The Client constructor is easy to adjust. But I know that people / service providers inherited the HTTP or Curl adapter in the past to add their own security layer and authentication.
I think it is OK for solarium 6 to have the new adapter declared as final and to minimize the BC breaks by not doing it for the remaining old adapters.

[thomascorthals]

Since @mkalkbrenner already changed the docs and we're not changing the implementation, I'm closing this. :)