Deprecate the methods on the HubInterface to get and set the current hub

[ste93cry]

Q	A
Branch?	2.2 (develop)
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	yes
License	MIT

As discussed in #845, putting the getCurrentHub and setCurrentHub on the HubInterface was an error. Based on what has been done in other SDKs (I used Python and JS as reference) I decided to deprecate them and move the handling of the current hub in a HubManager class that can be used as a service in a proper DI-based environment or through the global functions for those that prefers to use them. PR is not ready yet, but I would like an opinion on a few implementation details. In particular, I'm undecided on how the global function getHubManager should be written. The main thing I would like to support is the retrieval of the instance from a service container, so I had to pass a callback around that will be used as a sort of factory method to instantiate the singleton variable the first time it's accessed. Another thing I had to solve is that since we (especially in an application based on a framework) cannot be sure that the function won't be accessed before the service container is built and initialized I needed a way to force replacing the instance even if it was already set. I don't really like this because of all the bad things that could occur when replacing an instance when you shouldn't, but I couldn't came up with a better solution. Any idea, comment or feedback is greatly appreciated.

[ste93cry]

After looking at the implementation of several other SDKs I decided to follow the way of the .NET SDK and implement a singleton class with static methods only that will be the main entry point for all the SDK common features like capturing messages, errors, etc. To support getting the hub instance from a service container the users will need to register the instance of the HubAdapter class and then they will be able to typehint the HubInterface to avoid relying on the static class. I decided to deprecate the global functions even though we may still provide them, however they are just duplicate code of what the new SentrySdk class already provides. The integrations deserves a separate discussion on how to get the current hub instance in a flexible way: the .NET SDK differs from the standard setupOnce method used in all the other implementations and calls a Register method to which both the hub instance and the options are passed as arguments. At the moment I'm getting the hub from the constructor but I'm not sure this is the best and recommended way as it implies that if the current hub gets changed hunder the hood then magically the extension will use the new one and this is something we may not want. @HazAT I would like your opinion on this PR as it's a pretty big change and you have more in-depth knowledge of the unified API and how it should work

[HazAT]

I don't fully understand the motivation of this change, it was fine before (aside from the two error integrations that were broken before).
It changes a lot of internal behaviour which is now inconsitent at times.

[ste93cry]

I don't fully understand the motivation of this change, it was fine before

The change originated because as reported by @B-Galati the singleton should be managed somewhere else: it's not up to the implementors of the HubInterface to handle it. This in turn was a problem because I would had to save the instance in a global variable or in a global function as local static variable, but in this case then it was not possible to set it without making a getter that could act as a setter too according to the number of parameters that it was invoked with. So, I took inspiration from the .NET SDK and just adapted what was done there deprecating the global functions and moving everything into a static class that acts as central point for the SDK. The HubAdapter is just a proxy to avoid depending on such class. The rest is just a refactoring to make everything work

It changes a lot of internal behaviour which is now inconsitent at times.

Indeed I didn't account for some things, I reverted the changes and I will address the found bugs not strictly related to this change in a separate PR so that we can ship the fix as soon as possible

[stayallive]

I don't like to have the deprecation warnings thrown around. As soon as we release this all Laravel, Symfony and people that have just ^2.0 in their composer.json will be bombarded with ErrorExceptions from their error tracking library. This to me is unacceptable. I would suggest to remove those in favor of @deprecated docs so we can prevent that from happening.

[ste93cry]

people that have just ^2.0 in their composer.json will be bombarded with ErrorExceptions from their error tracking library.

This is part of how deprecation works, even in PHP... If you use something that is deprecated then unless you suppress the errors you will get warnings and this is good because you may not even notice a docblock that marks a function as deprecated. Even frameworks and other libraries out of here throws such warnings and we did the same in the past. This PR should not throw any deprecation as far as I coded it right , but if someone used deprecated functions then he will get them.

[ste93cry]

@HazAT are you more willing to approve this PR if I don't deprecate the global functions? Even though I strongly disagree because we have duplicate ways to do the same things in this case users would still be able to use the same API as now as the change would be transparent for them, apart from Hub::getCurrent that would throw a deprecation (and there is no way around this as it's the subject of this PR). Otherwise I'm out as I don't have the desire to further discuss why this change is better than our current situation as it seems that the people to involve are not willing to change their mind

EDIT
After investigating, here the summary of exactly what others SDK are doing. I post this in the hope that you can see that every SDK is somehow different in this API and some SDK don't even have the concept of hubs, so there is absolutely no good reason to deny the merging of this change. The only thing that deserves to be discussed is the deprecation of the global functions which I already explained why it makes sense, but I'm more than welcome to keep them as-is if this makes you feel better.

	JS	PHP	Ruby	Java	.NET	Python	Rust
Hub::getCurrent		x				x	x
Hub::setCurrent		x
getCurrentHub	x
setCurrentHub	x
SentrySdk::getCurrent
SentrySdk::setCurrent					x

EDIT 2
I removed all the deprecations from the global functions but the Hub::getCurrent and Hub::setCurrent methods. This way we should be aligned with what other SDKs provide out-of-the-box in terms of Unified API, with the added SentrySdk static class which works as main entry point for everything now. Should in the future decide that we want to drop the global functions (I hope so as they are now just duplicate code) we will have to add the deprecation warnings again

[HazAT]

We had a lot of back and forth on this topic, most of the stuff we discussed was "offline".

I am happy with the current implementation, it's sound and makes testing easier.

Some additional requirements:

• Changelog is missing
• I want also want an update in sentry-laravel (potentially sentry-symfony) to not use the deprecated functions getCurrentHub/setCurrentHub. The version of the core php package and updates to laravel/symfony should go at the same time.

[Jean85]

@HazAT the update on sentry-symfony is doable, it would require a bump in the minimum version of the SDK. That would be useful for me because I was already thinking about doing a bump due to getsentry/sentry-symfony#236.

[Jean85]

@HazAT another question: the two integrations will need a bump but they rely on sentry-sdk... Should we tag the bump there or should we add the direct constraint in the integrations?

[ste93cry]

You could also just use class_exists to decide whether to use the Hub or SentrySdk class to get and set the current hub so that there is no need to bump the minimum required version of the main SDK. Or am I missing something?

[Jean85]

@ste93cry yes you could. @HazAT it's up to you then, you should decide which approach we should choose.

[HazAT]

I am for bumping/code change if possible.