google-ads-php icon indicating copy to clipboard operation
google-ads-php copied to clipboard

Define gRPC and Protobuf verison compatible with Released version of Library

Open chiragvels opened this issue 5 years ago • 4 comments

Hi,

I would suggest to define/add gRPC and protobuf version on composer.json that is compatible with release of the new version of the Library.

So that we can prevent any stability related issues due to changes done on gRPC and Protobuf versions.

Thanks,

chiragvels avatar Dec 30 '19 09:12 chiragvels

Thanks for suggestion, but this library needs to rely on gax-php, which in turn has its own dependencies on gRPC and protobuf. I'm afraid that, like in the past, if we add the dependencies on gRPC and protobuf directly here but cannot update the versions at around the same time as those of gax-php, it will cause a conflict for people who specify dependency on both this library and gax-php directly.

We're pretty in touch with the teams managing gax-php, protobuf and gRPC, and coordinating with those teams to ensure the dependencies are always updated in the future. Pierrick, do you have any other updates or opinions on this?

fiboknacky avatar Jan 02 '20 03:01 fiboknacky

@chiragvels thanks for opening this issue.

IMO there is no perfect solution, only a trade-off between flexibility and stability. Relying on gax-php dependency rules makes things simple and flexible (useful for most environments) but it can sometimes makes things unstable (especially problematic for production environments). I think we should keep it as-is but plan mitigation action items:

  • Highlight in our documentation the stability benefit of specifying the versions of the Protobuf and gRPC dependencies and how it can be done in a project that depends on our library. We could recommend to do so for production environments.
  • Add integration testing on our end to raise issues to Protobuf and gRPC before new releases.

PierrickVoulet avatar Jan 02 '20 15:01 PierrickVoulet

I flagged this issue as enhancement: documentation could be improved to address this.

PierrickVoulet avatar Feb 28 '20 13:02 PierrickVoulet

Hi @fiboknacky , @PierrickVoulet ,

I have just updated the composer through composer update and my all functions started to fail which are using search or searchStream.

I am not sure it is the issue or not but I can see I still have 1.35.0 while checking php -r 'print phpversion("grpc");' but after updating composer it started to fail so I just added line to downgrade grpc.

No idea what was the issue, but I downgraded the grpc and it started again. I think this should be define at composer.json like protobuf would be helpful.

"google/protobuf": "^3.14.0",
"grpc/grpc":"1.35.0"    
    File: /google-ads-php/vendor/google/gax/src/Middleware/OptionsFilterMiddleware.php
    Line: 63
    Function: __invoke

    File: /google-ads-php/vendor/google/gax/src/Middleware/PagedMiddleware.php
    Line: 67
    Function: __invoke

    File: /google-ads-php/vendor/google/gax/src/Middleware/FixedHeaderMiddleware.php
    Line: 66
    Function: __invoke

    File: /google-ads-php/src/Google/Ads/GoogleAds/Lib/V6/UnaryGoogleAdsExceptionMiddleware.php
    Line: 63
    Function: __invoke

    File: /google-ads-php/src/Google/Ads/GoogleAds/Lib/V6/UnaryGoogleAdsResponseMetadataCallable.php
    Line: 64
    Function: __invoke

    File: /google-ads-php/vendor/google/gax/src/GapicClientTrait.php
    Line: 642
    Function: __invoke

    File: /google-ads-php/src/Google/Ads/GoogleAds/V6/Services/Gapic/GoogleAdsServiceGapicClient.php
    Line: 286
    Function: getPagedListResponse

Thanks,

chiragvels avatar Mar 02 '21 10:03 chiragvels

With https://github.com/googleads/google-ads-php/pull/922 implemented and the fewer occurrences of the conflicts of grpc and protobuf versions between this library's dependencies, I think this can be resolved now.

fiboknacky avatar Aug 02 '23 16:08 fiboknacky