Wednesday, 8 February 2017

PHP: more array dereferencing weirdness

G'day:
(Firstly: I hasten to add that the "weirdness" is "weird to me". It might be completely predictable to everyone else in the PHP world).

This is a follow-on from my earlier article "PHP: accessing an undefined array element yields a notice. Or does it? WTF, PHP?". Now before I start: Kalle Sommer Nielsen has come back to me in a comment on that article and mentions it's expected behaviour, and explained it... but I don't quite follow yet. I'm hoping some ensuing experimentation will cast some light on the scene.

My next variation of this has me scratching my head more. Well: it's demonstrating the same thing, but perhaps more clearly.

<?php

echo PHP_EOL . "Test with array" . PHP_EOL;

$a = [1, 2, 3];
var_dump($a[0]);
var_dump($a[0]['id']);
var_dump($a[0][0]['id']);
var_dump($a[0]['id'][0]);

echo PHP_EOL . "Test with int" . PHP_EOL;
$i = 1;
var_dump($i['id']);
var_dump($i[0]['id']);
var_dump($i['id'][0]);

echo PHP_EOL;

echo PHP_EOL . "Test with string" . PHP_EOL;
$s = '1';
var_dump($s['id']);
var_dump($s[0]['id']);
var_dump($s['id'][0]);

This yields:


C:\temp>php test.php

Test with array
C:\temp\test.php:6:
int(1)
C:\temp\test.php:7:
NULL
C:\temp\test.php:8:
NULL
C:\temp\test.php:9:
NULL

Test with int
C:\temp\test.php:13:
NULL
C:\temp\test.php:14:
NULL
C:\temp\test.php:15:
NULL


Test with string
PHP Warning:  Illegal string offset 'id' in C:\temp\test.php on line 21
PHP Stack trace:
PHP   1. {main}() C:\temp\test.php:0

Warning: Illegal string offset 'id' in C:\temp\test.php on line 21

Call Stack:
    0.0002     356384   1. {main}() C:\temp\test.php:0

C:\temp\test.php:21:
string(1) "1"
PHP Warning:  Illegal string offset 'id' in C:\temp\test.php on line 22
PHP Stack trace:
PHP   1. {main}() C:\temp\test.php:0

Warning: Illegal string offset 'id' in C:\temp\test.php on line 22

Call Stack:
    0.0002     356384   1. {main}() C:\temp\test.php:0

C:\temp\test.php:22:
string(1) "1"
PHP Warning:  Illegal string offset 'id' in C:\temp\test.php on line 23
PHP Stack trace:
PHP   1. {main}() C:\temp\test.php:0

Warning: Illegal string offset 'id' in C:\temp\test.php on line 23

Call Stack:
    0.0002     356384   1. {main}() C:\temp\test.php:0

C:\temp\test.php:23:
string(1) "1"

C:\temp>

OK, so here I factor-out the top-level array from the mix. That test was always referring to the first element of the array, which is just the integer 1, so we might as well just test with that. Needless to say, there's the same results as with the first array element. But I still don't get it. What is using array notation to dereference an integer supposedly doing? It's a non-sensical operation, as far as I understand things. And if it is a nonsensical operation: it should error.

For completeness I also try a variation with a string "1". This does error like I'd expect. Well: it gives a warning and then returns an inappropriate (IMO) result anyhow. This is probably PHP doing it's combo of telling me I did it wrong but then going ahead and guessing what I meant anyhow. Grrr.

I'm gonna mess around with de-referencing integers some more when I get a moment, and try to work out WTF.

Righto.

--
Adam

Tuesday, 7 February 2017

TDD: still testing too much

G'day:
A while back I wrote this article - Maintaining untested code, and mocking too much when backfilling test (with JavaScript & Jasmine) - wherein my colleague pulled me up for testing too much when I'm doing my TDD. I'm working with PHP this time, and I've just been pulled up on it again. For much the same reason. It created an interesting thought exercise... well interetsing ish... so I'll write it up here.

Firstly, I cannot use the actual code I am working with due to the usual over-baked "commercial sensitivity" considerations, so this is simply an analogy, but it captures the gist of what I was doing.

We have an adapter that handles HTTP calls. It's basically a generic wrapper that masks the API differences between Guzzle 5 and Guzzle 6, both of which we are still using, but we want a unified adapter to work with both. It's very generic, and basically has methods for GET, POST, PUT etc, and doesn't encompass any of our business logic.

