Hi. I really liked Sentry since the first time I saw it some years ago. All of its features along with it being a Free and Open Source Software make it awesome!
It took to me almost two years to convince the company I work to change from Bugsnag to Sentry and one of the last problems we had with they were bad handling of Pull Requests.
So, it was a surprise to me opening a PR requesting the change of only one character, in order to make a deprecated and unused value to be marked as nullable, and see a rude and tough response. It seemed to me that it was all about a discussing between personal values (disguised as good practices) vs a pragmatical need. Along with some people that I didn’t even know if they could help, as they are not in the Sentry’s GitHub organization.
I know some nice people that have developed Sentry and I can’t imagine them making this fuzz because of a so simple change while creating falsifiable arguments that also can be simply fixed, like passing through a null value (which could be handled with a few more characters and no side-effects) or which part of a short argument is wrong.
Well, by the end I want to leave this quote that says the main concern about it all: “People may forget what you say. They may forget what you did, but people will never forget how you made them feel.”
I’m sorry you feel mistreated. Upon reading the issue you have referred (not a PR), I have failed to arrive at the same conclusions you have though. Even if this seems like a single-character change, it is an important change regarding typing and API interface and the scrutiny you face seemed appropriate for the potential implications and enforcing type-safety.
On GitHub UI, these people are clearly marked as “Collaborator”. Per the nature of open-source projects like Sentry, there are many people helping with the code and some of our SDKs are mainly driven by the community instead of Sentry employees. I don’t see what the problem with this is.
Thanks a lot for both reporting the issue and coming here to share your experience
Hope you get the issue resolved and keep helping make Sentry better!
The type hinting Sentry SDK is doing is not followed by any of other big PHP projects like symfony, laravel, codeception or whoops[1]. I would say they simply don’t care about it, as it is a deprecated parameter and type hinting is still a new thing in the PHP world.
The change was on a critical piece of software that handles exceptions on my application. As you may know, this is the barest code, the final code, that will handle all errors in my application and it is stopping the application.
In the case you want to be a good guy and pass only the valid value, you can simply do $errcontext ?? []; and it is all done.
On GitHub UI, these people are clearly marked as “Collaborator”. Per the nature of open-source projects like Sentry, there are many people helping with the code and some of our SDKs are mainly driven by the community instead of Sentry employees. I don’t see what the problem with this is.
Do I need to talk to all collaborators to get a change approved? Collaborators can merge into master? Does the repository have a CONTRIBUTING file stating how does the merge process occur? From my point of view, I was only talking to people who made the change which broke my software, explaining to them how that unprecedented piece of code needed to be changed. I had a big problem and no one to understand it.
Notes:
[1] I got all the references to the source codes but this platform limits my post to only two links, so here is the complete post.
I don’t think this point is relevant. Others not doing type hinting and it being a new thing in the PHP world is not a valid reason to not do it in Sentry SDK repo.
I also fail to see how this is relevant as from the issue it looks like a cross-compatibility issue with Whoops. Also, since the code is fully open, there shouldn’t be anything from stopping you from forking the repo, making your change, proposing the PR with clear reasoning and then using the fork in the meantime to unblock yourself.
This is correct but at no point I see this mentioned in the issue as a potential solution. I also see a point for sticking to the official documentation which did not indicate the parameter was nullable as mentioned in the issue.
I don’t think so. Getting one approval and no rejections is usually the way to go.
The repo does not have a CONTRIBUTING file and you make an excellent point. We should have one and document our process to make it easier and more welcoming to contribute! Will take note of this and try to get it addressed ASAP.
I can definitely relate to and understand your frustration but in all honestly, I did not understand this was blocking you or suddenly broke something that was working before. Maybe you can be clearer next time about why this change is important as all I’ve seen was trying to say the change is needed because it was easy/small/not widely adopted etc. which are not strong technical reasons. They can only be contributing factors for a core need such as
This is breaking Whoops, a widely used library and cannot be fixed upstream because [blah]. For this reason, I strongly recommend making this field nullable for now (trying to find the middle-ground) and do something like $errcontext ?? [];. We may even print a warning log to trace or signal when $errcontext is null to see where it comes from.
Hope this helps and hoping to see you keep contributing to the repo!
I don’t think this point is relevant. Others not doing type hinting and it being a new thing in the PHP world is not a valid reason to not do it in Sentry SDK repo.
Well, I will give up, otherwise, we can continue to create arguments to support everything we can think about and invalidating others arguments with “I don’t think so”. This is the funny thing about software: it is mainly about what a person thinks it’s right.
