PHP: Deprecation message for "return_immediately" is thrown which is impossible to fix

[bshaffer]

See googleapis/google-cloud-php#5350

@pkruithof has reported the following, which seems to be an error in the Protobuf package. The function getReturnImmediately is being called internally without any way (as far as we can tell) to prevent it from happening:

Because the return_immediately option has been deprecated some time ago, this produces a deprecation log on every pull request:

[2022-06-17T06:57:33.459907+00:00] deprecation.INFO: User Deprecated: return_immediately is deprecated. {"exception":"[object] (ErrorException(code: 0): User Deprecated: return_immediately is deprecated. at /app/vendor/google/cloud-pubsub/src/V1/PullRequest.php:138)"} []
[2022-06-17T06:57:33.460273+00:00] deprecation.INFO: User Deprecated: return_immediately is deprecated. {"exception":"[object] (ErrorException(code: 0): User Deprecated: return_immediately is deprecated. at /app/vendor/google/cloud-pubsub/src/V1/PullRequest.php:118)"} []
[2022-06-17T06:57:33.460353+00:00] deprecation.INFO: User Deprecated: return_immediately is deprecated. {"exception":"[object] (ErrorException(code: 0): User Deprecated: return_immediately is deprecated. at /app/vendor/google/cloud-pubsub/src/V1/PullRequest.php:118)"} []

At first I thought to submit a workaround for this, so that the deprecation is only shown when the option is set to true (false is the default):

    public function getReturnImmediately()
    {
        if ($this->return_immediately === true) {
            @trigger_error('return_immediately is deprecated.', E_USER_DEPRECATED);
        }
        return $this->return_immediately;
    }

The setter should still always trigger the deprecation, so you still see it when you use the option and set it to false.

However, then I noticed that this code is generated. So I don't think this is possible, unless there's a way to customize generated classes like this?

In any case, I would very much like to be able to remove this deprecation from our processes, as it's polluting our logs, making them harder to read. Is there another way to go here?

Here is the stack trace

PHP Fatal error:  Uncaught ErrorException: return_immediately is deprecated. in /app/vendor/google/cloud-pubsub/src/V1/PullRequest.php:167
Stack trace:
#0 [internal function]: {closure}(16384, 'return_immediat...', '/app/vendor/goo...', 167)
#1 /app/vendor/google/cloud-pubsub/src/V1/PullRequest.php(167): trigger_error('return_immediat...', 16384)
#2 /app/vendor/google/protobuf/src/Google/Protobuf/Internal/Message.php(1598): Google\Cloud\PubSub\V1\PullRequest->getReturnImmediately()
#3 /app/vendor/google/protobuf/src/Google/Protobuf/Internal/Message.php(1842): Google\Protobuf\Internal\Message->existField(Object(Google\Protobuf\Internal\FieldDescriptor))
#4 /app/vendor/google/protobuf/src/Google/Protobuf/Internal/Message.php(1931): Google\Protobuf\Internal\Message->fieldByteSize(Object(Google\Protobuf\Internal\FieldDescriptor))
#5 /app/vendor/google/protobuf/src/Google/Protobuf/Internal/Message.php(1564): Google\Protobuf\Internal\Message->byteSize()
#6 /app/vendor/grpc/grpc/src/lib/AbstractCall.php(117): Google\Protobuf\Internal\Message->serializeToString()
#7 /app/vendor/grpc/grpc/src/lib/UnaryCall.php(39): Grpc\AbstractCall->_serializeMessage(Object(Google\Cloud\PubSub\V1\PullRequest))
#8 /app/vendor/grpc/grpc/src/lib/BaseStub.php(295): Grpc\UnaryCall->start(Object(Google\Cloud\PubSub\V1\PullRequest), Array, Array)
#9 /app/vendor/grpc/grpc/src/lib/BaseStub.php(545): Grpc\BaseStub->Grpc\{closure}('/google.pubsub....', Object(Google\Cloud\PubSub\V1\PullRequest), Array, Array, Array)
#10 /app/vendor/google/gax/src/Transport/GrpcTransport.php(218): Grpc\BaseStub->_simpleRequest('/google.pubsub....', Object(Google\Cloud\PubSub\V1\PullRequest), Array, Array, Array)
#11 /app/vendor/google/gax/src/GapicClientTrait.php(751): Google\ApiCore\Transport\GrpcTransport->startUnaryCall(Object(Google\ApiCore\Call), Array)
#12 /app/vendor/google/gax/src/Middleware/CredentialsWrapperMiddleware.php(59): Google\Cloud\PubSub\V1\Gapic\SubscriberGapicClient->Google\ApiCore\{closure}(Object(Google\ApiCore\Call), Array)
#13 /app/vendor/google/gax/src/Middleware/FixedHeaderMiddleware.php(68): Google\ApiCore\Middleware\CredentialsWrapperMiddleware->__invoke(Object(Google\ApiCore\Call), Array)
#14 /app/vendor/google/gax/src/Middleware/RetryMiddleware.php(85): Google\ApiCore\Middleware\FixedHeaderMiddleware->__invoke(Object(Google\ApiCore\Call), Array)
#15 /app/vendor/google/gax/src/Middleware/OptionsFilterMiddleware.php(62): Google\ApiCore\Middleware\RetryMiddleware->__invoke(Object(Google\ApiCore\Call), Array)
#16 /app/vendor/google/gax/src/GapicClientTrait.php(725): Google\ApiCore\Middleware\OptionsFilterMiddleware->__invoke(Object(Google\ApiCore\Call), Array)
#17 /app/vendor/google/cloud-pubsub/src/V1/Gapic/SubscriberGapicClient.php(1264): Google\Cloud\PubSub\V1\Gapic\SubscriberGapicClient->startCall('Pull', 'Google\\Cloud\\Pu...', Array, Object(Google\Cloud\PubSub\V1\PullRequest))
#18 [internal function]: Google\Cloud\PubSub\V1\Gapic\SubscriberGapicClient->pull('projects/dev-pk...', 1000, Array)
#19 /app/vendor/google/cloud-core/src/ExponentialBackoff.php(97): call_user_func_array(Array, Array)
#20 /app/vendor/google/cloud-core/src/GrpcRequestWrapper.php(148): Google\Cloud\Core\ExponentialBackoff->execute(Array, Array)
#21 /app/vendor/google/cloud-core/src/GrpcTrait.php(82): Google\Cloud\Core\GrpcRequestWrapper->send(Array, Array, Array)
#22 /app/vendor/google/cloud-pubsub/src/Connection/Grpc.php(411): Google\Cloud\PubSub\Connection\Grpc->send(Array, Array)
#23 /app/vendor/google/cloud-pubsub/src/Subscription.php(688): Google\Cloud\PubSub\Connection\Grpc->pull(Array)
#24 /app/bin/pubsub.php(18): Google\Cloud\PubSub\Subscription->pull()
#25 {main}
  thrown in /app/vendor/google/cloud-pubsub/src/V1/PullRequest.php on line 167

