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

Remove sdk #246

Closed
garak opened this issue Sep 25, 2019 · 20 comments
Closed

Remove sdk #246

garak opened this issue Sep 25, 2019 · 20 comments

Comments

@garak
Copy link
Contributor

garak commented Sep 25, 2019

I think that #190 should be reverted.
It's restricting choices of http client, defeating the purpose of PSR-18.
The benefit of reducing a supposed "complex" installation is not justified

@Jean85
Copy link
Collaborator

Jean85 commented Sep 25, 2019

The sentry/sdk package is a way to have a simple, out-of-the-box installation where the end user can just require a single package and have all running. You can easily require another transport by requiring the appropriate package and set it as a transport or, if you wish, you can override the sentry/sdk package like this in your composer.json:

{
    "require": {
        "guzzlehttp/psr7": "^1.6",
        "php-http/guzzle6-adapter": "^2.0",
        "sentry/sentry": "^2.1",
        "sentry/sentry-symfony": "^3.1"
    },
    "replace": {
        "sentry/sdk": "*"
    }
}

This is how I do it in my projects to use Guzzle.

@JanMikes
Copy link
Contributor

Hello! @Jean85 if i understand, your point is that you can quickstart with sentry in your symfony project with just composer require sentry/sentry-symfony vs requirement of installing multiple packages of your choice, etc composer require sentry/sentry-symfony php-http/curl-client guzzlehttp/psr7.

In that case i agree with you, stick to the sentry/sentry-symfony package is the only dependency you need to have it fully working in your project (it is same if you depend only on sentry/sentry it will probably not be enough.)

Though

"replace": {
        "sentry/sdk": "*"
    }

could be better documented at least in readme.md as it is not as straightforward as it might look like, there is mention about custom transports, but not about how to replace default sentry/sdk.

@Jean85
Copy link
Collaborator

Jean85 commented Sep 29, 2019

Sure. PR are welcome to fix that, then.

@Jean85
Copy link
Collaborator

Jean85 commented Oct 7, 2019

BTW I would like to point out that sentry/sdk will suggest Guzzle as the default HTTP client in the future: getsentry/sentry-php-sdk#4

@garak
Copy link
Contributor Author

garak commented Oct 7, 2019

I hope that in the future this bundle will require symfony/http-client (unless we can remove any hard dependency completely, that is IMHO the best solution). At least, it's more consistent with the Symfony ecosytem

@temp
Copy link

temp commented Nov 27, 2019

Maybe a Transport that uses symfony/http-client instead of httplug could be bundled in the sentry-symfony bundle? Could be optional, but it would replace a lot of dependencies...

@Jean85
Copy link
Collaborator

Jean85 commented Nov 28, 2019

Using symfony/http-client is more than feasible, but it can't be done on the bundle's side, it's up to the user to choose the preferred transport.

@temp
Copy link

temp commented Nov 28, 2019

That's why I wrote it could be optional. I don't say it should be required hard, but as a supported dependency. If symfony/http-client is installed, a special transport could be used.

@Jean85
Copy link
Collaborator

Jean85 commented Nov 28, 2019

That's already how HTTPlug works, which is the underlying lib used for HTTP transport in sentry/sentry. The only point is that you need to override sentry/sdk as said before, and require the right adapter to make autodiscovery work. Or, otherwise, set the client manually in the Sentry\ClientBuilder.

@temp
Copy link

temp commented Nov 28, 2019

Tried it with symfony/http-client (HttplugClient) and nyholm/psr7 last night, and it didn't work. Wasn't able to find the cause, will try again soon.

@garak
Copy link
Contributor Author

garak commented Dec 11, 2019

Same here.
I get No HTTPlug async clients found. Make sure to install a package providing "php-http/async-client-implementation" exception, even if symfony/http-client is declared as an implementation.

Do I need to require something else?

@garak
Copy link
Contributor Author

garak commented Dec 11, 2019

Self-reply: I managed to get it working by requiring php-http/discovery:^1.7@dev

@nicolas-grekas
Copy link

nicolas-grekas commented Jan 21, 2020

sentry/sdk is a metapackage that has a suggested transport, you can replace it

Wow, that's totally unexpected. Shouldn't it be the other way around?
The metapackage is what people should install directly when they want many deps in one batch. Then, people that want more granular deps could install deps by deps as they fit (the flex recipe should be bound to individual packages of course, not to the pack.)

@Jean85
Copy link
Collaborator

Jean85 commented Jan 21, 2020

That's what's happening for the base SDK, you install the metapackage and you get sentry/sentry and all the others. This bundle has to rely on the metapackage to have everything and THEN integrate with Symfony.

@B-Galati
Copy link

Other alternative is to not use the bundle and do the setup manually like so @garak:

https://github.com/B-Galati/monolog-sentry-handler/blob/master/doc/guide-symfony.md

@garak
Copy link
Contributor Author

garak commented Mar 20, 2020

I created an alternate metapackage: https://packagist.org/packages/pugx/sentry-sdk
Can we mention it in docs, as an alternate solution? I can provide a PR

@Jean85
Copy link
Collaborator

Jean85 commented Mar 20, 2020

Sure!

garak added a commit to garak/sentry-symfony that referenced this issue Mar 22, 2020
Jean85 pushed a commit that referenced this issue Mar 23, 2020
@Jean85
Copy link
Collaborator

Jean85 commented Mar 23, 2020

Solved in #327

@Jean85 Jean85 closed this as completed Mar 23, 2020
@gkawka
Copy link

gkawka commented Apr 12, 2021

@Jean85 are you sure it works properly? I have lib with dependency guzzlehttp/psr7 <1.7 and because of dep "guzzlehttp/psr7": "^1.7", in sentry/sentry I cannot use pugx/sentry-sdk or sentry at all (need one of latest releases because of using php8).

@Jean85
Copy link
Collaborator

Jean85 commented Apr 12, 2021

@gkawka if you have a constraint that forbids you from updating a package, that's something that we can't fix on our side. Please open a separate issue if you want something else addressed.

BTW now sentry-sdk uses the Symfony client, so you can ditch pugx/sentry-sdk and go back to the standard installation.

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

No branches or pull requests

7 participants