googleapis / googleapis/google-cloud-php

Consider removing final from client classes

Open
#8,588 12 comments 5 reactions 0 assignees View on GitHub
Dominant language
PHP
Stars
1.2k
Forks
463
Avg merge
2d 1h
Merged PRs (30d)
145

Description

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

As already discusses in
- https://github.com/googleapis/google-cloud-php/pull/8492
- https://github.com/googleapis/gapic-generator-php/pull/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 (https://github.com/dg/bypass-finals/issues/62) and static analysis tool like PHPStan (https://github.com/phpstan/phpstan/issues/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 !

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.