googleads / google-ads-php

Google Ads API Client Library for PHP
https://developers.google.com/google-ads/api/docs/client-libs/php
Apache License 2.0
280 stars 260 forks source link

Service client classes should not be marked as final. #1008

Closed 77web closed 3 months ago

77web commented 4 months ago

[DO NOT INCLUDE ANY PERSONAL OR SENSITIVE DATA - MAKE SURE TO REDACT CONTENT WHEN NECESSARY]

Your client library and Google Ads API versions:

Your environment:

Description of the bug:

Service Clients are marked as final.

Steps to reproduce:

see https://github.com/googleads/google-ads-php/tree/main/src/Google/Ads/GoogleAds/V16/Services/Client

Expected behavior:

They should not be marked as final.

Request/Response Logs:

Anything else we should know about your project / environment:

As I could not reopen #347, I wrote new issue here.

fiboknacky commented 4 months ago

Thanks for reporting. It looks like what you reported is different from #347, so it should be a separate issue.

That one is about making the GoogleAdsClient final, but from what you said, you'd like generated clients to be final. I know that the previously generated classes didn't have the "final" modifier before, but due to GAPIC v2 generation, they now have "final".

Could you tell me more what's your use case is? Are there any special functionalities that you need to inject into the classes before using them to send a request to the API server?

77web commented 4 months ago

@fiboknacky Hi, thank you for your reply. Our usecase is the same as #347, we want to unit test our class that depends of Service Client classes. For our test should not call real API, we usually mock Service Client classes, and final classes cannot be mocked in PHP.

Our code is like this:

<?php

class FooBarMyClass
{
    public function __construct(
        private readonly $someServiceClient,
    ) {
    }

    public function execute(): void
    {
        // actually some complexed logic to calculate some-proper-request-param will be here
        $this->someServiceClient->someApiCallMethod(new SomeServiceClientRequest('some-proper-request-param'));
    }
}

We want to write tests like this:

<?php

class FooBarMyClassTest extends TestCase
{
    public function testExecute(): void
    {
        // ensure ServiceClient should be called with proper request params
        $serviceClientMock = $this->createMock(SomeServiceClient::class);
        $serviceClientMock->expects($this->once())
            ->method('someApiCallMethod')
            ->with(new SomeServiceClientRequest('some-proper-request-param'))

        ;

        $SUT = new FooBarMyClass($serviceClientMock);
        $actual = $SUT->execute();
    }
}
bshaffer commented 4 months ago

@77web Follow the instructions here in order to mock classes marked as final in PHPUnit: https://tomasvotruba.com/blog/2019/03/28/how-to-mock-final-classes-in-phpunit

I was able to mock classes marked as final by installing "dg/bypass-finals: ^1.6" as a --dev dependency, and adding DG\BypassFinals::enable(); to our bootstrap.php file for testing.

NOTE: It's necessary to add this in the bootstrap file so it loads before any of the classes marked as final, otherwise it won't work.

fiboknacky commented 3 months ago

Thanks, Brent.

@77web Since the generated classes also depend on internal classes that the team wants to limit the extension, could you please adopt what @bshaffer has suggested? Thanks.