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

[PHP] option exclude backwards compatible proto files #9593

Closed
bshaffer opened this issue Mar 7, 2022 · 5 comments
Closed

[PHP] option exclude backwards compatible proto files #9593

bshaffer opened this issue Mar 7, 2022 · 5 comments
Assignees

Comments

@bshaffer
Copy link
Contributor

bshaffer commented Mar 7, 2022

For PHP all nested messages generate two classes, the current namespaced class and a file to support backwards compatibility with the previous non-namespaced files. There's also a class_alias call at the bottom of every nested message. See this PR from almost 4 years ago.

As been almost 4 years, these files are now more of a nuisance. I see three potential options here, ranked from least to most risky:

  • Add a flag to tell protoc to not generate these files for PHP (opt-out, most conservative)
  • Add a flag to tell protoc to generate these files for PHP (opt-in, could potentially break some very slow to upgrade users)
  • Remove the backwards compatibility logic all together (as it's been throwing deprecation errors for almost four years, I feel comfortable with this solution)

I'm happy to give a PR a try any of these options are possible!

@haberman
Copy link
Member

haberman commented Mar 7, 2022

I think removing the backward compat logic is the way to go at this point.

Four years with deprecation warnings seems sufficient.

@bshaffer
Copy link
Contributor Author

bshaffer commented Mar 8, 2022

The only issue I can think of is for libraries like google-cloud-php, we wouldn't want to remove the class_alias code from proto files which currently have it. Not because we think it's needed, but because it would constitute a backwards-compatibility change. So what we could do is:

  1. Only remove the old alias file (e.g. Nested_Message.php) but keep the class alias (e.g. the call to class_alias at the bottom of Nested\Message.php) for now
  2. Add a flag (once again) for removing or adding the call to class_alias
  3. Not care at all and we can handle it on our end :)

@terax6669
Copy link

since the latest release still outputs deprecated files...

grep 'is deprecated and will be removed in the next major release' -R path/to/proto -l | xargs rm'

Copy link

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.

@github-actions github-actions bot added the inactive Denotes the issue/PR has not seen activity in the last 90 days. label Jan 27, 2024
@haberman
Copy link
Member

haberman commented Feb 1, 2024

We're making a breaking change release now. This is the perfect time to remove these deprecated files.

copybara-service bot pushed a commit that referenced this issue Feb 1, 2024
Fixes: #9593
PiperOrigin-RevId: 603498316
copybara-service bot pushed a commit that referenced this issue Feb 2, 2024
@github-actions github-actions bot removed the inactive Denotes the issue/PR has not seen activity in the last 90 days. label Feb 2, 2024
copybara-service bot pushed a commit that referenced this issue Feb 2, 2024
zhangskz pushed a commit that referenced this issue Feb 2, 2024
zhangskz added a commit that referenced this issue Feb 5, 2024
Fixes: #9593
Fixes: #15696
PiperOrigin-RevId: 603777295

Co-authored-by: Joshua Haberman <haberman@google.com>
deannagarcia pushed a commit to deannagarcia/protobuf that referenced this issue Jun 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants