And we're back after a two week break: eight days of annual leave (nice), but since rolling back to my desk in the cabin, I have been busy... this has been the first opportunity I've had to put down some notes... so here we go!
Upgrade all the Drupal things!
The first task I picked up was to upgrade a Drupal 11 site from 11.1.7 to the most recent version of Drupal, which at the time of writing, is 11.2.4. Not a huge leap, but a minor version bump no less.
There were a few issues, which is to be expected: I found two patches would no longer apply cleanly.
The fix? Easy! A quick check of the issue for the first patch confirmed that it had been rolled into a release and was now included in the version I had upgraded to. Sweet. And the issue for the other patch had been closed as won't fix, but with a note stating that the original issue had been fixed using a different approach.
Armed with this information, I was able to simply remove the patches from the composer-patches.json file. I love removing code... even patches. Oh, and shout out to Cameron Eagans and the composer-patches project!
The curious case of the failing test - part II
Everything was looking good with the upgrade. The next step was to run the test suite, which threw up two failed tests: one was simple to resolve, but the other one was quite challenging and interesting to fix.
As is sometimes the case, the test itself seemed fairly innocuous... it certainly wasn't doing anything complicated.
The problem, as PHPUnit saw it
The test failure presented as follows:
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'fish'
+'friends'Fair enough... I can agree with this much at least: those two strings are definitely not the same. We're on the same page.
Like a fart in a bus, my thoughts drifted around for a moment before forming into a suspicion.
- I'll start with the test code, I said to myself. When was it last changed? I found nothing.
- I moved to the system under test - in this case, a method in a class... had that changed recently? No, it had not.
- Okay, what about the upgrade? Had some new version of a contributed module, or even something in Drupal core changed? Is that the next most likely thing causing this test to fail?
I spy with my little eye, something that looks like a problem
Digging deeper into the code, right in the middle of the method being test, I spied a conditional branch. I was onto something.
Both branches returned an array, but the structure of each array was ever so slightly different. I took the code, left it to simmer over a low heat for thirty-minutes, and this is what it boiled down to...
if ($thing->someProperty === 'something') {
return $goodData; // Data, structured how the test expects.
} else {
return $stillGoodData; // Data, good data, but not structured how the test expects.
}This, as you can imagine, was a happy discovery, I was nearly done!
Hahahahaha. Nearly done.
But what has changed that would mean the execution path now goes down the branch we don't want it to?
Glancing at the test code, everything looked fine and dandy...
$thing = $this->getMockBuilder(Thing::class)
->disableOriginalConstructor()
->getMock();
$thing->expects($this->any())
->method('id')
->willReturn('my_thing');
$thing->someProperty = 'something';Especially that last line there. So how on earth can `$thing->someProperty` not evaluate to something during the test suite run?
I decided the only sensible thing to do at this point was to have a big sulk, eat some chocolate and reset everything back to Drupal 11.1.7.
Running the test with everything rolled back resulted in a splash of beautiful green on my screen, the test passed!
I decided the only sensible thing to do at this point was to binge watch all three series of Bluey, eat more chocolate and reset everything back to Drupal 11.2.4.
Not one for giving up, I rolled up my sleeves with earnest intent.
I pushed up my glasses and refilled my coffee cup. I took the dog for a short walk; had a chat with a neighbour about his recent month long visit to Hong Kong; made and ate a sandwich (with extra pickle, and I don't mean Branston pickle, I'm talking about that really expensive stuff in the tiny jar with the ribbon that you find in the quaint farm shop at the weekend, and because it's a quaint farm shop, and it's the weekend, you momentarily abandon all reason and happily tap your card for £16.25); had another coffee; took a bus into town and did some shopping (picked up a lovely reed diffuser), read five chapters of "To a God Unknown" and then got right down to business resolving this annoying, failing test.
A case of exploitation? A foul smell?
So here's what I discovered.
In the minor release of Drupal 11.2.0, the version of PHPUnit bumped up a major. PHPUnit, the development dependency which provides the framework for creating and running tests, had be upgraded from PHPUnit 10 to PHPUnit 11.
Something between those two versions affected how mocks behave: in short I found that with PHPUnit 11, you can no longer set a dynamic property on a mock after you have created it.
And after all, if you actually think about it, it makes perfect sense. For what is a mock but a test double that verifies behaviour, and what is a stub if not a test double that simply stands in the place of another? And what is the role of the constructor in object instantiation, but to initialise the object... to prepare it for use.
I've always frowned when I see dynamic properties being used... just because you can, doesn't mean you should... and by the way, they are deprecated as of PHP8.2.0.
So it followed, in my mind at least, that asking for a mock of an interface or object, and then expecting to be able to set a dynamic property on it, which, did I mention, is deprecated as of PHP8.2.0, shouldn't set your teeth on edge at all. But a stub, perhaps that's okay?
Regardless of any teeth grinding going on, it was possible for mocks in PHPUnit 10, but isn't in PHPUnit 11...
- Was it an exploitation of an unsupported feature perhaps?
- Something that worked, but shouldn't have?
- Maybe there's been a fundamental change to the way in which mock object generation is handled under the hood in versions of PHPUnit 11 and above?
I'm not sure, I haven't pinpointed it yet. Something to do with this commit, maybe.
A fresh perspective
Like a breath of fresh air, I had a fresh perspective on the problem, I was now able to approach it with clarity... what role is $thing playing? Well, in my case $thing was actually a Drupal View.
And what aspects of the View does the code being tested interact with?
Quite simple. Two things...
- the code expects to call
$view->id()and get back a stubbed response - the code expects to call
$view->current_displayand get back a stubbed response
Do I need to know if $view->id() is called?
No.
So why use a mock at all?
I went on the hunt for a suitable interface: EntityInterface implements public function id() but knows nothing of public $current_display. I finally landed on ViewExecutable which has both public function id() and public $current_display.
I rewrote the arrange portion of the test code, this is what I came up with:
$view = $this->createStub(ViewExecutable::class);
$view->method('id')->willReturn('my_thing');
$view->current_display = 'something';But still there's that annoying dynamic property assignment... but it's a stub. Perhaps it's acceptable. For today at least.
The test passed.
That was quite a journey.
Buses are awful.