Business reality is that we use this mostly to transmit JSONified objects, so we're adding a layer in front of it to take the raw text responses and rehydrate them. I'm writing the GET method.

When I start designing my solution, I know that the happy path will return me a Response object that has JSON as its content. I just need to deserialise the JSON and return it. This is easy enough:

public function testGetAsObject() {
    $expectedResult = ['result' => 'valid JSON'];
    $expectedContent = json_encode($expectedResult);

    $adapter = $this->mockAdapterWithResponse(200, $expectedContent);

    $client = new HttpClient($adapter);
    $result = $client->getWithJson($this->uri);

    $this->assertEquals($expectedResult, $result);
}

mockAdapterWithResponse just does this:

private function mockAdapterWithResponse($statusCode, $content){
    $response = $this->getMockBuilder(Response::class)
        ->setMethods(['getStatusCode', 'getContent'])
        ->getMock();
        
    $response->expects($this->once())
        ->method('getStatusCode')
        ->willReturn(statusCode);

    $response->expects($this->once())
        ->method('getContent')
        ->willReturn(content);

    $adapter = $this->getMockBuilder(Adapter::class)
        ->setMethods(['get'])
        ->getMock();
        
    $adapter->expects($this->once())
        ->method('get')
        ->willReturn($this->response);
        
    return $adapter;
}

(there are other tests that already needed this). All it's doing is enabling us to set what the content for the call to the HttpClient will return, because we don't care how the client goes about doing this; we just care what we do with the content afterwards.

So back to my test code:

public function testGetAsObject() {
    $expectedResult = ['result' => 'valid JSON'];
    $expectedContent = json_encode($expectedResult);

    $adapter = $this->mockAdapterWithResponse(200, $expectedContent);

    $client = new HttpClient($adapter);
    $result = $client->getWithJson($this->uri);

    $this->assertEquals($expectedResult, $result);
}

All I'm doing here is mocking the adapter to return a predetermined result, and I then test that what comes back from my function is the deserialisation of that value. That's it. I then implement some code to make that test pass:

public function getAsObject($uri, $params = [], $headers = []) {
    $response = $this->get($uri, $params, $headers);

    $object = json_decode($response->getContent(), true);

    return $object;
}

And that's green: all good.

I've already done too much in my test, but have not noticed (and not put it in for code review, so no-one's pointed it out).

The next requirement for this method is that if the content coming back from the adapter isn't JSON, then just throw an exception instead of trying to deserialise it. Again, pretty easy to TDD that:

/** @expectedException \Exception */
public function testGetAsObjectThrowsExceptionOnNonJsonContent() {
    $content = 'NOT_VALID_JSON';

    $adapter = $this->mockAdapterWithResponse(200, $content);

    $client = new HttpClient($adapter);
    $client->getWithJson($this->uri);
}

Note there's not even any explicit assertions in there: the @expectedException annotation takes care of it.

This test fails at present: no exception is thrown thanks to PHP's stupid-arse behaviour if json_decode is given non-JSON: it just returns null. Bravo, PHP. But that's what we want... we've designed our code and have tests to prove the design, now we need to implement the code:

public function getAsObject($uri, $params = [], $headers = []) {
    $response = $this->get($uri, $params, $headers);

    $object = json_decode($response->getContent(), true);
    
    if (is_null($object)) {
        throw new \Exception('Unable to decode JSON result');
    }
    return $object;
}

The tests are green, so all good.

But what have I done wrong? Nothing major, but once again I'm testing too much. Actually my tests are fine. It's my helper method that's "wrong":

private function mockAdapterWithResponse($statusCode, $content){
    $response = $this->getMockBuilder(Response::class)
        ->setMethods(['getStatusCode', 'getContent'])
        ->getMock();
        
    $response->expects($this->once())
        ->method('getStatusCode')
        ->willReturn(statusCode);

    $response->expects($this->once())
        ->method('getContent')
        ->willReturn(content);

    $adapter = $this->getMockBuilder(Adapter::class)
        ->setMethods(['get'])
        ->getMock();
        
    $adapter->expects($this->once())
        ->method('get')
        ->willReturn($this->response);
        
    return $adapter;
}

