feat: add new LongRunning client, rename LongRunning namespace (and add alias for BC) #6675
Conversation
bshaffer
changed the title
| } | ||
| // Autoload the class and its alias | ||
| class_exists('\Google\LongRunning\OperationsClient'); | ||
| @trigger_error('Google\LongRunning\ApiCore\OperationsClient is deprecated and will be removed in the next major release. Use Google\LongRunning\OperationsClient instead', E_USER_DEPRECATED); |
There was a problem hiding this comment.
Do we want to do this if all of our clients are currently using this?
Maybe we add it later once we've also migrated GAPIC v1 and v2 to the new namespace?
There was a problem hiding this comment.
It's suppressed (hence the @ before the trigger_error), so it won't actually produce the error, but it will show up in logs and IDEs. So I think it's okay to do. But I am also okay removing it if you think it's overkill.
There was a problem hiding this comment.
I think it's great to add, but just not immediately, since for the most part, end users don't interact with this client directly, but all of our GAPICs do. The effect is that end users see a warning that they can't do anything about. Like they can't change the GAPIC code, only we can.
So we'd want to update GAPIC v1 surfaces to reference the new class instead, regenerate everything, push that out, then add the warning here. Does that make sense or?
There was a problem hiding this comment.
That's a very good point and definitely makes sense! Thanks for pointing it out, I'll remove the call.
| // the generated {@see OperationsGapicClient} class. | ||
| } | ||
|
|
||
| class_alias('Google\LongRunning\OperationsClient', 'Google\ApiCore\LongRunning\OperationsClient'); |
There was a problem hiding this comment.
Why do we need an Alias? My guess is that we are trying to avoid breaking changes. Is that right?
There was a problem hiding this comment.
yes that's correct! We don't want to introduce any behavioral change
Do you think this needs to happen before or after we release these changes? I assume after is fine but I'm not super clear on the implications of the GAPIC YAML files at this point. |
bshaffer
changed the title
bshaffer
changed the title
In order for docs references to render correctly, the
google-longrunningclient should be in the same namespace as its protos. This is also just a better practice, since the proto package isgoogle.longrunningfor the client, so we should use the one defined in it.We should potentially update longrunning_gapic.yaml as well (which is no longer used AFAIK).
BREAKING_CHANGE_REASON=this should not cause any breaking changes, but the detector does not recognize class aliases