[fowles]

https://github.com/googleapis/googleapis/blob/master/google/pubsub/v1/pubsub.proto#L1227-L1228 this field is marked deprecated. If you wish to not have the deprecation warning, you need to mark the field as non-deprecated.

[pkruithof]

The issue is not that the field is deprecated, but that some internal call is being done which triggers the deprecation warning, without any way to avoid that.

Please consider reopening the issue, as it is not fixed.

[fowles]

Ah, I misunderstood. Is there a way to suppress specific calls to deprecated methods at the callsite?

[pkruithof]

Well you can suppress these kinds of warnings in PHP, but that would either mean disabling all deprecated warnings, or doing some hacks to only disable them for this particular use case. Neither are really preferable IMO: I think when you're specifically not using this option, the library should not warn about using it.

[s2x]

Maybe it's not the best way, but I got rid of this error from the logs by setting the transport method to rest

$client = new PubSubClient(['transport'=>'rest']);

[github-actions]

We triage inactive PRs and issues in order to make it easier to find active work. If this issue should remain active or becomes active again, please add a comment.

This issue is labeled inactive because the last activity was over 90 days ago.

[pkruithof]

Issue is still active.

[bshaffer]

I see two potential solutions here:

1. Since this call is used internally, we could just remove the trigger_error warning, and say that the @deprecated tag in phpdoc will suffice
2. Similar to @pkruithof's suggestion, we can wrap trigger_error in a conditional so it only happens if the field has been set. This would look something like this:

    public function getSomeDeprecatedField()
    {
        if ($this->some_deprecated_field !== null) {
            @trigger_error('some_deprecated_field is deprecated.', E_USER_DEPRECATED);
        }
        return $this->some_deprecated_field;
    }

@fowles I'm happy to submit a PR for either of these if you think they're acceptable.

[github-actions]

We triage inactive PRs and issues in order to make it easier to find active work. If this issue should remain active or becomes active again, please add a comment.

This issue is labeled inactive because the last activity was over 90 days ago.

[pkruithof]

Issue is still active.

Either solution presented by @bshaffer looks good to me, @fowles can we move this issue along? Since we're nearing the 2-year mark since the start of it 😉

[github-actions]

We triage inactive PRs and issues in order to make it easier to find active work. If this issue should remain active or becomes active again, please add a comment.

This issue is labeled inactive because the last activity was over 90 days ago.

[bshaffer]

I can also take this on, if the solution is approved!

[pkruithof]

@fowles can you give a 👍 or 👎 in this so we can either fix it, or find another solution to it?

[googleberg]

@fowles has taken on expanded scope. I'm taking on his role on the protobuf team.

Experience with doc comments is that they are ignored -- a reasonable amount of log nagging is a good nudge in the right direction. Option 2 (triggering deprecation logging only when set), looks good from a protobuf perspective. @bshaffer @pkruithof I leave it to the two of you to decide who implements.

[github-actions]

We triage inactive PRs and issues in order to make it easier to find active work. If this issue should remain active or becomes active again, please add a comment.

This issue is labeled inactive because the last activity was over 90 days ago. This issue will be closed and archived after 14 additional days without activity.

[pkruithof]

The issue is still active, however I made a start for the fix in #17607, only I need some help getting it finished. I was hoping @bshaffer could find some time so we can (finally) get this issue resolved 😁 🤞

[bshaffer]

@pkruithof I'll take a look!

[bshaffer]

@pkruithof I've made my own PR for this, because there were some tricky implementation details. See #17788

[patrick-levesque]

@bshaffer Any update on this? I've checked your PR and it seems it has not yet been merged. Having the same issue with deprecated warning on every pull requests.

[pkruithof]

It is actually merged, it seems. I thought there wasn't a release with this change yet, but it seems it's available since v29.0: https://github.com/protocolbuffers/protobuf/releases/tag/v29.0 🎉

[edit] spoke too soon, because it's not landed in the PubSub library yet I think...

[bshaffer]

You're right, we haven't upgraded to protobuf v29 yet, we are still on v25

https://github.com/googleapis/googleapis/blob/master/WORKSPACE#L152

[pkruithof]

Do you have an estimate when this will be upgraded?

[bshaffer]

@pkruithof not yet but I'll look into it