-
Notifications
You must be signed in to change notification settings - Fork 214
Disable SDK from autoloading during composer execution. #1727
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
base: main
Are you sure you want to change the base?
Disable SDK from autoloading during composer execution. #1727
Conversation
483a850 to
c68f0dd
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1727 +/- ##
============================================
- Coverage 68.37% 68.25% -0.12%
- Complexity 2971 2974 +3
============================================
Files 448 449 +1
Lines 9020 8720 -300
============================================
- Hits 6167 5952 -215
+ Misses 2853 2768 -85
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 216 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
c68f0dd to
58b8ecc
Compare
58b8ecc to
9ec2a8c
Compare
| * if the version of PSR-3 installed in ./vendor conflicts with that of the | ||
| * packaged composer PSR-3 library. | ||
| * | ||
| * If COMPOSER_DEV_MODE is present, then we can assume that a composer script |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would happen if you were starting your application through a composer script? eg something like composer run-my-application ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously, if you were using a compatible version of psr/log then the SDK would bootstrap.
If you were using an incompatible version, it would cause a fatal exception.
With the proposed changes, the SDK is essentially disabled.
src/SDK/_autoload.php
Outdated
| * | ||
| * @see https://github.com/open-telemetry/opentelemetry-php/issues/1673 | ||
| */ | ||
| if (getenv('COMPOSER_DEV_MODE') === false) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if (getenv('COMPOSER_DEV_MODE') === false) { | |
| if (getenv('COMPOSER_BINARY') === false) { |
COMPOSER_DEV_MODE is only set by the run-script command, running scripts directly still fails:
OTEL_PHP_AUTOLOAD_ENABLED=true composer test-psr3Edit: checking solely for COMPOSER_BINARY will be too strict as this environment variable is also propagated to @php scripts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Nevay Ah, that is unfortunate - thanks for highlighting. Realistically, there may be no safe way to do this then? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not start with COMPOSER_DEV_MODE, since that covers some failure cases? It could be broken out into its own class+function such as ComposerHandler::isRunning() to allow for future expansion of methods to identify whether composer is running?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've made the suggested change to move to a ComposerHandler::isRunning() check.
I have also added an additional rudimentary check to try to infer whether or not the composer executable has been called directly.
This does fix the problem with running this way:
OTEL_PHP_AUTOLOAD_ENABLED=true composer test-psr3
Are there any foreseen problems in doing these additional checks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also noted a discussion started here which links to this issue.
Proposal to prevent the SDK from auto-loading during composer script execution.
During a composer script, the packaged dependencies in the
composer.pharare currently locked to PSR-3 v2 and thus any local application dependencies which expect v3 can trigger a fatal error.By preventing the
SdkAutoloaderfrom being auto-loaded, we can hopefully avoid the fatal error from triggering.Fixes #1673.
To test, we can perform the following actions:
Before:
After: