Showing posts with label PHP. Show all posts
Showing posts with label PHP. Show all posts

Monday 2 July 2018

PHP: Expectation management with Symfony Validator validation

G'day:
I was checking out of of our web services the other day, replicating a bug I was fixing, and found the need to guess at what the body content needed to be for a POST I was doing. Our validation is solid, so I could just start with an empty object, and the validation would tell me what I needed to pass. Or, yes, I could RTFM but I couldn't be arsed.

Anyhow, this tactic worked for the first few properties: the validation came back with "you need this property"... "nah, it needs to be an integer"... "that's too low", etc. Cool. But then I passed an empty string for one of the properties, and instead of getting a 400 back with a hint as to what the constraints needed to be; I instead got a 500 server error. F***'s sake.

{
    "errors": [
        "Internal Server Error"
    ]
}

This is good and bad. Bad it was a 500 for some reason; good that there's no detail bleeding out. This was part of what I was fixing.

Looking at the logs I was getting this:

{"errors": ["Expected argument of type \"array or Traversable and ArrayAccess\", \"string\" given"]}

Indeed I was passing in a string:

{
  "firstName" : "Zachary",
  "lastName" : "Cameron Lynch",
  "address" : ""
}

(not the actual data in question, but it's work code so I cannot share it). Right so address is supposed to be an object, eg:

{
    "firstName" : "Zachary",
    "lastName" : "Cameron Lynch",
    "address" : {
        "street" : "123 Some Street",
        "city" : "Our City"
    }
}

That sort of thing.

But anyhow, why's the valiation failing? Well not failing - I'd expect / want that - but why is it erroring? I assumed we were doing something wrong: not validating it was an object, and just charging off and using it. I looked into it, and our code seemed legit, so start trying to make a solid repro case so I can ask people out in the world about it.

I know nothing about Symfony's Validator component. I have code-reviewed other people using it, so am vaguely aware of how it works, but I've not really used it. First things first then... the docs.

I concocted this test case from the introductory docs:

use PHPUnit\Framework\TestCase;
use Symfony\Component\Validator\Constraints\Length;
use Symfony\Component\Validator\Constraints\NotBlank;
use Symfony\Component\Validator\Validation;

class StringValidatorTest extends TestCase
{
    public function testExampleFromDocumentation()
    {
        $validator = Validation::createValidator();
        $violations = $validator->validate(
            'Bernhard',
            [
                new Length(['min' => 10]),
                new NotBlank(),
            ]
        );

        $this->assertCount(1, $violations);
        $this->assertEquals(
            'This value is too short. It should have 10 characters or more.',
            $violations[0]->getMessage()
        );
    }
}

And this worked as expected:

PHPUnit 7.2.6 by Sebastian Bergmann and contributors.

.                                                                   1 / 1 (100%)

Time: 218 ms, Memory: 4.00MB

OK (1 test, 2 assertions)

Also this is pretty straight fwd. Create a validator, pass a value and some constraints to a validate, and... job done.

From there I needed to test the behaviour of passing the wrong data type when expected. I decided expecting an integer and then not giving it one would be the easiest test here:

First a class to wrap-up the validator with the constraints in question:

namespace me\adamcameron\general\validation\collectionIssue;

use Symfony\Component\Validator\Constraints\Type;
use Symfony\Component\Validator\Validation;

class IntegerValidator
{

    private $validator;
    private $constraints;

    public function __construct()
    {
        $this->validator = Validation::createValidator();
        $this->setConstraints();
    }

    private function setConstraints()
    {
        $this->constraints = [
            new Type(['type' => 'integer'])
        ];
    }

    public function validate($value)
    {
        return $this->validator->validate($value, $this->constraints);
    }
}

Nothing surprising there. I have a constraints collection which is simply "gotta be an integer". And a method that takes a value and validates it against those constraints.

And then some tests:

<?php

namespace me\adamcameron\general\test\validation\collectionIssue;

use me\adamcameron\general\validation\collectionIssue\IntegerValidator;
use PHPUnit\Framework\TestCase;

class IntegerValidatorTest extends TestCase
{
    use ViolationAssertions;

    private $validator;

    protected function setUp()
    {
        $this->validator = new IntegerValidator();
    }

    /** @dataProvider provideCasesForValidateTests */
    public function testValidate($validValue)
    {
        $violations = $this->validator->validate($validValue);
        $this->assertHasNoViolations($violations);
    }

    public function provideCasesForValidateTests()
    {
        return [
            'has value' => ['value' => 42],
            'is null' => ['value' => null]
        ];
    }

    /** @dataProvider provideCasesForViolationsTests */
    public function testValidateReturnsViolationsWhenConstraintsBroken($invalidValue)
    {
        $violations = $this->validator->validate($invalidValue);
        $this->assertHasViolations($violations);
    }

    public function provideCasesForViolationsTests()
    {
        return [
            'string' => ['value' => 'forty-two'],
            'integer string' => ['value' => '42'],
            'float' => ['value' => 4.2],
            'integer as float' => ['value' => 42.0],
            'array' => ['value' => [42]]
        ];
    }
}

So I have two tests: valid values; invalid values. I expect no violations from the valid values, and some violations from the invalid values (I don't care what they are, just that there's a violation when the value ain't legit):

PHPUnit 7.2.6 by Sebastian Bergmann and contributors.

.......                                                             7 / 7 (100%)

Time: 204 ms, Memory: 4.00MB

OK (7 tests, 14 assertions)

(Oh, I whipped a coupla custom assertions out into a trait):

namespace me\adamcameron\general\test\validation\collectionIssue;

use Symfony\Component\Validator\ConstraintViolationList;

trait ViolationAssertions
{
    public function assertHasNoViolations($violations)
    {
        $this->assertInstanceOf(ConstraintViolationList::class, $violations);
        $this->assertEmpty($violations);
    }

    public function assertHasViolations($violations)
    {
        $this->assertInstanceOf(ConstraintViolationList::class, $violations);
        $this->assertNotEmpty($violations);
    }
}


Bottom line, as you can see from the test results, one can pass an invalid value to a validator, and it won't error; it'll just return you yer violation. Cool.

So what's the go with a collection then? Why are we getting that error?

I concocted a similar CollectionValidator:

namespace me\adamcameron\general\validation\collectionIssue;

use Symfony\Component\Validator\Constraints\Collection;
use Symfony\Component\Validator\Constraints\Type;
use Symfony\Component\Validator\Validation;

class CollectionValidator
{

    private $validator;
    private $constraints;

    public function __construct()
    {
        $this->validator = Validation::createValidator();
        $this->setConstraints();
    }

    private function setConstraints()
    {
        $this->constraints = [
            new Collection([
                'fields' => [
                    'field1' => [new Type(['type'=>'integer'])]
                ],
                'allowMissingFields' => true,
                'allowExtraFields' => true
            ])
        ];
    }

    public function validate($value = null)
    {
        return $this->validator->validate($value, $this->constraints);
    }
}

This time I'm defining a "collection" (read: "array" in this case) which could have a property field1, and if it does; it needs to be an integer. It could have other properties too: don't care. Or indeed field1 could be missing: don't care. But if it's there... it needs to be an integer.

And we test this. I'll break the class down this time so I can discuss:

namespace me\adamcameron\general\test\validation\collectionIssue;

use me\adamcameron\general\validation\collectionIssue\CollectionValidator;
use PHPUnit\Framework\TestCase;

class CollectionValidatorTest extends TestCase
{
    use ViolationAssertions;

    private $validator;

    protected function setUp()
    {
        $this->validator = new CollectionValidator();
    }

    /** @dataProvider provideCasesForValidateEmptyCollectionTests */
    public function testValidateWithValidButEmptyCollectionTypeIsOk($validValue)
    {
        $violations = $this->validator->validate($validValue);
        $this->assertHasNoViolations($violations);
    }

    public function provideCasesForValidateEmptyCollectionTests()
    {
        return [
            'array' => ['value' => []],
            'iterator' => ['value' => new \ArrayIterator()]
        ];
    }
}


First just a baseline test with some empty (but valid) values. These pass:

PHPUnit 7.2.6 by Sebastian Bergmann and contributors.

..                                                                  2 / 2 (100%)

Time: 187 ms, Memory: 4.00MB

OK (2 tests, 4 assertions)


Next I test a fully-legit collection with a valid integer value for the field1 value.

public function testValidateWithIntegerField1ValueShouldPass()
{
    $violations = $this->validator->validate(['field1'=>42]);
    $this->assertHasNoViolations($violations);
}

All good:

PHPUnit 7.2.6 by Sebastian Bergmann and contributors.

.                                                                   1 / 1 (100%)

Time: 211 ms, Memory: 4.00MB

OK (1 test, 2 assertions)

And now with the correct field1 property, but without an integer. This should have violations

public function testValidateWithNonIntegerField1ValueShouldHaveViolation()
{
    $violations = $this->validator->validate(['field1'=>'NOT_AN_INTEGER']);
    $this->assertHasViolations($violations);
    $this->assertSame('This value should be of type integer.', $violations[0]->getMessage());
}

Results:

PHPUnit 7.2.6 by Sebastian Bergmann and contributors.

.                                                                   1 / 1 (100%)

Time: 191 ms, Memory: 4.00MB

OK (1 test, 3 assertions)


And now one without field1, but with another property:

public function testValidateWithOnlyOtherFieldsShouldPass()
{
    $violations = $this->validator->validate(['anotherField'=>'another value']);
    $this->assertHasNoViolations($violations);
}


PHPUnit 7.2.6 by Sebastian Bergmann and contributors.

.                                                                   1 / 1 (100%)

Time: 191 ms, Memory: 4.00MB

OK (1 test, 2 assertions)


And for good measure I chuck it a null and see what happens:

public function testValidateWithNullIsApparentlyOK()
{
    $violations = $this->validator->validate(null);
    $this->assertHasNoViolations($violations);
}


Not sure this is legit IMO - a null is not a collection as per my constraint definition. But... well... not what I care about right now. It passes:

PHPUnit 7.2.6 by Sebastian Bergmann and contributors.

.                                                                   1 / 1 (100%)

Time: 259 ms, Memory: 4.00MB

OK (1 test, 2 assertions)


OK so at this point I reckon I have done my constraints properly as my tests all pass.

And now the test that replicates our problem:

public function testValidateWithNonTraversableShouldCauseViolation()
{
    $violations = $this->validator->validate('a string');
    $this->assertHasViolations($violations);
}


Note that here I am not passing a collection value at all. I am just passing a string. So this should have a violation, right? Nuh-uh:

PHPUnit 7.2.6 by Sebastian Bergmann and contributors.

E                                                                   1 / 1 (100%)

Time: 194 ms, Memory: 4.00MB

There was 1 error:

1) me\adamcameron\general\test\validation\collectionIssue\CollectionValidatorTest::testValidateWithNonTraversableShouldCauseViolation
Symfony\Component\Validator\Exception\UnexpectedTypeException: Expected argument of type "array or Traversable and ArrayAccess", "string" given

C:\src\php\general\vendor\symfony\validator\Constraints\CollectionValidator.php:37
C:\src\php\general\vendor\symfony\validator\Validator\RecursiveContextualValidator.php:829
C:\src\php\general\vendor\symfony\validator\Validator\RecursiveContextualValidator.php:675
C:\src\php\general\vendor\symfony\validator\Validator\RecursiveContextualValidator.php:118
C:\src\php\general\vendor\symfony\validator\Validator\RecursiveValidator.php:100
C:\src\php\general\src\validation\collectionIssue\CollectionValidator.php:36
C:\src\php\general\test\validation\collectionIssue\CollectionValidatorTest.php:63

ERRORS!
Tests: 1, Assertions: 0, Errors: 1.


So let me get this straight. I am passing validation code a value that ought not validate. And what I get is not a "it dun't validate mate" violation, the validation lib just breaks.

Grumble.

Now this just seems to me to be a testing-101-fail on the part of the Validator library. One must always be able to give the validator any value, and the result should just be "yeah all good", or "nup. And here's why". However as the Symfony mob usually do such solid work, I am kinda wondering if I'm messing something up, and just not seeing it?

What do you think? Even if you don't know PHP or Symfony validation, what would your expectations be here? Or can yous ee the glaring error that I can't see from having looked at this too long?

I will presently ask a question on this on Stack Overflow (a heavily-abridged version of this), but wanted to get some more friendly eyes on it first.

Update

This was bugging my colleague so he went in and checked the underlying Symfony code - something I should have done - and the CollectionValidator is very specifically throwing this exception very early in the piece:

public function validate($value, Constraint $constraint)
{
    if (!$constraint instanceof Collection) {
        throw new UnexpectedTypeException($constraint, __NAMESPACE__.'\Collection');
    }
    if (null === $value) {
        return;
    }
    if (!is_array($value) && !($value instanceof \Traversable && $value instanceof \ArrayAccess)) {
        throw new UnexpectedTypeException($value, 'array or Traversable and ArrayAccess');
    }


That strikes me as very daft, and contrary to the intend of a validator lib. It's been there since the very first commit of that code, as far as I can tell (back in 2010). It's clearly their intent, but the intent is misguided IMO.

Another update

This has just bitten us on the butt again, and my mate Levi came across this very article when trying to troubleshoot it. Ha. However I didn't post an answer. I know we worked around it somehow... just trying to find the code now.

We found a coupla issues on the Symfony bug tracker:
But seems the former one was closed in favour of the latter one, and nothing's been done about the latter one. Sigh.

I'll post a follow-up article and cross link to here once we've worked around it.

Cheers.

--
Adam

Monday 7 May 2018

In defence of public properties

G'day:
Recently I was re-writing one of our cron job scripts. It's the sort of thing that runs at 2am, reads some stuff from the DB, analyses it, and emails some crap to interested parties. We usually try to avoid complete rewrites, but this code was written... erm... [cough insert some period of time ago when it was apparently OK to write really shit code with no code quality checks and no tests]... and it's unmaintainable, so we're biting the bullet and rewriting coherently whilst adding some business logic changes into it.

The details for the email - to, from, subject, data to build the body from - come from three different sources in total, before sending to the mailer service to be queued-up for sending. So I knocked together a simple class to aggregate that information together:

class SpecialInterestingReportEmail {

    public $from;
    public $to;
    public $subject;
    public $body;

    public static $bodyTemplate = "specialInterestingReportEmail.html.twig";
    
    function __construct($from, $to, $subject, $body) {
        $this->from = $from;
        $this->to = $to;
        $this->subject = $subject;
        $this->body = $body;
    }
}

I then have a factory method in an EmailMessageService (not entirely sure that's the best name for it, but [shrug]) which chucks all the bits and pieces together, and returns an email object:

function createSpecialInterestingReportEmail($special, $interesting) {
    $body = $this-twigService->render(
        SpecialInterestingReportEmail::bodyTemplate,
        [
            "special" => $special,
            "interesting" => $interesting
        ]
    )
    return new SpecialInterestingReportEmail(
        $this->addressesForReport->from,
        $this->addressesForReport->to,
        $this->reportSubject,
        $body
    );
}

I don't think this design is perfect, but in the context of what we're doing it's OK.

Notice:
All interaction I mention below with my colleagues is paraphrased, embellished, or outright changed to make my point. I am not quoting anyone, and take this as a work of fiction. It's also drawn from previous similar conversations I've had on this topic.

One of my code reviewers was horrified:

Code Reviewer: "You can't have public properties! That's bad OOP!"
Me: "Why?"
CR: "You're breaking encapsulation. You need to do it like this [proceeds to tutor me on how to write accessor methods, because, like, I needed a tutorial on design anti-patterns]".
Me: "Why? What are we gaining by making those properties private, just to force us to write a bunch of boilerplate code to then access them? And how is that a good change to make to this code that now exists and is fulfilling the requirement? This is code review... we've already done the planning for this. This class is just a single container to aggregate some data into a conceptual 'email message' object. It doesn't need any behaviour - it doesn't need any methods - it's just for moving data between services".
CR: "But you can't break encapsulation!"
Me: "Why not? Other than parroting dogma read from some OOP 101 book, why is this a problem? And how's it solved by accessor methods?"
CR: "Well someone could directly change the values to be wrong!"
Me: "Well... they probably shouldn't do that then. That'd be dumb. They could equally put the wrong values straight into the constructor too. It'd still be just as wrong. Look... we don't need to validate the data - that seems to be your concern here? - as it's just a script reading known-good values from the DB, and sending them to our email queue. The code's right there. There's no scope for the data to accidentally be wrongified. And if it was... the tests would pick it up anyhow".
CR: "But what if other code starts using this code?"
Me: "What... yer saying 'what it some other application starts using this cronjob as some sort of library?' Why would that happen? This is not a public API. It makes no pretence of being a public API. If anyone started using this as an API, they deserve everything they get".
CR: "But they might".
ME: "OK, let's say some of this code is gonna be needed somewhere else, and we need to make it into a library. At that point in time, we'd extract the relevant code, consider stuff like encapsulation, data hiding, providing a public interface etc. But that would be different code from this lot".
CR: "But you should write all code like it will be used that way".
Me: "Again: why? This code is not going to be used this way. It just isn't. And anyhow, what yer suggesting is YAGNI / coding-for-an-unknown-future anyhow. We don't gain anything for the purposes of the current requirement chucking pointless boilerplate code into these classes. That's not an improvement".

And it went on.

One problem I encounter fairly frequently - both at work and in the wild - is people who will read about some concept, and thenceforth That Concept Is Law. Someone has Said It, therefore it applies to every single situation thereafter. They don't ever seem to bother trying to reason why the "law" was stated in the first place, what about it makes it a good rule, and when perhaps it's not necessary.

I look at these things and think "will this benefit my current task?". Generally it does because these things don't acquire acceptance without scrutiny and sanity-checking. But sometimes, however, it doesn't make sense to follow the dogma.

In this situation: it does not help.

In a different situation, if I was writing a separate API which handled the Email object creation, and other apps were gonna use it, I'd've possibly tightened-up the property access. But only possibly. My position on such things is to be liberal with what one permits to be done with code. If all my theoretical accessor method was gonna achieve was to return a private value... I'd really consider just leaving it public instead, and allow direct access. Why not?

There's a risk that later I might need to better control access to those properties, but... I'd deal with that at the time: these things can be handled as/when. It's even easy enough to have transitionary code from direct-access to accessor-access using __get and __set. I know these are frowned-upon, but in transitionary situations: they're fine. So one could seamlessly patch the API for code already consuming it via direct access with that transitionary approach, and then in the next (or some subsequent ~) version, make the breaking change to require using accessors, eg:

v1.0 - direct access only.
v1.1 - add in transitionary code using __get and __set. Advise that direct access is deprecated and will be removed in the next release. Also add in accessor methods.
v2.0 - remove direct access.

It doesn't even need to be v2.0. Just "some later version". But for the sake of making sure the transitionary code is temporary, better to do sooner rather than later. The thing is that software versioning is there for a reason, so it's OK to only introduce necessary coding overhead when it's needed.

Another thing that occurred to me when I was thinking about this. Consider this code:

$email = [
    "from" => "from@example.com",
    "to" => "to@example.com",
    "subject" => "Example subject",
    "body" => "Example body"
];

$fromAddress = $email["from"];

Perfectly fine. So how come this code is not fine:

$email = new Email(
    "from@example.com",
    "to@example.com",
    "Example subject",
    "Example body"
);

$fromAddress = $email->from;

Why's it OK to access an array directly, but it's not - apparently - OK to give the collection of email bits a name (Email), and otherwise use it the same way?

I can't think of one.

Rules are great. Rules apply sensibly 95% of the time. But when one reads about a rule... don't just stop there... understand the rule. The rules are not there simply for the sake of enabling one to not then think about things. Always think about things.

Righto.

--
Adam

PS: completely happy to be schooled as to how I am completely wrong here. This is half the reason I wrote this.

Saturday 5 May 2018

PHP: perpetual misuse of array type declarations

G'day:
This has been irking me enough recently to cause me to dust off this old blog.

For a while PHP has had limited argument type checking on function arguments, and this was improved in PHP 7. The docs cover it - Function arguments › Type declarations - so I'll not repeat them here. But we can have this code:

function takesArray(array $a){
    return $a;
}

var_dump(takesArray(["tahi", "rua", "toru", "wha"]));

Output:

array(4) {
  [0]=>
  string(4) "tahi"
  [1]=>
  string(3) "rua"
  [2]=>
  string(4) "toru"
  [3]=>
  string(3) "wha"
}

And if we try to pass something other than an array:

<?php
function takesArray(array $a){
    return $a;
}

try {
    $a = takesArray("NOT_AN_ARRAY");
} catch (\Throwable $t) {
    echo $t;
}

We get this:

TypeError:
Argument 1 passed to takesArray() must be of the type array, string given,
called in [...][...] on line 7 and defined in [...][...]:2
Stack trace:
#0 [...][...](7): takesArray('NOT_AN_ARRAY')
#1 {main}

Fair enough. No complaint there.

What I have an issue with is that now I see a lot of devs slapping "array" as an parameter type in every method signature that takes an array. What's wrong with that you probably ask? Well... in most situations they're doing this, it's not simply an array that is the requirement. It's generally one of these two situations:

function getFirstName(array $name){
    return $name['firstName'];
}

$firstName = getFirstName(["lastName" => "Cameron", "firstName" => "Adam"]);

echo $firstName;

OK, this is a really contrived example, but it's simple and demonstrates the point. Here the function takes an array, and returns the firstName from the array. But note that the requirement of the argument is not "it's an array", as stated. It very specifically needs to be "an associative array that has a key firstName". "array" is not correct here. It's not at all helpful. If one is needing to do this sort of thing, then create a class Name, and specify that. Because that's what you actually want here:

class Name {
    public $lastName;
    public $firstName;
    
    function __construct($lastName, $firstName) {
        $this->lastName = $lastName;
        $this->firstName = $firstName;
    }
}

function getFirstName(Name $name){
    return $name->firstName;
}

The second bogus usage of array as a type declaration is with indexed arrays. Consider this:

function filterOutEarlierDates(array $dates, int $year){
    $cutOff = new \DateTime("$year-01-01");

    $filtered = array_filter($dates, function (\DateTimeInterface $date) use ($cutOff) {
        $diff = $date->diff($cutOff);
        return $diff->invert === 1;
    });
    
    return array_values($filtered);
}

$dobs = [
    new \DateTime("1968-12-20"),
    new \DateTime("1970-02-17"),
    new \DateTime("2011-03-24"),
    new \DateTime("2016-08-17")
];
$cutOff = 2011;

$youngins = filterOutEarlierDates($dobs, $cutOff);

var_dump($youngins);


Here filterOutEarlierDates claims it takes an array. Well it does. Again this lacks the necessary precision for the requirement. It doesn't take an array. It takes - specifically - an array of DateTimeInterface objects. If you give it anything else than that, the code breaks. As type checking is specifically to guard against that, specifying array is simply neither correct or even helpful here.

Specifying array as an argument type check does have a place. When what the function needs actually is an array. And just an array. Any old array.

function array_some(array $array, callable $callback) {
    foreach ($array as $key => $value) {
        $result = $callback($value, $key);
        if ($result) {
            return true;
        }
    }
    return false;
}

$colours = ["whero", "karaka", "kowhai", "kakariki", "kikorangi", "poropango", "papura"];

$containsSixLetterWords = array_some($colours, function ($colour) {
   return strlen($colour) === 6; 
});

var_dump($containsSixLetterWords);

$numbers = [
    "one" => "tahi",
    "two" => "rua",
    "three" => "toru",
    "four" => "wha",
    "five" => "rima",
    "six" => "ono",
    "seven" => "whitu",
    "eight" => "ware",
    "nine" => "iwa",
    "ten" => "tekau"
];

$hasShortKeys = array_some($numbers, function ($number, $key) {
   return strlen($key) <= 3; 
});

var_dump($hasShortKeys);

Here's a PHP equivalent of JS's some method. It takes an array (any array) and a callback, and applies the callback to each array entry until the callback returns true, then it exits. This function correctly claims the first argument value needs to be an array. Without any other sort of qualification: it just needs to be an array.

Actually there's a similar issue with my type checking of the $callback parameter there. It can't be any callable: it needs to be a callable that takes a coupla arguments and returns a boolean. In C# I guess we'd use a Delegate for that, but there's no such construct in PHP. So I should probably settle for giving the argument a clear name ($callback probably isn't good enough here), and dispense with the type check.

But even then I question the merits of type-checking array here. What if instead of just an array, we had a collection object that implements \Iterator, eg:

class ColourCollection implements \Iterator {
    private $position = 0;
    private $colours;  

    public function __construct($colours) {
        $this->position = 0;
        $this->colours = $colours;
    }

    public function rewind() {
        $this->position = 0;
    }

    public function current() {
        return $this->colours[$this->position];
    }

    public function key() {
        return $this->position;
    }

    public function next() {
        ++$this->position;
    }

    public function valid() {
        return isset($this->colours[$this->position]);
    }
}

$rainbow = new ColourCollection($colours);

If we run this variation of the code, we get:

Fatal error:
Uncaught TypeError: Argument 1 passed to array_some() must be of the type array, object given,
 called in [...][...] on line 48 and defined in [...][...]:2
Stack trace:
#0 [...][...](48): array_some(Object(ColourCollection), Object(Closure))
#1 {main}
  thrown in [...][...] on line 2

If we take the type check out: the code works. So how is the array type check really helping us here? It's not.

To be honest I think I'm being a bit edge-case-y with that last example. I don't see a lot of objects that implement the iterator class: we just stick with arrays instead. But it is a legit consideration I guess.

I would use array as a type check in that array_some situation - where the argument actually just needs to be an array, with no special other qualities about it. But this is very rare. In other cases where the argument value needs to be "some specific type or array", then I think it's dead wrong to have an array type check there.

Righto.

--
Adam

Tuesday 28 November 2017

That array_map quandary implemented in other languages

G'day:
A coupla days ago I bleated about array_map [having] a dumb implementation. I had what I thought was an obvious application for array_map in PHP, but it couldn't really accommodate me due to array_map not exposing the array's keys to the callback, and then messing up the keys in the mapped array if one passes array_map more than one array to process.

I needed to remap this:

[
    "2008-11-08" => "Jacinda",
    "1990-10-27" => "Bill",
    "2014-09-20" => "James",
    "1979-05-24" => "Winston"
]

To this:

array(4) {
  '2008-11-08' =>
  class IndexedPerson#3 (2) {
    public $date =>
    string(10) "2008-11-08"
    public $name =>
    string(7) "Jacinda"
  }
  '1990-10-27' =>
  class IndexedPerson#4 (2) {
    public $date =>
    string(10) "1990-10-27"
    public $name =>
    string(4) "Bill"
  }
  '2014-09-20' =>
  class IndexedPerson#5 (2) {
    public $date =>
    string(10) "2014-09-20"
    public $name =>
    string(5) "James"
  }
  '1979-05-24' =>
  class IndexedPerson#6 (2) {
    public $date =>
    string(10) "1979-05-24"
    public $name =>
    string(7) "Winston"
  }
}

