Skip to content
This repository has been archived by the owner on Jun 28, 2022. It is now read-only.

fix: don't change PHP functions which are reserved words #3317

Merged
merged 3 commits into from
Dec 22, 2020

Conversation

bshaffer
Copy link
Contributor

@bshaffer bshaffer commented Dec 9, 2020

with the exception of __halt_compiler, which is so unlikely to be generated as a function name that we don't need to write a case for it, none of the PHP keywords are invalid as class function names. We can safely remove wrapping them.

@bshaffer bshaffer requested a review from a team as a code owner December 9, 2020 18:02
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Dec 9, 2020
@codecov
Copy link

codecov bot commented Dec 9, 2020

Codecov Report

Merging #3317 (586dcde) into master (0456289) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #3317   +/-   ##
=========================================
  Coverage     87.14%   87.14%           
- Complexity     6120     6121    +1     
=========================================
  Files           495      495           
  Lines         24172    24172           
  Branches       2637     2637           
=========================================
+ Hits          21064    21065    +1     
  Misses         2242     2242           
+ Partials        866      865    -1     
Impacted Files Coverage Δ Complexity Δ
.../google/api/codegen/util/php/PhpNameFormatter.java 77.27% <100.00%> (ø) 14.00 <3.00> (ø)
.../java/com/google/api/codegen/discovery/Schema.java 84.84% <0.00%> (+0.50%) 43.00% <0.00%> (+1.00%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0456289...586dcde. Read the comment docs.

@bshaffer bshaffer changed the title fix: only wrap PHP function keywords which are parse errors fix: don't change PHP functions which are reserved words Dec 9, 2020
Copy link
Contributor

@vam-google vam-google left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can it result in a breaking change for the existing PHP gapic clients which already have the function names changed even though it was not necessary.

@vam-google
Copy link
Contributor

@bshaffer LGTM with a question

@bshaffer
Copy link
Contributor Author

@vam-google I searched for the function prefix _( in google-cloud-php and there were no results. I believe this is sufficient to ascertain there are no conflicts!

Copy link
Contributor

@vchudnov-g vchudnov-g left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, but I'm no PHP expert

@bshaffer
Copy link
Contributor Author

@jdpedrie caught that this is only the case for PHP 7.0+

Versions of PHP 5.6 and below will throw fatal errors for functions named with reserved words

@vam-google vam-google merged commit 3ff8ca9 into master Dec 22, 2020
@bshaffer
Copy link
Contributor Author

Note: Removing the escaping of functions for PHP Reserved Words for all GAPICs exposes the very low-likelyhood that we might generating a GAPIC with a PHP Fatal Error on PHP < 7 between now and when we deprecate support for PHP < 7.

The plan is to deprecate support in early 2021. If such a fatal error occurs, we will be able to wrap the functions in reserved words for all but DIREGAPIC client libraries, which will all require PHP > 7 from the start.

@bshaffer bshaffer deleted the fix-unnecessarily-reserved-functions branch December 22, 2020 22:45
bshaffer added a commit that referenced this pull request Dec 29, 2020
vam-google pushed a commit that referenced this pull request Dec 29, 2020
@release-please release-please bot mentioned this pull request Dec 29, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants