Replace usage of deprecated $canonicalize phpunit parameter #238
Conversation
| @@ -26,7 +30,7 @@ public function testAllergiesToOneAllergen($allergicTo) | |||
| $otherAllergen = array_filter(Allergen::allergenList(), function ($allergen) use ($allergicTo) { | |||
| return $allergen != $allergicTo; | |||
| }); | |||
| $self = $this; | |||
| $self = $this; | |||
| array_map(function ($allergen) use ($allergies, $self) { | |||
| $self->assertFalse($allergies->isAllergicTo($allergen)); | |||
There was a problem hiding this comment.
hmm this is weird, passing $self through here, not sure why it isn't just $this->assertFalse() inside the closure 🤔
There was a problem hiding this comment.
Prior to PHP 5.4 $this wasn't bound automatically inside anonymous functions, so it was possibly written with that in mind? The test was originally written in 2016, when 5.4 had only just reached it's end of life and keeping PHP up to date wasn't as common.
It should probably be removed now, however, as 5.4 has been EOL'd for four years.
There was a problem hiding this comment.
Let me know if you guys want me to remove this :)
Also: Hi Ross! :P
There was a problem hiding this comment.
I think it makes sense to remove it, as it's bad practice now and the tests are something students should be looking at while they're working through a problem.
Thoughts @petemcfarlane?
There was a problem hiding this comment.
If you guys want this change, could you please request it? (i.e review with "changes requested")
/cc @DerTee
|
@G-Rath Since the assertEqualsCanonicalize thing was fixed in a different PR: Could you rebase this PR on the current master and check what is left? |
|
Sure thing! |
…phpunit-parameter
|
@DerTee I've merged this branch with master - hopefully it's how you want it. I had a bit of trouble with rebasing & such (I'm still learning about working across forks), so let me know if there's any problems with it. Worse case, I can remake the branch by just cherry picking the commits out. |
|
@G-Rath No, a merge just clutters the history unnecessarily in this case. But a rebase with conflicts can be a major pain, that's true :) I think you are right, doing a cherry pick is probably the smarter move (maybe in a new PR if it's too much hassle to change this one, I don't know what's better). |
| @@ -26,7 +30,7 @@ public function testAllergiesToOneAllergen($allergicTo) | |||
| $otherAllergen = array_filter(Allergen::allergenList(), function ($allergen) use ($allergicTo) { | |||
| return $allergen != $allergicTo; | |||
| }); | |||
| $self = $this; | |||
| $self = $this; | |||
There was a problem hiding this comment.
Please can we remove this temporary variable $self, as it's no longer necessary
More deprecated things.
In other news, I found the PHPUnit issue on github 😃