Note how the remapped object also contains the original key value. That was the sticking point. Go read the article for more detail and more whining.

OK so my expectations of PHP's array higher order functions are based  on  my experience with JS's and CFML's equivalents. Both of which receive the key as well as the value in all callbacks. I decided to see how other languages achieve the same end, and I'll pop the codee in here for shits 'n' giggles.


CFML

Given most of my history is as a CFML dev, that one was easy.

peopleData = ["2008-11-08" = "Jacinda", "1990-10-27" = "Bill", "2014-09-20" = "James", "1979-05-24" = "Winston"]

people = peopleData.map((date, name) => new IndexedPerson(date, name))

people.each((date, person) => echo("#date# => #person#<br>"))

Oh, this presupposes the IndexedPerson component. Due to a shortcoming of how CFML works, components must be declared in a file of their own:

component {

    function init(date, name) {
        this.date = date
        this.name = name
    }

    string function _toString() {
        return "{date:#this.date#; name: #this.name#}"
    }
}


But the key bit is the mapping operation:

people = peopleData.map((date, name) => new IndexedPerson(date, name))

Couldn't be simpler (NB: this is Lucee's CFML implementation, not ColdFusion's which does not yet support arrow functions).

The output is:


2008-11-08 => {date:2008-11-08; name: Jacinda}
1990-10-27 => {date:1990-10-27; name: Bill}
2014-09-20 => {date:2014-09-20; name: James}
1979-05-24 => {date:1979-05-24; name: Winston}

Also note that CFML doesn't have associative arrays, it has structs, so the keys are not ordered. This does not matter here. (Thanks to Zac for correcting me here: CFML does have ordered structs these days).


JS

The next language I turned to was JS as that's the I'm next most familiar with. One thing that hadn't occurred to me is that whilst JS's Array implementation has a map method, we need to use an object here as the keys are values not indexes. And whilst I knew Objects didn't have a map method, I didn't know what the equivalent might be.

Well it turns out that there's no real option to use a map here, so I needed to do a reduce on the object's entries, Still: it's pretty terse and obvious:

class IndexedPerson {
    constructor(date, name) {
        this.date = date
        this.name = name
    }
}

let peopleData = {"2008-11-08": "Jacinda", "1990-10-27": "Bill", "2014-09-20": "James", "1979-05-24": "Winston"}

let people = Object.entries(peopleData).reduce(function (people, personData) {
    people.set(personData[0], new IndexedPerson(personData[0], personData[1]))
    return people
}, new Map())

console.log(people)

This returns what we want:

Map {
  '2008-11-08' => IndexedPerson { date: '2008-11-08', name: 'Jacinda' },
  '1990-10-27' => IndexedPerson { date: '1990-10-27', name: 'Bill' },
  '2014-09-20' => IndexedPerson { date: '2014-09-20', name: 'James' },
  '1979-05-24' => IndexedPerson { date: '1979-05-24', name: 'Winston' } }

TBH I think this is a misuse of an object to contain basically an associative array / struct, but so be it. It's the closest analogy to the PHP requirement. I was able to at least return it as a Map, which I think is better. I tried to have the incoming personData as a map, but the Map prototype's equivalent of entries() used above is unhelpful in that it returns an Iterator, and the prototype for Iterator is a bit spartan.

I think it's slightly clumsy I need to access the entries value via array notation instead of some sort of name, but this is minor.

As with all my code, I welcome people showing me how I should actually be doing this. Post a comment. I'm looking at you Ryan Guill ;-)

Java

Next up was Java. Holy fuck what a morass of boilterplate nonsense I needed to perform this simple operation in Java. Deep breath...

import java.util.HashMap;
import java.util.Map;
import java.util.stream.Collectors;

class IndexedPerson {
    String date;
    String name;
    
    public IndexedPerson(String date, String name) {
        this.date = date;
        this.name = name;
    }
    
    public String toString(){
        return String.format("{date: %s, name: %s}", this.date, this.name);
    }
}

class Collect {

    public static void main(String[] args) {

        HashMap<String,String> peopleData = loadData();

        HashMap<String, IndexedPerson> people = mapToPeople(peopleData);
            
        dumpIdents(people);
    }
    
    private static HashMap<String,String> loadData(){
        HashMap<String,String> peopleData = new HashMap<String,String>();
        
        peopleData.put("2008-11-08", "Jacinda");
        peopleData.put("1990-10-27", "Bill");
        peopleData.put("2014-09-20", "James");
        peopleData.put("1979-05-24", "Winston");
        
        return peopleData;
    }
    
    private static HashMap<String,IndexedPerson> mapToPeople(HashMap<String,String> peopleData) {
        HashMap<String, IndexedPerson> people = (HashMap<String, IndexedPerson>) peopleData.entrySet().stream()
            .collect(Collectors.toMap(
                e -> e.getKey(),
                e -> new IndexedPerson(e.getKey(), e.getValue())
            ));
            
        return people;
    }
    
    private static void dumpIdents(HashMap<String,IndexedPerson> people) {
        for (Map.Entry<String, IndexedPerson> entry : people.entrySet()) {
            System.out.println(String.format("%s => %s", entry.getKey(), entry.getValue()));
        }
    }
    
}

Result:
1979-05-24 => {date: 1979-05-24, name: Winston}
2014-09-20 => {date: 2014-09-20, name: James}
1990-10-27 => {date: 1990-10-27, name: Bill}
2008-11-08 => {date: 2008-11-08, name: Jacinda}

Most of that lot seems to be just messing around telling Java what types everything are. Bleah.

The interesting bit - my grasp of which is tenuous - is the Collectors.toMap. I have to admit I derived that from reading various Stack Overflow articles. But I got it working, and I know the general approach now, so that's good.

Too much code for such a simple thing though, eh?


Groovy

Groovy is my antidote to Java. Groovy makes this shit easy:

class IndexedPerson {
    String date
    String name

    IndexedPerson(String date, String name) {
        this.date = date;
        this.name = name;
    }

    String toString(){
        String.format("date: %s, name: %s", this.date, this.name)
    }
}

peopleData = ["2008-11-08": "Jacinda", "1990-10-27": "Bill", "2014-09-20": "James", "1979-05-24": "Winston"]

people = peopleData.collectEntries {date, name -> [date, new IndexedPerson(date, name)]}

people.each {date, person -> println String.format("%s => {%s}", date, person)}

Bear in mind that most of that is getting the class defined, and the output. The bit that does the mapping is just the one line in the middle. That's more like it.

Again, I don't know much about Groovy… I had to RTFM to find out how to do the collectEntries bit, but it was easy to find and easy to understand.

I really wish I had a job doing Groovy.

Oh yeah, for the sake of completeness, the output was thus:

2008-11-08 => {date: 2008-11-08, name: Jacinda}
1990-10-27 => {date: 1990-10-27, name: Bill}
2014-09-20 => {date: 2014-09-20, name: James}
1979-05-24 => {date: 1979-05-24, name: Winston}


Ruby

Ruby's version was pretty simple too as it turns out. No surprise there as Ruby's all about higher order functions and applying blocks to collections and stuff like that.

class IndexedPerson

    def initialize(date, name)
        @date = date
        @name = name
    end

    def inspect
        "{date:#{@date}; name: #{@name}}\n"
    end
end

peopleData = {"2008-11-08" => "Jacinda", "1990-10-27" => "Bill", "2014-09-20" => "James", "1979-05-24" => "Winston"}

people = peopleData.merge(peopleData) do |date, name|
    IndexedPerson.new(date, name)
end

puts people

Predictable output:

{"2008-11-08"=>{date:2008-11-08; name: Jacinda}
, "1990-10-27"=>{date:1990-10-27; name: Bill}
, "2014-09-20"=>{date:2014-09-20; name: James}
, "1979-05-24"=>{date:1979-05-24; name: Winston}
}

I wasn't too sure about all that block nonsense when I first started looking at Ruby, but I quite like it now. It's easy to read.


Python

My Python skills don't extend much beyond printing G'day World on the screen, but it was surprisingly easy to google-up how to do this. And I finally got to see what Python folk are on about with this "comprehensions" stuff, which I think is quite cool.

class IndexedPerson:
    def __init__(self, date, name):
        self.date = date
        self.name = name

    def __repr__(self):
        return "{{date: {date}, name: {name}}}".format(date=self.date, name=self.name)

people_data = {"2008-11-08": "Jacinda", "1990-10-27": "Bill", "2014-09-20": "James", "1979-05-24": "Winston"}

people = {date: IndexedPerson(date, name) for (date, name) in people_data.items()}

print("\n".join(['%s => %s' % (date, person) for (date, person) in people.items()]))


And now that I am all about Clean Code, I kinda get the "whitespace as indentation" thing too. It's clear enough if yer code is clean in the first place.

The output of this is identical to the Groovy one.

Only one more then I'll stop.

Clojure

I can only barely do G'day World in Clojure, so this took me a while to work out. I also find the Clojure docs to be pretty impentrable. I'm sure they're great if one already knows what one is doing, but I found them pretty inaccessible from the perspective of a n00b. It's like if the PHP docs were solely the user-added stuff at the bottom of each docs page. Most blog articles I saw about Clojure were pretty much just direct regurgitation of the docs, without much value-add, if I'm to be honest.

(defrecord IndexedPerson [date name])

(def people-data (array-map "2008-11-08" "Jacinda" "1990-10-27" "Bill" "2014-09-20" "James" "1979-05-24" "Winston"))

(def people
  (reduce-kv
    (fn [people date name] (conj people (array-map date (IndexedPerson. date name))))
    (array-map)
    people-data))

(print people)

The other thing with Clojure for me is that the code is so alien-looking to me that I can't work out how to indent stuff to make the code clearer. All the examples I've seen don't seem very clear, and the indentation doesn't help either, I think. I guess with more practise it would come to me.

It seems pretty powerful though, cos there's mot much code there to achieve the desired end-goal.

Output for this one:

{2008-11-08 #user.IndexedPerson{:date 2008-11-08, :name Jacinda},
1990-10-27 #user.IndexedPerson{:date 1990-10-27, :name Bill},
2014-09-20 #user.IndexedPerson{:date 2014-09-20, :name James},
1979-05-24 #user.IndexedPerson{:date 1979-05-24, :name Winston}}


Summary

This was actually a very interesting exercise for me, and I learned stuff about all the languages concerned. Even PHP and CFML.

I twitterised a comment regarding how pleasing I found each solution:


This was before I did the Clojure one, and I'd slot that in afer CFML and before JS, making the list:
  1. Python
  2. Ruby
  3. Groovy
  4. CFML
  5. Clojure
  6. JS
  7. PHP
  8. Java

Python's code looks nice and it was easy to find out what to do. Same with Ruby, just not quite so much. And, really same with Groovy. I could order those three any way. I think Python tips the scales slightly with the comprehensions.

CFML came out suprisingly well in this, as it's a bloody easy exercise to achieve with it.

Clojure's fine, just a pain in the arse to understand what's going on, and the code looks a mess to me. But it does a lot in little space.

JS was disappointing because it wasn't nearly so easy as I expected it to be.

PHP is a mess.

And - fuck me - Java. Jesus.

My occasional reader Barry O'Sullivan volunteered some input the other day:


Hopefully he's still up for this, and I'll add it to the list so we can have a look at that code too.

Like I said before, if you know a better or more interesting way to do this in any of the languages above, or any other languages, make a comment and post a link to a Gist (just don't put the code inline in the comment please; it will not render at all well).

I might have another one of these exercises to do soon with another puzzle a friend of mine had to recently endure in a job-interview-related coding test. We'll see.

Righto.

--
Adam

Sunday 26 November 2017

PHP: array_map has a dumb implementation

G'day:
This cropped up a few weeks back, but I've had no motivation to write anything recently so I've sat on it for a bit. It came back to bite me again the other day and my motivation seems to be returning so here we go.

I was working on some code the other day wherein I needed to query some stuff from one DB, and then for the records returned from that, look for records in an entirely different data repository for some "additional data". The best I could do was to get the IDs from the first result and do like a WHERE IN (:ids) of the other data source to get records that were the additional data for some/all of the IDs. There was no one-to-one match between the two data sources, so the second set of results might not have records for all IDs from the first one. It was convenient for me to return the second result not as an indexed array, but as an associative array keyed on the ID. This is easy enough in PHP, and is quite handy:

$peopleData = $conn
    ->query('SELECT date, name FROM mapexampledata')
    ->fetchAll(PDO::FETCH_ASSOC | PDO::FETCH_COLUMN | PDO::FETCH_UNIQUE);

var_dump($peopleData);

Result:

[
    "2008-11-08" => "Jacinda",
    "1990-10-27" => "Bill",
    "2014-09-20" => "James",
    "1979-05-24" => "Winston"
]

Note how the array key is the date column from the query, and the value of the array is the other column (or all the subsequent columns, if there were more). I've just indexed by date here to make it more clear that the index key is not just an integer sequence.

From this data, I needed to remap that to be an array of PersonalMilestone objects, rather than an array of raw data. Note that the object has both date and name values:

class PersonalMilestone {
    public $date;
    public $name;

    function __construct($date, $name) {
        $this->date = $date;
        $this->name = $name;
    }
}

(If it's not obvious... this domain model and data is completely unrelated to the work I was actually doing, it's just something simple I concocted for this blog article. The general gist of the issue is preserved though).

"Easy", I thought; "that's just an array_map operation". Oh hohohoho, how funny I am. I forgot I was using PHP.

So I wrote my tests (OK, I did not write the tests for this example for the blog. "IRL" I did though. Always do TDD, remember?), and then went to write the array_map implementation:

$peopleData = ["2008-11-08" => "Jacinda", "1990-10-27" => "Bill", "2014-09-20" => "James", "1979-05-24" => "Winston"];

$people = array_map(function ($name) {
    return new PersonalMilestone(
        $date, // dammit this won't work: array_map only gives me the value, not the key
        $name
    );
}, $peopleData);

F*** it. This just beggars my belief as the other languages I have done the same operation on passes both key and value into the map call back. What does PHP do instead? Well it allows you to pass multiple arrays to the array_map function, and the callback receives the value from each of those arrays. I dunno why they chose to implement that as even additional behaviour, let along the default (and only) behaviour. Sigh.

But I thought I could leverage this! I could pass the keys of my array in as a second array, and then I'll have both the values I need to create the PersonalMilestone object. Hurrah!

$peopleData = ["2008-11-08" => "Jacinda", "1990-10-27" => "Bill", "2014-09-20" => "James", "1979-05-24" => "Winston"];

$keys = array_keys($peopleData);

$people = array_map(function ($name, $date) {
    return new PersonalMilestone($date, $name);
}, $peopleData, $keys);

var_dump($people);

Result:

array(4) {
  [0]=>
  object(PersonalMilestone)#2 (2) {
    ["date"]=>
    string(10) "2008-11-08"
    ["name"]=>
    string(7) "Jacinda"
  }
  [1]=>
  object(PersonalMilestone)#3 (2) {
    ["date"]=>
    string(10) "1990-10-27"
    ["name"]=>
    string(4) "Bill"
  }
  [2]=>
  object(PersonalMilestone)#4 (2) {
    ["date"]=>
    string(10) "2014-09-20"
    ["name"]=>
    string(5) "James"
  }
  [3]=>
  object(PersonalMilestone)#5 (2) {
    ["date"]=>
    string(10) "1979-05-24"
    ["name"]=>
    string(7) "Winston"
  }
}

Looks OK right? Look again. The array has been changed into an indexed array. So this isn't a remapping, this is just "making a different array". F***'s sake, PHP.

Initially I thought this was because the two arrays I was remapping had different keys, and thought "aah... yeah... if I was daft I could see how I might implement the thing like this", so I changed the second array to be keyed by its own value (whilst shaking my head in disbelief).

$peopleData = ["2008-11-08" => "Jacinda", "1990-10-27" => "Bill", "2014-09-20" => "James", "1979-05-24" => "Winston"];
var_dump($peopleData);

$keys = array_keys($peopleData);
$keysIndexedByKeys = array_combine($keys, $keys);
var_dump($keysIndexedByKeys);

$people = array_map(function ($name, $date) {
    return new PersonalMilestone($date, $name);
}, $peopleData, $keys);

var_dump($people);

(and, yes, this is a lot of horsing around now, even had it worked which (spoiler) it didn't).

Result:
array(4) {
  ["2008-11-08"]=>
  string(7) "Jacinda"
  ["1990-10-27"]=>
  string(4) "Bill"
  ["2014-09-20"]=>
  string(5) "James"
  ["1979-05-24"]=>
  string(7) "Winston"
}
array(4) {
  ["2008-11-08"]=>
  string(10) "2008-11-08"
  ["1990-10-27"]=>
  string(10) "1990-10-27"
  ["2014-09-20"]=>
  string(10) "2014-09-20"
  ["1979-05-24"]=>
  string(10) "1979-05-24"
}
array(4) {
  [0]=>
  object(PersonalMilestone)#2 (2) {
    ["date"]=>
    string(10) "2008-11-08"
    ["name"]=>
    string(7) "Jacinda"
  }
  [1]=>
  object(PersonalMilestone)#3 (2) {
    ["date"]=>
    string(10) "1990-10-27"
    ["name"]=>
    string(4) "Bill"
  }
  [2]=>
  object(PersonalMilestone)#4 (2) {
    ["date"]=>
    string(10) "2014-09-20"
    ["name"]=>
    string(5) "James"
  }
  [3]=>
  object(PersonalMilestone)#5 (2) {
    ["date"]=>
    string(10) "1979-05-24"
    ["name"]=>
    string(7) "Winston"
  }
}

So the first array and the second array now have the same keys, but... the remapped array still doesn't. This is where we get to the subject line of this article: array_map has a dumb implementation.

I gave up on array_map. I really don't think it's fit for purpose beyond the most obviously simple applications. I just used a foreach loop instead:

$people = [];
foreach ($peopleData as $date => $name) {
    $people[$date] = new PersonalMilestone($date, $name);
}

var_dump($people);

And this works:

array(4) {
  ["2008-11-08"]=>
  object(PersonalMilestone)#1 (2) {
    ["date"]=>
    string(10) "2008-11-08"
    ["name"]=>
    string(7) "Jacinda"
  }
  ["1990-10-27"]=>
  object(PersonalMilestone)#2 (2) {
    ["date"]=>
    string(10) "1990-10-27"
    ["name"]=>
    string(4) "Bill"
  }
  ["2014-09-20"]=>
  object(PersonalMilestone)#3 (2) {
    ["date"]=>
    string(10) "2014-09-20"
    ["name"]=>
    string(5) "James"
  }
  ["1979-05-24"]=>
  object(PersonalMilestone)#4 (2) {
    ["date"]=>
    string(10) "1979-05-24"
    ["name"]=>
    string(7) "Winston"
  }
}

This is simple enough code, but I like having my code more descriptive if possible, so if I'm iterating over an array because I want to remap its values, then I prefer to use a mapping call, not a generic loop. But we need to be pragmatic about these things.

Oh... for the sake of completeness I did work out how to do it "properly" (where "properly" == "passes my tests" not "is good code"):

$keys = new ArrayIterator(array_keys($peopleData));

$people = array_map(function ($name) use ($keys) {
    $date = $keys->current();
    $keys->next();
    return new PersonalMilestone($date, $name);
}, $peopleData);

(I'll spare you the output... you get the idea by now).

What I've done here is to pass a separate iterator into the callback, and I manually iterate over that each iteration of the callback loop. This gives me the value for the date key for each element of the source array. But that's just shoehorning square peg into a round hole, so I would note that down as a curio, but not actually advocate its use.

I was left with the feeling "PHP, you're f***in' rubbish" (a feeling I get increasingly the more I use it), but I thought just cos I know other languages which do this in a more clear fashion doesn't mean that they're right and PHP is (unhelpfully) wrong. I decided to have a look at how a few other languages tackle the same issue, to see if PHP really does suck in this regard. However that will be the topic for a follow-up article, as this one is already long enough.

Righto.

--
Adam

Sunday 23 July 2017

PHP: A function that returns two different things

G'day:
I'm gonna plagiarise one of me own answers to a Stack Overflow question here, as I think it's good generic advice.

The question was "PHP 7: Multiple function return types". The subject line sums it up really. The person here wants to return a value or false from a function. I see this a lot in PHP code. Indeed... PHP itself does it a lot.

My answer was as follows:

[...]

From a design perspective, having a function that potentially returns different types of result indicates a potential design flaw:

  • If you're returning your result, or otherwise false if something didn't go to plan; you should probably be throwing an exception instead. If the function processing didn't go according to plan; that's an exceptional situation: leverage the fact. I know PHP itself has a habit of returning false if things didn't work, but that's just indicative of poor design - for the same reason - in PHP.
  • If your function returns potentially different things, then it's quite possible it's doing more than one thing, which is bad design in your function. Functions should do one thing. If your function has an if/else with the true/false block handling different chunks of processing (as opposed to just handling exit situations), then this probably indicates you ought to have two functions, not one. Leave it to the calling code to decide which is the one to use.
  • If your function returns two different object types which then can be used in a similar fashion in the calling code (ie: there's no if this: do that; else do this other thing in the calling code), this could indicate you ought to be returning an interface, not a concrete implementation.
There will possibly be legit situations where returning different types is actually the best thing to do. If so: all good. But that's where the benefit of using a loosely typed language comes in: just don't specify the return type of the function. But… that said… the situation probably isn't legit, so work through the preceding design considerations first to determine if you really do have one of these real edge cases where returning different types is warranted.

I'll add two more considerations here.

There's another option which is kinda legit. In PHP 7.1 one can declare the return type to be nullable.

Consider a function which potentially returns a boolean:

function f($x) {
    if ($x) {
        return true;
    }
}

We can't declare that with a return type of bool as if our logic strays done that falsey branch, we get a fatal error:

function f($x) : bool {
    if ($x) {
        return true;
    }
}

f(1); // OK

f(0); // PHP Fatal error:  Uncaught TypeError: Return value of f() must be of the type boolean, none returned


But in 7.1, one can declare the return type as nullable:

function g($x) : ?bool {
    if ($x) {
        echo "returning true";
        return true;
    }

    echo "returning null";
    return null;
}

g(1); // returning true
g(0); // returning null

Now sometimes this is legit. One might be looking for something from some other resource (eg: a DB record) which quite legitimately might not exist. And there might not be a legit "null implementation" of the given class one is wanting to return: for example returning the object with some/all of its properties not set or something. In this case a null is OK. But remember in this case one is forcing more logic back on the calling code: checking for the null. So not necessarily a good approach. I'd consider whether this situation is better handled with an exception.

The last consideration is that one can still specify multiple return types in a type-hint annotation, eg:

/**
 * @param bool $b
 * @return A|B
 */
function f($b){
    if ($b) {
        return new A();
    }else {
        return new B();
    }
}


This doesn't actually enforce anything, so it's mostly a lie, just like all comments are, but at least one's IDE might be able to make sense of it, and give code assistance and autocomplete suggestions when writing one's code:



See how it offers both the method from an A and a B there.

Still: I really dislike these annotations as they're really just comments, and especially in this case they're lying: f can actually return anything it likes.

Right... must dash. I am supposed to be down @ my ex's place hanging out with my son in 10min. I'll press "send" and proof-read this later ;-)

Righto.

--
Adam

Saturday 13 May 2017

PHPUnit: using a mocked-method call log to track its activity

G'day:
This is just a quick PHPUnit one, inspired by a trick my team lead suggested to me. I think it's quite handy and works around what seems to be a shortcoming of how PHPUnit mocks stuff. I'm sure there's strong opinions held on this sort of thing somewhere as to how I'm not supposed to want to do what my requirement has me needing to do here. I do so look forward to being told about it.

Anyway, I'm writing a script to repair some bung data we have. The details are irrelevant, but it's fairly "important" data (well: not to me... but to someone, apparently...), and I want to make sure we can track when things go wrong. And they will go wrong. Firstly I'm talking to an external third-party service, and it's foolhardy to trust those; secondly I'm relying on some of our own internal services which... ah... well yeah they were written in a different era by people with an unusual understanding of how to write managable, testable, and reliable code. And in the course of doing this work I turned-up a coupla bugs and had to fix those, so I don't back our own code to be stable here. I don't mean that judgementally: all code bases have better and not so good code in them. Anyway...

The upshot of this is the script logs a bunch of stuff - said script will be run by a cron job - and part of my testing is to verify the right things gets logged at the right time. A lot of it is fairly perfunctory "belt and braces" sort of logging, say "Records 1000-1999 processed OK", and I don't really care about that; but there's other stuff where the script deals with unexpected circumstances and I want to make sure that's done right.

For the purposes of this test I am mocking out all the dependencies so the script doesn't actually do anything, but the mocks return "workable" mocked data sufficient to walk through the stages of the script. And the logger is one of these dependencies, and is also mocked.

The test I was stuck on was one where an unhandled exception was thrown, and the script basically shuts itself down; having first logged what went wrong.

Here's a very pared-back version of the script:

public function myMethod($times){
    try {
        $this->logger->logMessage("Starting off");
        for ($i=1; $i <= $times; $i++) {
            $this->logger->logMessage("Starting processing iteration $i");
            $this->helper->doThing();
            $this->logger->logMessage("Finished processing iteration $i");
        }
        $this->logger->logMessage("Finishing off");
    }
    catch (\Exception $e) {
        $this->logger->logMessage(sprintf("Something went wrong: %s", $e->getMessage()));
        throw $e;
    }
    finally {
        $this->logger->logMessage("All done");
    }
}

Here I've just got the one doThing dependency call in the loop, but it makes the point. And around that, and before and after the the loop I log a line to track progress. And I log a line if there's an unhandled exception. As I said the real script is somewhat more complex than this, but this is an adequate analogy.

I have another test which tests the ongoing logging of a successful run with 0, 1, >1 records, but this test is to make sure the script a) reports the exception; b) still shuts down OK.

PHPUnit's approach to checking a specific argument was passed to a particular mock call is to use the at matcher method when setting an expectation. My first pass at the test used this matcher:

function testMyMethodUsingAt()
{
    $exceptionMessage = "EXPECT_THIS";
    $this->expectException(\RuntimeException::class);
    $this->expectExceptionMessage($exceptionMessage);

    $testIterations = 6;
    $errorAt = $testIterations / 2;

    $this->helper
        ->expects($this->at($errorAt))
        ->method("doThing")
        ->willThrowException(new \RuntimeException($exceptionMessage));

    $this->logger
         ->expects($this->at(7))
         ->method("logMessage")
         ->with("Starting processing iteration 4");
    $this->logger
         ->expects($this->at(8))
         ->method("logMessage")
         ->with("Something went wrong: $exceptionMessage");
    $this->logger
         ->expects($this->at(9))
         ->method("logMessage")
         ->with("All done");

    $this->sut->myMethod($testIterations);
}

Here I worked out the the call history for info would be nine entries long, and the entries I wanted to confirm the exception-tracking logic was correct was the 7th, 8th and 9th ones.

But this is problematic for a number of ways:
  • it's not immediately clear what hat I pulled 7th, 8th and 9th from;
  • it's fragile because it relies on there being precisely seven earlier log entries for it to work. EG: if we were to add another log line into the method, that would break this test, which is rubbish;
  • semantically it's incorrect, as it's not the 7th, 8th and 9th entries I'm interested in; it's the last three. It'd be better to code it accordingly so the code was more readable to maintainers, later on.
  • Lastly the at approach to things in PHPUnit is just a bit crap because the index is based on the mock object, not the mock method, despite almost all use-cases I've had for this sort of thing have been wanting to check sequential calls to the mocked method, not just to any method in the mocked object. So I try to steer clear of at at the best of times.
