Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Spanner] class ReadOnly throws error in PHP 8.1 since it became a reserved word #5129

Closed
taka-oyama opened this issue Feb 17, 2022 · 13 comments
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@taka-oyama
Copy link
Contributor

taka-oyama commented Feb 17, 2022

Trying to use google/cloud-spanner/src/V1/TransactionOptions/ReadOnly.php results in error because readonly became a reserved word in PHP 8.1.

Would it be possible to rename this class?

ParseError {#1842
#message: "syntax error, unexpected token "readonly", expecting identifier"
#code: 0
#file: "./vendor/google/cloud-spanner/src/V1/TransactionOptions/ReadOnly.php"
#line: 16

Environment details

  • PHP version: 8.1

Steps to reproduce

  1. use PHP 8.1 and instantiate class.

Issue was first raised at colopl/laravel-spanner#38
And more detail was revealed in googleapis/gax-php#374

@yoshi-automation yoshi-automation added triage me I really want to be triaged. 🚨 This issue needs some love. labels Feb 17, 2022
@dwsupplee dwsupplee added priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. and removed triage me I really want to be triaged. 🚨 This issue needs some love. labels Feb 22, 2022
@bshaffer
Copy link
Contributor

We have a fix submitted to protobuf here: protocolbuffers/protobuf#9633

@taka-oyama
Copy link
Contributor Author

taka-oyama commented May 9, 2022

Thank you for the fix.

I noticed protocolbuffers/protobuf#9633 was merged to master some time ago.

Was this a breaking change? Do we have to wait until protobuf 4.0 for this to be released?

@bshaffer
Copy link
Contributor

bshaffer commented May 9, 2022

Thanks @taka-oyama. Yes, unfortunately we are still waiting for Protobuf to cut a release containing those changes. We will update these packages as soon as it happens.

Fortunately, it will not be a breaking change

@taka-oyama
Copy link
Contributor Author

taka-oyama commented May 12, 2022

Hi, thank you for the quick reply.
I was able to confirm that the fix was included in the v3.21.0 (the tag says v21 but I'm guessing that's a typo?) 🎉.

Hopefully the change will be applied here soon.

@taka-oyama
Copy link
Contributor Author

Status update.

21.0 was officially released. 🚀

https://github.com/protocolbuffers/protobuf/releases/tag/v21.0

@taka-oyama
Copy link
Contributor Author

We are eagerly waiting for this fix to be included in the release.
Are there any estimates on when this might be fixed?

@bshaffer
Copy link
Contributor

@taka-oyama unfortunately we discovered a bug in the fix we had and we had to revert the changes. We've submitted a secondary fix here.

We will need to wait until the next release before this rolls out. And because the fix requires a new version of protobuf AND a new version of this library, we have to wait for them to release a new version of protobuf before we can fix it here.

It will be at least two more weeks before this happens, but we are working to get this fix out as quickly as we can.

@taka-oyama
Copy link
Contributor Author

Thank you for the quick response.

It will be at least two more weeks before this happens, but we are working to get this fix out as quickly as we can.

OK. Understood.

@taka-oyama
Copy link
Contributor Author

Hi, sorry for the constant ping but I noticed the protocolbuffers/protobuf#10041 has been silent for over 2 weeks now.

Will this be addressed any time soon?

@taka-oyama
Copy link
Contributor Author

Merged and released!

https://github.com/protocolbuffers/protobuf/releases/tag/v21.3

@bshaffer
Copy link
Contributor

This has been fixed in #5377

@taka-oyama
Copy link
Contributor Author

Hi, thank you for the fix!

@bshaffer
Copy link
Contributor

You're very welcome! See 0.186.0 for the release notes. The individual packages which were affected by this were:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Development

No branches or pull requests

4 participants