Skip to content

Conversation

stayallive
Copy link
Collaborator

From #1462, we decided it might be better to pin the dependency to a non-problematic version for the time being (php-http/discovery#213).

@stayallive stayallive requested a review from cleptric February 9, 2023 16:16
@stayallive stayallive force-pushed the feature/fix-php-http-discovery branch from 67c9ed1 to 71807da Compare February 9, 2023 16:17
@stayallive stayallive force-pushed the feature/fix-php-http-discovery branch from 4d5b648 to 99b6648 Compare February 9, 2023 16:25
@stayallive stayallive changed the title feat: Add enableTracing Option (#1458) fix: pin php-http/discovery to < 1.15 Feb 9, 2023
@X-Coder264
Copy link

I wouldn't do that, the only actual issue that 1.15 caused already has a bug fix PR opened and is probably gonna be merged/tagged soon -> php-http/discovery#214

@cleptric
Copy link
Member

cleptric commented Feb 9, 2023

I don't want to prompt our users with php-http/discovery contains a Composer plugin which is currently not in your allow-plugins config when they update their dependencies. Before we ship #1459, there are currently no benefits of using 1.15.

@cleptric cleptric merged commit 70c9904 into master Feb 9, 2023
@cleptric cleptric deleted the feature/fix-php-http-discovery branch February 9, 2023 16:41
@nicolas-grekas
Copy link
Contributor

nicolas-grekas commented Feb 9, 2023

I wouldn't have merged this because it's not sentry's responsibility to force an upper bound to another dep.
But anyway, #1459 should be the next step so 🤷
See php-http/discovery#213 (comment) also

@HypeMC
Copy link
Contributor

HypeMC commented Feb 9, 2023

Won't this block any project using sentry from using v1.15. Sentry is not the only library that uses php-http/discovery, there might be libraries/projects that can already benefit from the new version, but now they're blocked from doing so.

@mfb
Copy link
Contributor

mfb commented Feb 9, 2023

A new plugin feels like a BC break for library maintainers such as myself. E.g., my library depends on sentry/sdk and is in turn used by thousands of actual end-user projects. I obviously don't have a way to enable/disable plugins in my end users' projects. I could update my README and roll a new release to attempt to educate users about how they suddenly need to enable or disable the new plugin, but that still feels like a BC break.

@nicolas-grekas
Copy link
Contributor

@mfb please have a look at the discussion in php-http/discovery#213 it might give a few useful hint about the topic.

@mfb
Copy link
Contributor

mfb commented Feb 9, 2023

@nicolas-grekas I've read that but seems like the takeaway is simply that the plugin needs to be disabled or enabled. And as a library maintainer, I cannot disable or enable the plugin, all I can do is ask the end users to disable or enable the plugin for their projects. I'm happy to add that instruction to my README, but I also wanted to point out that ideally, users wouldn't see such breakage happen without a major release somewhere in the chain of dependencies.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants