Sentry-php: [2.0] Ideas and proposals

[ste93cry]

Looking at the issues opened over the course of the years, I see that the most wanted things for a new version are:

• Switch to PSR-4 (#192, #414, #197)
• Use Composer for autoloading (#211, #414, #197)
• Drop PHP <5.6 support (#192, #197)

I think all of them are feasible only by working on a 2.x branch, because they all break BC. So, in addition to that I would like to propose the following things:

• Improve documentation by adding docblocks where needed (PSR-5)
• Refactor the processors system and introduce a ProcessorInterface interface
• Use Httplug to not having the need to maintain the transport clients
• Make use of factories or find a way to make things like processors and transports configurable and injectable (this way frameworks can create instances of processors and do custom things, actually this is not possible)
• Improve the unit tests
• Improve how the options are passed to the client by using the OptionsResolver Component by Symfony to validate them. The same could be done for the processors, thus making them configurable
• Refactor how the context of an event is gathered by introducing a way to retrieve the needed data from a set of collectors (see for reference bugsnag/bugsnag-php)

In general, I see the 2.0 version as a total rewrite of the current library that takes into account all modern features offered by PHP and makes it easy to extend and customize.

---

Some relevant issues by @stayallive so we (I) won't forget: #459

[nokitakaze]

Improve the unit tests

What do you mean? Do you want 90% code coverage?

I would also add scrutinizer to the project.

Improve documentation

All phpdoc comments are only needed for autocompleting and warnings in a IDE. Do you make auto documentation for your projects?

Component by Symfony

Do you want to loading symfony as dependencies to every project what requires Sentry? Do you REALLY want it?

[Taluu]

Use as default transport method Guzzle, which is battletested,, or alternately implement it along with a Curl handler and let users choose which they want to use

How about using Httplug instead ?

[ste93cry]

I did not know that library, this is why I talked about Guzzle. Honestly I have no experience with Httplug, but it seems a bit abandoned while instead Guzzle is a well-tested library that's used in many projects and we're sure it's not going away anytime soon. By the way, having a configurable transport system would mean that we can provide a few implementations of the clients that will send data to the server, or let users make their own based on their preferred library. We could just provide a basic CurlHandler like now, for example.

What do you mean? Do you want 90% code coverage?

I mean have unit tests for all new things, and improve the quality and code of the old ones

All phpdoc comments are only needed for autocompleting and warnings in a IDE. Do you make auto documentation for your projects?

I'm not sure I understand the las sentence, but a good project has a good documentation. Actually, this library lacks a lot of doc. And no, commenting the code is not only for autocompleting and warnings in a IDE, it's also for humans that look at the code and can understand what it does.

Do you want to loading symfony as dependencies to every project what requires Sentry? Do you REALLY want it?

That component can be used standalone, so it will only download that package and nothing more of Symfony. It provides a way to require options, set default values and validate the provided one so I don't see why we should not use it. But this is just a proposal issue, after all 😄

[nokitakaze]

I'm not sure I understand the las sentence

Have you ever made auto documentation for your projects?

commenting the code

I'm not talking about comments but phpdoc like

/**
 * @var double[] $values
 */

[ste93cry]

Have you ever made auto documentation for your projects?

Honestly, I don't know what you are talking about, so I think the anwer is no. I always write my doc myself, if this is what you're asking

I'm not talking about comments but phpdoc like

There are people that think that's useless, people that think it's not. I'm one of them, I think everything should be documented. Yes, indeed sometimes it's not necessary to document obvious things but I think that having _everything_ documented is a good practice and it won't hurt anyone doing it, and at least it's consistent with the other code

[nokitakaze]

I don't know what you are talking about

https://www.phpdoc.org/

phpDocumentor enables you to generate documentation from your PHP source code

[ste93cry]

Ok, then yes, I know phpdocumentor and I use it at work. But I still not get the point about what we're discussing here

[Taluu]

but [httplug] seems a bit abandoned

It's because it is stable actually (and very responsive too). It provides an adapter for guzzle, curl, and whatnot (you can even use your own implementation), as this is really an implementation of the http psr (psr-7 I think it was). A mock client is even provided if you need to test the lib without making actual requests. And it also even allow you to use a "discovery", so that the right client is picked (it checks which client is available, and use it), if you do not set a client.

The only drawback is that as it is fully decoupled, you have a shitload of vendors to install... but heh, it's worth it actually.

[ste93cry]

I think both works well, it's just that Guzzle is more mature (6.x vs 1.x) and it has more installs and GitHub stars

[Taluu]

Once again, it is an abstraction, so you can use the guzzle adapters (version 5, version 6)

This allows the developpers using sentry not to be locked out with guzzle actually (or a specific version). Otherwise, what happens is what is happening with the majority of api clients : a lot are still locked with guzzle 3, making it impossible to upgrade other vendors. Some choose to make their own curl abstraction, which is IMO too bad.

More over, guzzle is not really "stable", as it releases a major version every 5 months (check the delays between guzzle 4 and guzzle 6 (maybe 7 ? not sure))

[ste93cry]

I really think there are more chances someone is already using it instead of Httplug. Also, how could Sentry be locked with Guzzle if it's only a transport system of many? What I mean is that Guzzle, Curl, Httplug or whathever other client we are going to use will only be an adapter that users can switch. Do we really want an adapter of an adapter?

[Taluu]

If someone already has guzzle, then httplug will use it. What's the point of making and maintaining an abstraction when there's already one ? Especially one that respects PSR-7 for http messages. That is what I'm not getting.

Once again, httplug is an abstraction, not really a http client (it can't work if there are no http clients installed).

Basically, if the user wants to use guzzle, all he has to do is :

{
    "require": {
        "sentry/sentry": "^2.0",
        "php-http/guzzle6-adapter": "^1.0"
    }
}

That's for guzzle 6, guzzle 5 you just need to use the other package. In sentry, however, we just need to "discover" the client with the discovery client. This and using PSR-7

And then it's a full blown http client with the abtraction the developper implementing sentry chose, and in the sentry client, nothing has to be considered (no need to build an abstraction, no need to check if it is guzzle or whatnot, ... etc)

Okay, this needs the user to install one other library, but this seriously bring so much.

But then if you want to maintain the abstraction, the "adapters" as you call it, your call.

[ste93cry]

But then if you want to maintain the abstraction, the "adapters" as you call it, your call.

I think the less Sentry has to maintain the better it is. I get your point now, and I agree with you. Just to be sure that I understand all this correctly: if you don't specify an HttpClient instance, we could use the discovery client. If an user wants to create their own transport client (e.g. a customized Curl client) he just have to extend that class and provide an adapter for his client?

[Taluu]

He actually just has to implement the http client interface. In sentry, all you have to care about is building the psr-7 Request message, and making a send() call. You will then have a Response object (or a promise, in which you'll have the request and the response, or the error if there was one).

If the user wants to customise the curl client, he can extend the curl client or make one of his own.

So basically, it's what you understood. :}

[nokitakaze]

@dcramer should create new branch (i.e. feature/version2) then we can send pull requests to it instead of master branch. But he didn't say a word.

[ste93cry]

It would be better to name the branch 2.0, because this way we follow semver

[dcramer]

I've created a 2.0 branch. Regarding minimum versions, I want to make sure we're not excluding e.g. 30% of the audience by removing all of the older support.

[dcramer]

Aisde, I haven't had time to review this, but of note I'd prefer we not add external deps for trivial things. Specifically talking about the HTTP implementation here. Deps cause a lot of issues, and the more we can avoid them the better.

[nokitakaze]

I want to make sure we're not excluding

So tell us the oldest PHP version you want.

And we are not excluding anyone. The entire audience can use version 1.7

[ste93cry]

And we are not excluding anyone. The entire audience can use version 1.7

Exactly, there is no point in not supporting at least PHP 5.6 in 2.x. Anyone that wants to use older versions of PHP will continue to use Sentry <2.0

I'd prefer we not add external deps for trivial things.

I think the same, but we should not start by just excluding all deps just because we have fear of "deps cause a lot of issues". This is not true, at least if we use the right deps. We should not reinvent the weel when possible

[Taluu]

You should support PHP7+ onwards, as this is the current stable IMO. 5.6 is about to die (it is just in security fix), and the other ones are EOL'd since a long time already.

As it was said, people can use 1.x if they want older php releases.

On which 30% are you basing yourself on ? The composer stats ? These are really more indicative than the wordpress stats (after all, a lot of wordpresses are forgotten and not updated, and still stuck on 5.1s, while those using composer (actual active users I think ?) are 5.6 / 7.0+). Moreover, the link is only on last november, so when jordi will do another recap (in a few months I guess), I'm pretty sure it will have changed towards php 7.

[ste93cry]

You should support PHP7+ onwards, as this is the current stable IMO. 5.6 is about to die (it is just in security fix), and the other ones are EOL'd since a long time already.

While this is true, PHP 5.6 is still used too much to drop support for it. Even frameworks still support it, take for example Symfony 3: it's support is PHP 5.5+. There is no reason to not support it even in 2.x, but as said before I agree on dropping support for older versions

[Taluu]

Well, Symfony 4, which is planned for the end of the year, will support only PHP7+.

[ste93cry]

Sure, but as you said it will be released at the end of the year. I hope we release 2.0 way before 😄

[Taluu]

Maybe waiting may / june for a new edition of jordi's composer stats would be beneficial then, to see if anything changed (https://news.ycombinator.com/item?id=13797042)

[ste93cry]

Even if we would do that, we can't use PHP7 features only and then (maybe) find that PHP 5.6 is still widely used. I would instead start the work on 2.x by supporting PHP 5.6+, and if it will be the case to switch to PHP 7+ only then we are free to do it later.

[dcramer]

Just to avoid going too deep here, Sentry is not like a traditional library. Just saying "they can use an old version of the library" isnt enough, as we expand features over time. We dont want to backport these features, and if 80% of the audience is not using PHP 7, then we have to support older versions. We should make the decision based on marketshare, and I'm ok with deferring to WordPress' stats on it. If we determine that 90% of the audience is at least on 5.6, then we can drop older support and say "well unfortunately, you'll need to upgrade PHP or use this older, slightly less functional version".

[nokitakaze]

as we expand features over time

I think you mean situations when Sentry Server gets new features. It only means we sometimes need "to backport these features".
There is no big problem to support two version at the same time. But version 1.7 will get only "security updates". Everybody else does the same thing

There is only one reason PHP 5.5 holds 18.9% of the market. Because previous Ubuntu LTS (14.04) support only PHP 5.5. To support 5.6 you need to add packages of @oerdnj

[nokitakaze]

Okay, #414 was merged. What's the next? We need to choose minimal version we want to support. So, David? 5.5 or 5.6?

[stayallive]

Looking at the WordPress statistics it would mean:

• >=5.5 supports 58.7% of the userbase
• >=5.6 supports 45.1% of the userbase

Looking at the latest composer statistics it would mean:

• >=5.5 supports 92.9% of the userbase
• >=5.6 supports 73.9% of the userbase

I would personally opt for >=5.6 since that is the only supported version right now and by the time 2.0 is ready we are probably over a year outside the security support for 5.5.

Going on the WordPress dataset might not be a good idea since lot's of shared hosts are running outdated php versions, and people with shared hosts probably won't use Sentry but do run WordPress :) Also Sentry doesn't have an official WordPress plugin so... 💃

[nokitakaze]

By the way
https://github.com/getsentry/sentry-php/pull/440#discussion_r109283165

David, we still need your opinion. 5.5 or 5.6?

[Taluu]

Just posting this here : https://kinsta.com/blog/php-versions/

[nokitakaze]

I think we should fix some things which exist because of back compatibility.

• Protected properties should begin with underscore.
• Public properties should not begin with underscore.
• All properties in \Raven\Client (and other class) should be protected if they are not directly touched in other classes in Raven package and there is no needs to be changed after construction complete. I did all of them public because they weren't described at that time and by default all properties are public. Done in #457

[nokitakaze]

We need to choose code style so codebase will be uniformity.

function typical_method($every_param='foo', $described_with_unique = 'style',$good=false)
{
    return 'this' . 'is'.' brilliant, isn\'t it?';
}

If I do Ctrl+Alt+I in my Idea, I get so much blue lines so I didn't understand if there uniform code style at all.

[ste93cry]

Protected properties should begin with underscore.

Why should protected properties begin with an underscore? It's better to just stick with once convention for all properties regarding of their accessor modifier, and it's not so common to see this type of naming strategy

[nokitakaze]

@dcramer could you do fixed build in 2.0 with current commit f680436 in packagist?

I want to use it (Sentry 2.0-dev) in my projects so I could see what's missing

[dcramer]

@nokitakaze im not sure what you mean?

[nokitakaze]

I want to install (via composer) one specific version. I.e. 2.0.1-dev or 2.0.0-dev. I want to be sure composer will install fixed version (specific commit).

I do not want to install different versions each time. Because I want to be sure about behaviour of the package

[ste93cry]

A tag should be created only when we're ready to ship a release (be alpha, beta, rc, stable or whatever else). If you want to install a specific version of the project, you can always use the commit hash in the constraint of Composer ("sentry/sentry": "dev-master#<hash here>")

[nokitakaze]

you can always use the commit hash

Oh, I didn't know that. Thank you

[Taluu]

Here, Jordi Boggiano just released a new usage stats : https://seld.be/notes/php-versions-stats-2017-1-edition

It seems that +50% are using at least php 7.0.

[nokitakaze]

We need to choose code style

Maybe PSR-12? But as I can see PHP-CS-Fixer does not support it

[nokitakaze]

@dcramer, you should decide could 10% of market use old version (1.7.0) or they can't. I think they can. So we can remove version 5.5 from Sentry 2.0

[nokitakaze]

Why do we need uopz?
https://travis-ci.org/nokitakaze/sentry-php/builds/232481776

[Jean85]

I endorse the dropping of PHP 5.5.

In the matter of Httplug, it would be nice to use that, since Guzzle would be anyway a new dependency, and it would ease adoption.
With an example, my current project uses Guzzle 5. If we would support Guzzle directly, we would either choose to support Guzzle 6, or both 5 and 6, but with additional work on our side. Instead, using HttPlug we would be able to support both and even other clients, using a single abstraction.

The only drawback that I see on HttPlug would be if the abstraction is too "generic" for our usage, and it would impede our work. But I never used that, so I'm not sure about this.

[nokitakaze]

uopz

6123643 . Ooookay. I don't even want to know what is this. Lets just delete it.
Okay, #463

[nokitakaze]

As I saw while I worked on PR #466 we must fix all our documentation. It is still uses old Raven_
I have fixed some in d8e2bbd, but I don't know if I do this correctly

[nokitakaze]

Should we add

/.travis.yml export-ignore
/.scrutinizer.yml export-ignore
/.php_cs export-ignore

to .gitattributes?

[nokitakaze]

Gource for our sentry-php (branch 2.0 till 7ba43781): https://www.youtube.com/watch?v=wXGGizgZxsk

[nokitakaze]

Should we delete Makefile?

[dcramer]

Why would you remove the Makefile? it's a generic/accessible way for people to understand how to do various tasks.

I won't accept any PRs that remove it, as maintaining repos in different languages that do things completely differently is a huge knowledge burden without some standards

[nokitakaze]

First of all we don't support it. Maybe we can change all commands in it to composers call? Like

cs-dry-run:
    composer phpcs

test: cs-dry-run
    composer tests

?

[dcramer]

IMO it should just be shortcuts to composer. The idea is that in any of our projects you can do something akin to make to set the project up, and make test to run the tests.

[nokitakaze]

There is anything in Sentry Server what PHP Client doesn't support, but may support and so may send to server? Something like breadcrumbs.

We should add all of it until we mark first commit from 2.0-dev as RC.

[dcramer]

@nokitakaze we are working on a feature grid and going to overhaul SDK specifications as well as generate TODOs for the core missing items. Most of the major things exist in PHP already. The ones missing are edge-case things like caching reports to disk so that it can be used in offline mode.

[dcramer]

FYI all, I'm going to defer to @stayallive on most things here.

I'll just reiterate that my goal is still as much compatibility as we can afford, but e.g. wordpress users might not really be using Sentry anyways. I'd say ideally, if we hit an issue with 2.0 where everyone can't use it because of 5.5, we can re-introduce compatibility (i dont know how hard that would be ).

[Jean85]

If code coverage is optimal, reintroducing 5.5 later shouldn't be an issue. Our deps are not strongly tied to 5.6.

So :+1: for 5.6 for me!

[stayallive]

Let's go for >=5.6.

The 1.x versions are perfectly fine for <5.6 and people need to upgrade some time... 😅

[Jean85]

While reviewing #473, I've found a protected static function uuid4() in the Client. Shouldn't we ditch that, in favor of an external lib? ramsey/uuid is more than stable and wildly used.

[ste93cry]

Indeed, I already have another PR ready to submit that introduces a dependency on ramsey/uuid and refactor the events handling by introducing an object-oriented approach

[nokitakaze]

https://blog.travis-ci.com/2017-08-31-trusty-as-default-status
We should release 2.0 before March 2018.

end support for dist: precise | Sunday, April 1 2018

No precise — no PHP 5.3 tests.

[nokitakaze]

Okay, anything other than #472 waits for completion? Both my PRs (#454 & #457) could be discarded, so we need only one pull request to complete our goal and release 2.0, right?

[Jean85]

We can use the milestone to track progress: https://github.com/getsentry/sentry-php/milestone/1

[nokitakaze]

end support for precise: Sunday, April 1 2018

We still have to hurry.

[Jean85]

We should publish a "support policy" to say that older PHP versions could not get full support from us because of that.

BTW I think that this issue has become too noisy, maybe we should leverage the milestone to follow what's missing, and open dedicated issue marked with the milestone.

[ste93cry]

Since there are multiple issues opened for 2.0, I'm going to close this issue as the API is almost pretty stable now and I don't expect any major change.