Consider removing final from client classes

[VincentLanglet]

cc @bshaffer, as asked I move the discussion to an issue.

As already discusses in

• [Translate] Remove final from TranslationServiceClient #8492
• Remove final from client without interfaces gapic-generator-php#774

The current fact that client like TranslationServiceClient are final have the current drawback of

• not being able to be extended
• therefor not being able to be mocked for tests

While I do understand that google does not want such service to be extended (and I don't see a real need personally), on the contrary it's annoying to me to not being able to mock such external-client service in tests.
The current proposed solution is to use https://github.com/dg/bypass-finals in order to remove the final keyword at runtime.

I encounter the following (at least two) issue with this bypass library:

• First, if google use final but promote using this lib to remove the keyword, it doesn't forbid anyone to extend the final classes and therefor it's useless to add a final keyword at the first place
• Secondly and most annoying, both PHPStorm ([DISCUSSION] Doubling final classes inspection in PHPStorm dg/bypass-finals#62) and static analysis tool like PHPStan (PHPStan unresolvable type for intersection of final and MockObject phpstan/phpstan#8901) are not understanding bypass-final and report errors.

The two possible solutions I have in mind would be:

• Removing final hard keyword and use the soft one with phpdoc @final.
• Or introducing an interface for each existing client, listing every public method of the client.

While the second one seems to be the right one, I understand google might not want to introduce 100 new interfaces with tons of methods and prefer the first one as a quickwin.

Quoting:

I don't want to be stubborn on this, however. If there's more support from our developers that bypass-finals is not sufficient, and using final is too much of a pain, I'm happy to remove it from the client libraries!

So if you encounter any issue with those final classes, please share a word !

[Griffon-Weglot]

Hi !

I agree, the final keyword on client classes makes them impossible to mock, which is a major pain point for unit testing.

The suggested bypass-finals workaround is not ideal as it causes issues with static analysis tools like PHPStan and degrades the overall developer experience.

Switching to a @Final PHPDoc annotation would be a good compromise. Thanks for considering it!

[jeromegamez]

Without wanting to go into the debate if final should be used or not in general, I'd avoid trying to mock final classes (or implementations in general) directly, but introduce the interfaces in your project instead.

To take the example of the TranslationServiceClient you mentioned in the other PR. If you have

class TranslationService
{
    public function __construct(private TranslationServiceClient $client) {}

    public function translate(string $input): string
    {
        return $this->client->translate($input);
    }
}

It's usually better to depend on an interface instead:

interface TranslationService
{
    public function translate(string $input): string;
}

final class GoogleTranslationService implements TranslationService
{
    public function __construct(private TranslationServiceClient $client) {}

    public function translate(string $input): string
    {
        return $this->client->translate($input);
    }
}

final class DeeplTranslationService implements TranslationService
{
    public function __construct(private DeeplServiceClient $client) {}

    public function translate(string $input): string
    {
        return $this->client->translate($input);
    }
}

final class FakeTranslationService implements TranslationService
{
    public function translate(string $input): string
    {
        return "[translated] $input";
    }
}

This lets you mock or fake your own TranslationService in tests, and keep the real implementations for integration testing.

It also makes sense that the Google libraries themselves bypass final in their own tests - they're testing their own concrete implementations, so instrumenting them internally is a different context than application-level testing, where vendor classes should be treated as black boxes.

[VincentLanglet]

class TranslationService
{
public function __construct(private TranslationServiceClient $client) {}

public function translate(string $input): string
{
    return $this->client->translate($input);
}

I'm currently already using a Facade in order to mock the client. But since we're using it in multiple project, it forces us to duplicate such code while it would be so much easier to just remove the final keywork or providing an interface on Google side, following best practices.

Also, you're missing the real testing issue with your "dumb" TranslationService implementation

public function translate(string $input): string
    {
        return $this->client->translate($input);
    }

Since TranslationServiceClient and DeeplServiceClient doesn't use the same methods, and the same logic you end up with

final class GoogleTranslationService implements TranslationService
{
    public function __construct(private TranslationServiceClient $client) {}

    public function translate(string $input): string
    {
        // Lot of string manipulation specific for Google

        $result = $this->client->translateMethodOfGoogle($input);

        // Lot of result manipulation specific for Google

        return $result;
    }
}

final class DeeplTranslationService implements TranslationService
{
    public function __construct(private DeeplServiceClient $client) {}

    public function translate(string $input): string
    {
        // Lot of string manipulation specific for Deepl

        $result = $this->client->translateMethodOfDeepl($input);

        // Lot of result manipulation specific for Deepl

        return $result;
    }
}

and with logic in your translationService, you'll want to test your TranslationService::translate method now, which is not possible since TranslationServiceClient is not mockable.

I'm currently solving my issue with a

class GoogleClient
{
    public function __construct(
        private readonly TranslationServiceClient $translationServiceClient,
    ) {
    }

    public function translateText(TranslateTextRequest $request, array $callOptions = []): TranslateTextResponse
    {
        return $this->translationServiceClient->translateText($request, $callOptions);
    }
}

class and injecting the GoogleClient inside the GoogleTranslationService because my GoogleClient is mockable.

[jeromegamez]

Thank you for the detailed explanation 🙏🏻. Aside from the Fake being too dumb (it is 😅), isn't it still the case that GoogleTranslationService and DeeplTranslationService are for integration tests (where you don't have to mock), and a mock/fake for unit tests?

(Genuine question, I'm here to learn as well)

[MaxdeoxiS]

Hi @bshaffer,

I would recommend reading the article https://ocramius.github.io/blog/when-to-declare-classes-final/.
final keyword is promoted indeed, but only under the following assumptions:

• There is an abstraction (interface) that the final class implements
• All of the public API of the final class is part of that interface

So this definitely goes in the direction of this issue.

[VincentLanglet]

isn't it still the case that GoogleTranslationService and DeeplTranslationService are for integration tests (where you don't have to mock), and a mock/fake for unit tests?

(Genuine question, I'm here to learn as well)

This is a valid question @jeromegamez.

We do have few integration test, but would like to avoid having too much and running them too often because

• they rely on the fact Google/Deepl is not down
• they consume Google/Deepl credit
• they are slower
• they sometimes ends up in false failure (because Google might translate a word into Hello one day, and Hi another).

So we also works with unit tests for those class in order to

• simulate a client exception to ensure we're handle them correctly
• simulate lot of different payload and ensure we're manipulating correctly the input/output
• have quick checks which doesn't cost any cent by making API calls.

[jeromegamez]

Thanks for the further details, I absolutely understand the pain points here!

I'm facing them as well in one particular project using Google APIs - they don't cost me money, but can be brittle. In my case it's only acceptable because I'm the single maintainer, know when to allow failing tests, and don't have a deployment pipeline that requires all tests to succeed)

Replacing final with the @final annotation seems like a good compromise - but I have to add from experience that it can lead to "I extended the class and now it doesn't work anymore" reports. That's, of course, a skill issue, but it can lead to noise, especially in a widely used project.

[Noah-Alexander-Jaeger]

I ran into the same issue today. After reading through @VincentLanglet’s PR discussions, I agree—having final on the clients results in a lot of overhead for a simple unit-test when using dg/bypass-finals. VincentLanglet explained the disadvantages of dg/bypass-finals really well, and from what I can tell, it’s still not the cleanest solution.

[bshaffer]

The issue with removing the final keyword is it allows our clients to be extended in ways we don't support. The issue with adding interfaces for each client is we would add ~1000 new files that we would then have to maintain.

I am not strongly opposed to either of these options, I just think what we have currently (implementing final) is the best one, as I have yet to hear a convincing downside other than not wanting to use dg/bypass-finals.

[VincentLanglet]

as I have yet to hear a convincing downside other than not wanting to use dg/bypass-finals.

Multiple tools are not compatible bypass-finals (which is understandable since it's a big "hack").
I gave you example in my initial message: both PHPStorm (dg/bypass-finals#62) and static analysis tool like PHPStan (phpstan/phpstan#8901) are not understanding bypass-final and report errors.

Also, adding final and recommending to use bypass final is paradoxical since user could still just extend your clients in a way you don't support thanks to bypass finals. So it's basically the same than just a phpdoc @final.

[bshaffer]

@VincentLanglet et al

after discussing this with my team, I've decided to implement interfaces for all the service classes. Does this sound like an acceptable solution to everyone?

[VincentLanglet]

It does, thanks.