Scroll down to the script below, click on any sentence (including terminal blocks!) to jump to that spot in the video!Cool, got it! Show me the script!
We just reviewed our first pull request! So let's see what other trouble we can get into! One of the most important thing you can do is to triage issues.
If an issue is for a feature request, then it's probably a discussion about whether or not it's a good idea and the best implementation. Helping those discussions is great. But... click on the "bug" label. These are where you can really help.
Symfony is pretty stable & complex. So, if there is a bug, it's usually pretty complicated or involves some edge-case situation. These can take a lot of time to understand and replicate. The core team really needs help from the community to verify the bug, ask for more information from the user and, ultimately, to create a "reproducer": a tiny app that clearly shows that bug in action.
Let's triage an issue I found a few days ago: #27901. Ok, the user says that he got an error when trying to serialize a Doctrine QueryBuilder with the web profiler: something about not being able to serialize a PDO instance.
The web profiler works by collecting a bunch of information about the request and serializing it to a cache file. It looks like something failed during that process. This is actually a pretty nice bug report because he lists the steps to reproduce: install the web profiler and then call
execUpdate on Doctrine's lower-level
Connection object, passing it a QueryBuilder. He even suggests a solution!
Ok, so, how can we help? First: see if we can reproduce & understand the issue. Let's create another small project for this. Notice that this is an issue that's reported on the stable version of Symfony: 4.1. So, we should create a new app based on that same version - not
dev-master like before.
C to stop the server, move back up to the top
contributing directory and run:
composer create-project symfony/skeleton triage_issue_27901
Because we're not specifying a version, it will use the current stable version: 4.1.
When this finishes, move into the directory. To replicate this bug, we will at least need to install the stuff he's using: Doctrine and the web profiler. Back at the terminal, just install Doctrine for now so we can write some code:
composer require orm
And... done! Close the old directory and open this new one. Then, go straight to create a new controller class: how about
Issue27901Controller. Give it a
public function test():
|... lines 1 - 2|
Ok: check back on the issue. He's using the Doctrine Connection object - a lower-level object I don't use too often. To see out how to get it, find your terminal and run:
php bin/console debug:autowiring
and scroll up. Yep! It looks like we can type-hint a
Connection class to get the service we need. Do that:
|... lines 1 - 2|
|... lines 11 - 12|
Next, he calls
execUpdate() and passes it a
QueryBuilder argument. You may already be familiar with the
QueryBuilder from Doctrine. Well, in this case, because we're using the lower-level
Connection class from the Doctrine DBAL library, the
QueryBuilder is also a lower-level class from that library.
These are the types of little details that can make triaging a bug tough! But, it's also part of the fun: you'll need to really dig into the code to find out what's going on.
Create the QueryBuilder with
|... lines 1 - 6|
|... line 12|
I won't even do anything with it yet: we're still investigating. Next, he calls
execUpdate(). Oh, but that doesn't exist! I bet he meant
executeUpdate() - pass that
|... lines 1 - 6|
Great! At this point, I would normally install the web profiler, create some database entities and use a real query in the controller to see if we can replicate the error. But, before we do that, I noticed something: the first argument looks like it's supposed to be a string! Hold
Ctrl and click to open the
Yep! The first argument should be a string! But, the user is passing a
QueryBuilder object! In other words, I don't think this is a bug! The only reason the user's query actually works is that, if you open the
QueryBuilder class, it has a
__toString() method. Doctrine is probably accidentally converting this object to a string and using that SQL.
This is why his possible solution is to, inside a related class, convert the
sql - which is a
QueryBuilder in his case - into a string. That would fix things, but I don't think this is really a bug.
But even still, it's awesome that the user opened this issue. In a lot of cases, even if there is no bug, we can use the mistake to improve things, like with better error messages.
So, let's reply! And give as many specific reasons why we think this might not be a bug: in this case, that the first argument expects a string.
As extra credit, I'll link to this exact code. Go to the
doctrine/dbal repository. Then, press the letter
t to open this search screen. I live by this shortcut. Look for
Connection.php and open it. Search for
executeUpdate() and... click to select that line: this updates the URL to point here.
Then - here's another trick - press the
y key. This changes the URL from
master to the actual commit sha. This helps make sure that this link - to line 1068 - will forever point to the line we want - even if someone makes changes on the
master branch and moves this line.
I'll paste the link and add a few more details. I really try to be as friendly as possible: this is our chance to help make Symfony a warm & welcoming community. Even if this is not a valid issue, it's great the user took their time to help report it.
And... boom! You probably won't have the power to close the issue, but this should make it easy for someone else to do that. Achievement unlocked!
This bug turned out to not be a bug. So, let's hunt for a bug that really is a bug. And learn how to create and share a "reproducer" project... which is seriously almost as valuable as actually fixing the bug.