Why am I testing that those methods are called once? It is completely irrelevant to what I'm testing how many times they're called.

What got into my head is I was being too dogmatic when I was writing my code, and I also cheated a bit with the TDD. Instead of designing the test variation first, and then implemented the code to pass the test, I was doing both at the same time. And it shows from my test implementation. Basically I didn't design my solution first, I designed it as I went, so - whilst tricking myself into thinking I was doing TDD - I was asking myself "right, what does my code need to do? It needs to call get on the adapter. OK, so I'll test I call it. What next... I need to get the content from the response. Right: I'll make sure that getContent gets called". All very stolid and admirable, but: wrong. All this is doing is testing my code (almost line by line!). TDD is not about testing my code; it's about testing my design. And it's the design and behaviour of the API of the method I'm testing. As I've said before, it'd be excellent it TDD was retconned to mean "Test Driven Design", not "Test Driven Development". The former is closer to the intent of the exercise.

I don't need to test those methods are called once: there's no logic in my function that will vary how many times they might be called, so they will be called once. We can trust PHP to do what we tell it to do by giving it source code.

The logic flaw here was demonstrated by my code reviewer observed: if you think you need to check how many times that functionality is called, why are you also not concerned with how many times json_decode is called too? It's an important part of your functionality which definitely needs to be called. Ha... yeah "duh", I thought. This drove home I was testing too much.

Another thing is that I was getting too distracted by what my implementation was whilst writing my test. The requirements of that function are:

  • make a request, the response of which might be JSON:
  • if it is: return the deserialised content;
  • if it's not: raise an exception.

That first step is implementation detail from the POV of testing. What we're testing with TDD is the outcome of the function call. Which is: an object, or an exception. We're also only supposed to be testing the logic in the function the test is actually calling. I emphasise "logic" there because we're not testing the code. TDD is not about that. We're testing the logic. This is why we have two test variations: one wherein the logic to throw the exception is not true, so we get our object back; another when that if condition is true, so we get the exception.

We only need to test logic branching code (conditionals and loops, basically).

We do not need to prove every single statement is executed.

I was reminded a rule of thumb to apply when considering whether to use expects or not. Basically it boils down to if you have test variations which might result in a dependency being called a different amount of times, and that variation is germane to the result, then verify the call count.

Another good point made by my other colleague Brian was that when mocking stuff... only mock the bare minimum to get the test passing. Then stop. And that's probably the right place to leave it be.

An example of this might be if we had this logic in our method:

public function getAsObject($uri, $params = [], $headers = []) {
    $response = $this->get($uri, $params, $headers);

    $object = json_decode($response->getContent(), true);
    
    if (is_null($object)) {
        $this->logger("Bung content returned: $object");
        throw new \Exception('Unable to decode JSON result');
    }
    return $object;
}

Logging is an optional outcome of that method call, so we might want to test that the logger definitely gets called once in that second test. We could also be belt-and-braces about it and test it's not called at all in the happy-path test.

Or let's say the code was a caching proxy and one is testing the uncached and cached variations:

function get($id){
    if ($this->cache->exists($id)){
        return $this->cache->get($id);
    }
    return $this->dao->get($id);
}

In the uncached variation, the data is not found in the cache, so we want to make sure that the DAO is called to get the data from the DB. In the cached variation, we want to make sure the DAO isn't called. Even here, in the first example we're not really testing the DAO call was made specifically; we're testing the logic around the cache check didn't trigger the early return. We're testing the if statement, not the DAO call.

Bottom line, I think that if I find myself starting to do a call count on a mocked method, I need to stop to think about whether I will have cases where the expected call count will be different, and only then ought I even be bothering if the call-count variation is important to the outcome of the function call. If I'm only checking for one call count value... I'm doing it wrong.

And now rather than talking about my day job... I should actually be doing it...

Righto.

--
Adam

Monday, 30 January 2017

PHP: accessing an undefined array element yields a notice. Or does it? WTF, PHP?

G'day:
So we encountered this today:

<?php
$array = [1, 2, 3];

var_dump($array['id']); // this yields a warning
var_dump($array[0]['id']); // none of these do
var_dump($array[0][0]['id']);
var_dump($array[0]['id'][0]);

As indicated, this yields results:

C:\temp>php shite.php
PHP Notice:  Undefined index: id in C:\temp\shite.php on line 4
PHP Stack trace:
PHP   1. {main}() C:\temp\shite.php:0

Notice: Undefined index: id in C:\temp\shite.php on line 4

Call Stack:
    0.0002     353472   1. {main}() C:\temp\shite.php:0

C:\temp\shite.php:4:
NULL
C:\temp\shite.php:5:
NULL
C:\temp\shite.php:6:
NULL
C:\temp\shite.php:7:
NULL

C:\temp>

I think this behaviour is bloody daft. Accessing something that doesn't exist should be more than a notice, and it shouldn't then go ahead and return "null" anyhow. And giving a notice and returning null is possibly the worst way of combining possible actions here. What am I supposed to do with that? Other than bury my head in my hands a weep.

Now this is partially documented on the Arrays page of the docs:

Note:
Attempting to access an array key which has not been defined is the same as accessing any other undefined variable: an E_NOTICE-level error message will be issued, and the result will be NULL.
Note:
Array dereferencing a scalar value which is not a string silently yields NULL, i.e. without issuing an error message.

That's shit, but... well: it's not that surprising (I mean... it's PHP, right?). But how come

$array['id']

incurs the warning, but this doesn't:

$array[0]['id']

This just seems to make fairly unhelpful behaviour just that much less helpful.

I'm sure there's a reason for this. Not a good one, I hasten to add, but no doubt there's a reason.

Anyone know what it is?

Update:

Thanks to Dave (comment below) for pointing out there's a bug been raised for this: 68110.


Righto.

--
Adam

Thursday, 26 January 2017

Survey results: most useful response first

G'day:
I've got the results in from that survey, and started looking through them last night. I haven't really organised anything yet, so nothing useful to report, but this response stood out. The survey had 16 questions, but they left them all blank except question 15:

  1. What else do you think might be useful to share?
    You are toxic.
That's it. That's what they decided to write. That's what they figured was worth investing their time in.

Now I'll not contest whether or not I'm toxic, but I'm not sure that in a community survey aimed at understand how best to help people migrate their skills from CFML is really the best place for it. They coulda just emailed me (my email address is in the "Communications policy" link to the right there). But what I will conclude is if they read the survey questions, and decided that was a sensible, relevant and effective answer to make, they should perhaps be worrying less about how toxic I might or might not be, and more concerned about their own state of mind.

But still: at least I got a new quote for my banner line.

Yours toxically,

--
Adam

Tuesday, 24 January 2017

I'm a dick: part 4: why I'm a dick

G'day:
Sigh.

So other the page few days I've posted these articles:

This was all around the survey I was running (Survey: CFML usage and migration strategies), which freeonlinesurveys.com had closed access to unless I upgraded to a pay-for account. I assumed they were being dodgy, and reacted poorly to this.

A very patient user support person contacted me this evening, and pointed out this screen, when I first went to launch the survey:


And I needed to click on the "OK" button to proceed. I will admit I scanned the first coupla options and went "yeah yeah, same as the pricing page, where's the button to click?", but didn't get as far as the relevant bullet point, or it didn't sink in, or whatever.

However one spins it: they did tell me this information, I simply chose not to pay attention to it.

In freeonlinesurveys.com's favour, the offered to release the data anyhow. I have said to them "no, ballocks to that: it's my bad, I'll pay for it".

Sigh.

Let's join in a chorus of "Cameron's a dick".

But at least I can now start assessing those results, and try to work out how to feed back to you.

Righto.

--
Adam (who is a dick)

I'm a dick: part 3 Email sent to Bristol Trading Standards office re freeonlinesurveys.com

G'day:
What I said in this article turns out to be erroneous. I will post a follow-up to it shortly, but first things first, I'm taking the content down as I don't want it on Google.

But equally I don't want to hide what I said, so I'm gonna re-insert it as an image, so y'all can see the original.

I'll link to the follow-up once I've written it.

Righto.

--
Adam




I'm a dick: part 2 T&Cs located! partial correction on the freeonlinesurveys.com stitch-up

G'day:
What I said in this article turns out to be erroneous. I will post a follow-up to it shortly, but first things first, I'm taking the content down as I don't want it on Google.

But equally I don't want to hide what I said, so I'm gonna re-insert it as an image, so y'all can see the original.

I'll link to the follow-up once I've written it.

Righto.

--
Adam