Here's the question now: how can we make our test app use the pull request code from the symfony/ directory? Check out the vendor/symfony directory in the app: it's just a bunch of sub-directories, each containing code from Symfony's master branch. But, what we really want is for this validator directory to instead be a symbolic link to the correct path in our symfony/ directory: src/Symfony/Component/Validator.
We could do this by hand... but! Symfony has a cool script to do this automatically: it's called link.
Go back to the terminal that holds the app and run:
ls -la vendor/symfony
Yep! Just a bunch of lonely directories. Go back to the other tab and run ./link and point to our app: ../triage_pr_28069:
./link ../triage_pr_28069/
Wow! Go back to your app and check the symfony directory again:
ls -la vendor/symfony
Awesome! Every package that comes from the symfony/symfony repository is now a symlink to our local copy! In other words, our app is now using Colin's code!
Testing the PR Code!
It's time for us to write some code that tests the new validator! In src/, create a new PHP class, um, how about: ClassToValidate:
| // ... lines 1 - 2 | |
| namespace App; | |
| // ... lines 4 - 6 | |
| class ClassToValidate | |
| { | |
| // ... lines 9 - 13 | |
| } |
I'm feeling creative!
Inside add a new public property called $enteredNumber:
| // ... lines 1 - 2 | |
| namespace App; | |
| // ... lines 4 - 6 | |
| class ClassToValidate | |
| { | |
| // ... lines 9 - 12 | |
| public $enteredNumber; | |
| } |
I'm trying to keep my code as simple as possible: a public property is a nice shortcut.
Next, add the annotation: @Assert\MultipleOf() of 10. I'm also going to add a second annotation that will eventually fail - @Assert\Blank() - just to make sure everything is working ok:
| // ... lines 1 - 2 | |
| namespace App; | |
| use Symfony\Component\Validator\Constraints as Assert; | |
| class ClassToValidate | |
| { | |
| /** | |
| * @Assert\MultipleOf(5) | |
| * @Assert\Blank() | |
| */ | |
| public $enteredNumber; | |
| } |
To try this, in the Controller/ directory, create a new class: TestingController. Fill in the namespace: App\Controller:
| // ... lines 1 - 2 | |
| namespace App\Controller; | |
| // ... lines 4 - 7 | |
| class TestingController | |
| { | |
| // ... lines 10 - 17 | |
| } |
Because we have multiple apps in one PhpStorm project, some of the magic we normally get isn't working. Inside this, add public function test(). Ok: to test validation we'll need the validator service and the object to validate. And an argument: ValidatorInterface $validator:
| // ... lines 1 - 2 | |
| namespace App\Controller; | |
| // ... lines 4 - 5 | |
| use Symfony\Component\Validator\Validator\ValidatorInterface; | |
| class TestingController | |
| { | |
| public function test(ValidatorInterface $validator) | |
| { | |
| // ... lines 12 - 16 | |
| } | |
| } |
Then, $myObject = new ClassToValidate() and set its enteredNumber to 10:
| // ... lines 1 - 2 | |
| namespace App\Controller; | |
| use App\ClassToValidate; | |
| use Symfony\Component\Validator\Validator\ValidatorInterface; | |
| class TestingController | |
| { | |
| public function test(ValidatorInterface $validator) | |
| { | |
| $myObject = new ClassToValidate(); | |
| $myObject->enteredNumber = 10; | |
| // ... lines 14 - 16 | |
| } | |
| } |
Oh, and change the MultipleOf to be 5:
| // ... lines 1 - 6 | |
| class ClassToValidate | |
| { | |
| /** | |
| * @Assert\MultipleOf(5) | |
| // ... line 11 | |
| */ | |
| public $enteredNumber; | |
| } |
We'll test that 10 is a multiple of 5.
To validate, add $errors = $validator->validate($myObject);. Then, dump that and die!
| // ... lines 1 - 7 | |
| class TestingController | |
| { | |
| public function test(ValidatorInterface $validator) | |
| { | |
| $myObject = new ClassToValidate(); | |
| $myObject->enteredNumber = 10; | |
| $errors = $validator->validate($myObject); | |
| var_dump($errors);die; | |
| } | |
| } |
Finally, we need a route for this! We don't have annotations installed, so, to keep things simple, add this in routes.yaml: uncomment the example, and change the controller to TestingController::test:
| index: | |
| path: / | |
| controller: App\Controller\TestingController::test |
Done! Back in the terminal, start the built-in PHP web server in the public/ directory:
php -S localhost:8000 -t public
We're ready! Find your browser, go to http://localhost:8000 and... what?! No validation errors! That's actually not good - the @Blank constraint should give us one error:
| // ... lines 1 - 6 | |
| class ClassToValidate | |
| { | |
| /** | |
| // ... line 10 | |
| * @Assert\Blank() | |
| */ | |
| public $enteredNumber; | |
| } |
The problem is that our setup is not quite complete: to use annotations with the validator, you need to install the annotations library. Stop the web server and run:
composer require annotations
This installs sensio/framework-extra-bundle... we only technically need to install doctrine/annotations. But, that's ok. Restart the built-in web server:
php -S localhost:8000 -t public
Move over and, refresh. It works! We get the one expected error from @Assert\Blank, but we do not get a second error: 10 is a multiple of 5. To make sure the failure works, change this to 9:
| // ... lines 1 - 7 | |
| class TestingController | |
| { | |
| public function test(ValidatorInterface $validator) | |
| { | |
| // ... line 12 | |
| $myObject->enteredNumber = 9; | |
| // ... lines 14 - 16 | |
| } | |
| } |
move over and... yes! There is the second error. That error language looks really nice to me.
Finishing the PR Review
Hey! The code works! This is great news! Let's go back to GitHub and tell the world! I'll "Approve" this pull request. And, like everything, don't worry: this doesn't mean that the PR is definitely perfect: just that you think it's ready. The really important part is to add as much information about why you think it's ready or not ready. In this case, we checked the code and we actually tested this in a real project.
And... approve! Oh, but I did forget to check one thing: whether or not this new feature has a documentation pull request or issue. But, of course, it does! Not every pull request needs documentation - but, if you think it does, and it's missing, gently poke the pull request's author. Or, even better! Go create the documentation pull request yourself! We'll do that later.
Oh, and fun fact! This feature was merged about 1 week after we did this review. Go open source! After some good community feedback, it was renamed from MultipleOf to DivisibleBy. A great change!
Next, let's triage an issue and try to close a bug.
1) Why do you clone the symfony/symfony repository? Couln't you just clone Colin's repository?
2) Why did you create the project from the dev-master version of the skeleton? Couldn't you use the last stable version?
Thank you