Other mocking frameworks I've used expose the call-log for a mocked method, but I cannot find any way to get it from PHPUnit. I can see where the data is stored, but to get it out would be deeply "Heath Robinson", and would be even worse than the approach above.

I was stumped.

Then me boss said "just use a callback in the will expectation, and build the log yerself". Initially I just looked at him like he was a loony (he's used to this), but then it dawned on me what he was suggesting. And it was easy, and not altogether awful, either:

function testMyMethodUsingCallLog()
{
    $exceptionMessage = "EXPECT_THIS";
    $this->expectException(\RuntimeException::class);
    $this->expectExceptionMessage($exceptionMessage);

    $testIterations = 6;
    $errorAt = $testIterations / 2;

    $this->helper
        ->expects($this->at($errorAt))
        ->method("doThing")
        ->willThrowException(new \RuntimeException($exceptionMessage));

    $callLog = [];
    $this->logger
        ->method("logMessage")
        ->will($this->returnCallback(function ($message) use (&$callLog) {
            $callLog[] = $message;
        }));

    try {
        $this->sut->myMethod($testIterations);
    }
    catch (\Exception $e) {
        $this->assertSame("All done", array_pop($callLog));
        $this->assertSame("Something went wrong: $exceptionMessage", array_pop($callLog));
        $this->assertNotSame("Finishing off", array_pop($callLog));

        throw $e;
    }
}

The first key bit is capturing the call-log. This creates an array, and with each call to the mocked logging method, we capture what was logged. Note that these expectations don't enforce any rules here. They just capture what's going on.

Because this method call throws an exception, I have to try/catch it so that I can regain control once it's errored, and inspect the call log to see it's what I expect. My approach here is far more semantic: I check the last entry is the "Finishing off" one, the one before that is logging the correct exception, and - this is slightly different now - as a failsafe I check that the one before that is anything other than "All Done". Because if it was that value, it'd mean the logic flow did not branch at the right stage of things. Lastly I throw the exception I caught so PHPUnit's own exception-expectation-checking also validates that too.

One of the best things about this is that this test doesn't care about what goes on in the log prior to the stuff it's testing, it is able to focus on precisely what it cares about. This is much clearer code. An example of this would be to change this line in each test:

$testIterations = 6;

Change it to be any other number, and the first test breaks, whereas the second test keeps working. Win.

That's pretty cool.

I hasten to add I would probably not usually be focusing quite this closely on this sort of sequencing, but in this case we need to know things are working properly.

It's a shame PHPUnit doesn't just expose the call-log like this, as I can't be the first person who's been in this situation. But I guess the tools are there to DIY like I have here, so no harm done.

Righto.

--
Adam

Thursday 9 March 2017

PHPUnit: setMethods and how I've been writing too much code

G'day:
Seems our technical architect is becoming my PHP muse. Good on ya, Pete.

Anyway, he just sent out what I thought was a controversial email in which he said (paraphrase) "we're calling setMethods way too often: in general we don't need to call it". To contextualise, I mean this:

$dependency = $this
    ->getMockBuilder(MyDependency::class)
    ->setMethods(['someMethod'])
    ->getMock();

$dependency
    ->method('someMethod')
    ->willReturn('MOCKED RESULT');


See here I'm explicitly saying "mock out the someMethod method". Pete's contention is that that's generally unnecessary, cos getMockBuilder mocks 'em all by default anyhow. This goes against my understanding of how it works, and I've written bloody thousands of unit tests with PHPUnit so it surprised me I was getting it wrong. I kinda believed him cos he said he tested it, but I didn't believe him so much as to not go test it myself.

So I knocked out a quick service and dependency for it:

namespace me\adamcameron\myApp;

class MyService
{

    private $myDecorator;

    public function __construct(MyDecorator $myDecorator)
    {
        $this->myDecorator = $myDecorator;
    }

    public function decorateMessage($message)
    {
        $message = $this->myDecorator->addPrefix($message);
        $message = $this->myDecorator->addSuffix($message);
        return $message;
    }

}

namespace me\adamcameron\myApp;

class MyDecorator
{

    public function addPrefix($message)
    {
        return "(ACTUAL PREFIX) $message";
    }

    public function addSuffix($message)
    {
        return "$message (ACTUAL SUFFIX)";
    }

}

Nothing surprising there: the service takes that decorator as a constructor arg, and then when calling decorateMessage the decorator's methods are called.

In our tests of decorateMessage, we don't want to use the real decorator methods, we want to use mocks.

First I have a test the way I'd normally mock things: explicitly calling setMethods with the method names:

public function testWithExplicitMock()
{
    $decorator = $this
        ->getMockBuilder(MyDecorator::class)
        ->setMethods(['addPrefix', 'addSuffix'])
        ->getMock();

 $decorator
  ->method('addPrefix')
  ->willReturn('(MOCKED PREFIX)');

 $decorator
  ->method('addSuffix')
  ->willReturn('(MOCKED SUFFIX)');

 $service = new MyService($decorator);

 $result = $service->decorateMessage('TEST MESSAGE');

 $this->assertSame(
        '(MOCKED SUFFIX)',
        $result
    );
}

And this mocks out the method correctly. This is my baseline.

Secondly, I still call setMethods, but I give it an empty array:

public function testWithImplicitMock()
{
    $decorator = $this
        ->getMockBuilder(MyDecorator::class)
        ->setMethods([])
        ->getMock();

 $decorator
  ->method('addPrefix')
  ->willReturn('(MOCKED PREFIX)');

 $decorator
  ->method('addSuffix')
  ->willReturn('(MOCKED SUFFIX)');

 $service = new MyService($decorator);

 $result = $service->decorateMessage('TEST MESSAGE');

 $this->assertSame(
        '(MOCKED SUFFIX)',
        $result
    );
}

This also mocks out the method correctly. Passing an empty array mocks out all the methods in the mocked class.

Next I try passing null to setMethods:

public function testWithNull()
{
    $decorator = $this
        ->getMockBuilder(MyDecorator::class)
        ->setMethods(null)
        ->getMock();

    $decorator
        ->method('addPrefix')
        ->willReturn('(MOCKED PREFIX)');

    $decorator
        ->method('addSuffix')
        ->willReturn('(MOCKED SUFFIX)');

    $service = new MyService($decorator);

    $result = $service->decorateMessage('TEST MESSAGE');

    $this->assertSame(
        '(ACTUAL PREFIX) TEST MESSAGE (ACTUAL SUFFIX)',
        $result
    );
}

This creates the mocked object, but the methods are not mocked. So the result here is using the actual methods.

In the last example I mock one of the methods (addPrefix), but not the other (addSuffix):

public function testWithOneExplicitMock()
{
    $decorator = $this
        ->getMockBuilder(MyDecorator::class)
        ->setMethods(['addPrefix'])
        ->getMock();

    $decorator
        ->method('addPrefix')
        ->willReturn('(MOCKED PREFIX)');

    $service = new MyService($decorator);

    $result = $service->decorateMessage('TEST MESSAGE');

    $this->assertSame(
        '(MOCKED PREFIX) (ACTUAL SUFFIX)',
        $result
    );
}

Here we see the important bit: when one explicitly mocks one or a subset of methods, then the other methods are not mocked. Which is kinda obvious now I say it, but I guess we had been thinking one needed to call setMethods to be able to then mock the behaviour of the method. And, TBH, I don't think we thought through what was happening to the methods we were not mocking. I think we started calling setMethods all the time because we were slavishly following the PHPUnit docs, which always call it too, which is a less than ideal precedent to "recommend".

Generally speaking, one should want to mock all methods of a dependency, and it should really be the exception when explicitly not wanting to mock a method: ie, to leave it actually callable.

The win here is that we can now de-clutter our tests a bit. We really seldom need to call setMethods, and we sure want to make it clear when we do want to only mock some of a dependency's methods. So this will make our code cleaner and clearer, and easier to code review and maintain. Win!

Cheers Pete!

Righto.

--
Adam

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