fix(php): Remove public and weak dependencies from generated PHP.#26598
Open
alowde wants to merge 1 commit intoprotocolbuffers:mainfrom
Open
fix(php): Remove public and weak dependencies from generated PHP.#26598alowde wants to merge 1 commit intoprotocolbuffers:mainfrom
alowde wants to merge 1 commit intoprotocolbuffers:mainfrom
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
acba269 to
e2224ec
Compare
Author
|
CLA has been signed and I'm keeping this rebased on main as mentioned in CONTRIBUTING.md. |
fef2083 to
76682e9
Compare
76682e9 to
f3d47e8
Compare
Author
|
Looks like it did pass tests before I rebased to origin 😅. I'll rebase again now but avoid it in future if I see the tests have run - apologies for the hassle @esrauchg. |
f3d47e8 to
1885219
Compare
Fixes issue protocolbuffers#11146 caused by transitive dependencies referencing removed dependencies in the emitted PHP metadata classes.
1885219 to
263d4ab
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In issue #11146 people reported problems with the PHP package version 3.20.0RC3 and above crashing when the native extension was used with protobufs that had transitive dependencies. This comment noted that it looked like an issue with the dependency list being modified.
This PR resolves the issue by also removing public and weak dependency references in the emitted PHP metadata classes.
I've added a unit test showing that it compiles OK, but the unit tests don't exercise the PHP side so I've taken the liberty of extending the reproducer case kindly provided by @tforesti and pushed a version that shows the fix to github.com/alowde/php-protobuf-ext-issue. The Dockerfile builds a copy of
protocfrom my branch and shows that the patched version doesn't cause a crash on import when the image is launced.An LLM was used to investigate the issue and write the unit test, with human review. The fix itself and the fix demonstration were written by @alowde. The fix demo builds on the failure case Dockerfile by @tforesti to provide a clear demonstration of the fix.
Please let me know if any further information or changes would be useful. Thanks!