Wednesday, 1 February 2023

PHP: refactoring logic out of a controller and into a service class

G'day:

Context

In yesterday's article - TDD & Symfony & Monolog: adding some logging to that endpoint from last time - I added some logging logic into this address-lookup web service endpoint I'm putting together. At the end of it, I thought the logic - which I had slung into the controller - was in the wrong place, and I needed to shimmy it into an intermediary service class between the controller and the upstream web service adapter (this will make more sense once you see the code below). Today I've done that refactoring.

Where I was

Here's the controller code:

public function doGet(string $postcode) : JsonResponse
{
    try {
        $response = $this->addressServiceAdapter->get($postcode);

        $this->logUnexpectedFailures($response, $postcode);

        return new JsonResponse(
            [
                'postcode' => $postcode,
                'addresses' => $response->getAddresses(),
                'message' => $response->getMessage()
            ],
            $response->getHttpStatus()
        );
    } catch (\Exception $e) {
        return new JsonResponse(
            [
                'postcode' => $postcode,
                'addresses' => [],
                'message' => $e->getMessage()
            ],
            HttpStatusCode::HTTP_INTERNAL_SERVER_ERROR
        );
    }
}
private const RESPONSES_TO_LOG = [
    HttpStatusCode::HTTP_UNAUTHORIZED => Level::Critical,
    HttpStatusCode::HTTP_FORBIDDEN => Level::Critical,
    HttpStatusCode::HTTP_TOO_MANY_REQUESTS => Level::Warning,
    HttpStatusCode::HTTP_INTERNAL_SERVER_ERROR => Level::Warning
];

// …

private function logUnexpectedFailures(
    AddressService\Response $response,
    string $postcode
): void {
    $statusCode = $response->getHttpStatus();

    if (array_key_exists($statusCode, self::RESPONSES_TO_LOG)) {
        $this->logger->log(
            self::RESPONSES_TO_LOG[$statusCode],
            AddressService\Adapter::ERROR_MESSAGES[$statusCode],
            ['postcode' => $postcode, 'message' => $response->getMessage()]
        );
    }
}

What's going on here is that the upstream web service could return with a number of cautionary HTTP status codes which we really need to know about if they happen, so I've added some logging in to make sure we know.

And the adapter code it calls:

public function get(string $postCode) : AddressService\Response
{
    $response = $this->makeRequest($postCode);
    $lookupResult = $this->extractValidLookupResult($response);

    return $this->handleValidatedResponse($response, $lookupResult);
}

After getting the response from the upstream third-party web service, I validate what comes back (valid JSON with the schema I expect, etc; this could throw an exception if the data is no good), I pass it to a handler that returns well-formed data:

private function handleValidatedResponse(
    ResponseInterface $response,
    array $lookupResult
): AddressService\Response {
    $statusCode = $response->getStatusCode();

    if ($statusCode == HttpFoundationResponse::HTTP_OK) {
        return $this->handleSuccessResponse($lookupResult);
    }
    return $this->handleFailureResponse($lookupResult, $statusCode);
}

private function handleSuccessResponse(array $lookupResult): AddressService\Response
{
    if (
        !array_key_exists("addresses", $lookupResult)
        || !is_array($lookupResult["addresses"])
        || count(array_filter($lookupResult["addresses"], fn($address) => !is_string($address)))
    ) {
        throw new AddressService\Exception("Response JSON schema is not valid");
    }

    return new AddressService\Response(
        $lookupResult["addresses"],
        HttpFoundationResponse::HTTP_OK
    );
}

private function handleFailureResponse(
    array $lookupResult,
    int $statusCode
): AddressService\Response {
    if (array_key_exists("Message", $lookupResult) && is_string($lookupResult["Message"])) {
        return new AddressService\Response([], $statusCode, $lookupResult["Message"]);
    }
    return new AddressService\Response([], $statusCode, "No failure message returned from service");
}

There's no need to worry too much about the details there. Just that we're returning data from a third-party web service, and the results could be good, bad or ugly.

This approach of having a controller calling the adapter directly was fine for the first iteration of this code, where it was basically getting the data from the adapter and returning it. Once I added in the logging - and the conditionality (business logic) around it - it occurred to me that the controller was not the right place for this. Logging is nothing to do with servicing an HTTP request, after all. And that's all a controller is supposed to do. I also couldn't push the logging into the adapter, as that's not an adapter's job either. It's just to provide an application-friendly interface to something that doesn't have such an interface. And nothing else.

So I'm shifting that logging logic into a service.

Where I am now

There's the controller now:

class PostcodeLookupController extends AbstractController
{

    public function __construct(private readonly PostcodeLookupService $postcodeLookupService)
    {
    }

    public function doGet(string $postcode) : JsonResponse
    {
        $response = $this->postcodeLookupService->lookup($postcode);
        return new JsonResponse(
            [
                'postcode' => $postcode,
                'addresses' => $response->getAddresses(),
                'message' => $response->getMessage()
            ],
            $response->getHttpStatus()
        );
    }
}

Note how it is receiving a PostcodeLookupService now. It was receiving the Adapter and the Logger before. And also note how it's really not doing much any more. Asking for some data from the service, returning it. Simple. Excellent.

And the service:

class PostcodeLookupService
{
    private const RESPONSES_TO_LOG = [
        HttpStatusCode::HTTP_UNAUTHORIZED => Level::Critical,
        HttpStatusCode::HTTP_FORBIDDEN => Level::Critical,
        HttpStatusCode::HTTP_TOO_MANY_REQUESTS => Level::Warning,
        HttpStatusCode::HTTP_INTERNAL_SERVER_ERROR => Level::Warning
    ];

    public function __construct(
        private readonly GetAddress\Adapter $adapter,
        private readonly LoggerInterface $addressServiceLogger
    ) {
    }

    public function lookup(string $postcode): GetAddress\Response
    {
        try {
            return $this->getAddresses($postcode);
        } catch (GetAddress\Exception $e) {
            return $this->handleAdapterException($e, $postcode);
        }
    }

    private function getAddresses(string $postcode): GetAddress\Response
    {
        $response = $this->adapter->get($postcode);

        $this->logUnexpectedFailures($response, $postcode);

        return $response;
    }

    private function logUnexpectedFailures(
        GetAddress\Response $response,
        string $postcode
    ): void {
        $statusCode = $response->getHttpStatus();

        if (array_key_exists($statusCode, self::RESPONSES_TO_LOG)) {
            $this->addressServiceLogger->log(
                self::RESPONSES_TO_LOG[$statusCode],
                GetAddress\Adapter::ERROR_MESSAGES[$statusCode],
                ['postcode' => $postcode, 'message' => $response->getMessage()]
            );
        }
    }

    private function handleAdapterException(
        GetAddress\Exception|\Exception $e,
        string $postcode
    ): GetAddress\Response {
        $this->addressServiceLogger->log(
            Level::Error,
            $e->getMessage(),
            ['postcode' => $postcode, 'message' => $e->getMessage()]
        );

        return new GetAddress\Response(
            [],
            HttpStatusCode::HTTP_INTERNAL_SERVER_ERROR,
            $e->getMessage()
        );
    }
}

All the logic around calling the Adapter for the data, the error handling and logging is now here instead. I've also made a point of refactoring the main try / catch so that the "what it's doing" and "handling errors" is nicely separate.

All the code for this iteration of this work is tagged as 1.10 on Github.

Righto.

--
Adam

Tuesday, 31 January 2023

TDD & Symfony & Monolog: adding some logging to that endpoint from last time

G'day:

Hopefully this one is shorter than the previous monster (TDD & Symfony: creating a small web service end point). I should perhaps have split that one in two, in hindsight (the Adapter, then the Controller). This article builds on that code.

I have create a wee webservice that I call like this: http://localhost:8008/postcode-lookup/XX200X, and that goes off and hits getaddress.io with much the same request. This web service of mine is two components:

Simple. Except I managed to write 4000-ish words on it on Sunday, somehow.

One shortfall I identified with the initial implementation is that it was just swallowing some failure situations that - whilst should not cause a problem for the consuming client of my web service - should be something I pay attention to if they occur. A quick fix for this is to chuck some logging in. And this is a good exercise as it will require me to revisit Monolog, and also I'll need to work out a) how to wire it into Symfony; b) and test my integration.

In the previous article I TDDed the first part of the exercise, and then backfilled the testing on the second part as I didn't even know how to wire things together in Symfony when I started, so I decided to spike that first. Today I'm taking a hybrid approach. I'm going to try to TDD it, but there will be some points at which I need to wrestle with Symfony/Monolog config, and I am just gonna go do that when I need to. NB: this is not to suggest any of this config is arduous: I've just forgotten how to do it, so I need to work it out again.

Here goes.

This unit test in tests/Unit/Controller/PostcodeLookupControllerTest.php describes the problem I'm trying to solve:

/**
 * @testdox It logs any issues we might need to deal with
 * @dataProvider provideCasesForLoggingTests
 */
public function testLogging(
    int $statusCode,
    string $expectedMessage,
    Level $expectedLogLevel
) {
    $testHandler = new TestHandler();

    $this->configureControllerWithTestLoggingHandler(
        $statusCode,
        $expectedMessage,
        $testHandler
    );

    $this->client->request(
        "GET",
        sprintf("/postcode-lookup/%s", TestConstants::POSTCODE_OK)
    );

    $this->assertLogEntryIsCorrect(
        $testHandler,
        $expectedLogLevel,
        $statusCode,
        $expectedMessage
    );
}

Especially with the data-provider method:

public function provideCasesForLoggingTests() : array
{
    return [
        "Unauthorized should log critical" => [
            Response::HTTP_UNAUTHORIZED,
            "Unauthorized",
            Level::Critical
        ],
        "Forbidden should log critical" => [
            Response::HTTP_FORBIDDEN,
            "Forbidden",
            Level::Critical
        ],
        "Too many requests should log critical" => [
            Response::HTTP_TOO_MANY_REQUESTS,
            "Too Many Requests",
            Level::Warning
        ],
        "Server error should log critical" => [
            Response::HTTP_INTERNAL_SERVER_ERROR,
            "Internal Server Error",
            Level::Warning
        ]
    ];
}

Basically the getaddress.io call could return each of those failures, and I wanna log when they occur. I don't care about "the client app passed an invalid postcode", but I do care if I'm using the wrong API key, or if I haven't paid my account (those're both bad, so: critical); and I also kinda wanna keen an eye on throttling issues, and unexpected server errors on their end ("good to know", so just warnings). If any of these occur, I'm still returning a usable response to the client so they don't need to care, but I keep an eye on issues I am having with getaddress.io

There's nothing interesting in that test method, but there's some stuff in the helper methods:

private function configureControllerWithTestLoggingHandler(
    int $statusCode,
    string $expectedMessage,
    TestHandler $testHandler
): void {
    $container = self::getContainer();
    $mockedAddressServiceAdapter = $this
        ->getMockBuilder(AddressService\Adapter::class)
        ->disableOriginalConstructor()
        ->onlyMethods(['get'])
        ->getMock();
    $mockedAddressServiceAdapter
        ->expects($this->once())
        ->method('get')
        ->willReturn(new AddressService\Response(
            [],
            $statusCode,
            $expectedMessage
        ));
    $container->set(AddressService\Adapter::class, $mockedAddressServiceAdapter);

    $logger = $container->get("monolog.logger.address_service");
    $logger->setHandlers([$testHandler]);
}

This shows how to grab the DI container and replace my AddressService/Adapter with a mock that returns the values I need to exercise my controller code. Remember: I will be logging in the controller here, as it's a reaction to the response it's returning. I am not changing any Adapter logic here, I am adding some logging to the controller. TBH thinking about it now, maybe this should be in Service/Address instead of the controller. Hrm. Anyhow, I can refactor later if I want (100% test coverage so I'm safe to do that).

I'm also replacing the "live" logging handler in the container with a test one. This is so I don't actually log to the file system, it instead exposes an array of log entries, which I then check out in the custom assertion function:

public function assertLogEntryIsCorrect(
    TestHandler $testHandler,
    Level $expectedLogLevel,
    int $statusCode,
    string $expectedMessage
): void {
    $logRecords = $testHandler->getRecords();
    $this->assertCount(1, $logRecords);
    $this->assertEquals($expectedLogLevel->getName(), $logRecords[0]["level_name"]);
    $this->assertEquals(
        AddressService\Adapter::ERROR_MESSAGES[$statusCode],
        $logRecords[0]["message"]
    );
    $this->assertEquals(
        [
            "postcode" => TestConstants::POSTCODE_OK,
            "message" => $expectedMessage
        ],
        $logRecords[0]["context"]
    );
}

(Full disclosure, I am writing this after I have done the full implementation so like there's that ERROR_MESSAGES const array that came out of some refactoring I did after everything was working).

This assertion is simple enough: look for one log entry, and it needs to be the level, message and context that I should expect.

Before I can run that, I need to wire in Monolog, and before I do that, I need to install it. So I'm gonna have a quick functional test for that too:

/** @testdox It writes AddressService entries to the expected log file */
public function testAddressServiceLogFile()
{
    $kernel = new Kernel("test", false);
    $kernel->boot();
    $container = $kernel->getContainer();
    $logFile = $container->getParameter("kernel.logs_dir") . "/address_service.log";

    $logger = $container->get("monolog.logger.address_service");

    $this->assertEquals($logFile, $logger->getHandlers()[0]->getUrl());
}

This doesn't test any actual writing of data to a file: I figure that's Monolog's job to look after. I'm just verifying my config stays the same as I expect it. This is not a unit test, it's just an functional test: testing I've done the config right, and no-one monkeys with it later.

Now I will permit myself to actually install Monolog; or as it is in this case: symfony/monolog-bundle.

That needs a monolog.yaml file:

monolog:
    handlers:
        address_service_log:
            type: stream
            path: '%kernel.logs_dir%/address_service.log'
            level: debug
            channels: [address_service]

    channels: [address_service]

Having added that: my functional test works, so I'm happy I've configured my log.

Now I can do my implementation of the logging in the controller. I'll show you this in parts:

public function __construct(
    AddressService\Adapter $addressServiceAdapter,
    LoggerInterface $addressServiceLogger
) {
    $this->addressServiceAdapter = $addressServiceAdapter;
    $this->logger = $addressServiceLogger;
}

I need to add the logger parameter here, but I don't need to do anything to wire it in to the DI container. Symfony works out that if I ask for a LoggerInterface, then it'll take the parameter name, lop off "Logger" and look for a channel in my monolog.yaml file that is the snake-case version of that. So the paramter name here - $addressServiceLogger will find the address_service channel in my Monolog config. That's quite cool.

public function doGet(string $postcode) : JsonResponse
{
    try {
        $response = $this->addressServiceAdapter->get($postcode);

        $this->logUnexpectedFailures($response, $postcode);

        return new JsonResponse(
            [
                'postcode' => $postcode,
                'addresses' => $response->getAddresses(),
                'message' => $response->getMessage()
            ],
            $response->getHttpStatus()
        );
    } catch (\Exception $e) {
        return new JsonResponse(
            [
                'postcode' => $postcode,
                'addresses' => [],
                'message' => $e->getMessage()
            ],
            HttpStatusCode::HTTP_INTERNAL_SERVER_ERROR
        );
    }
}

There's just that one insertion into the controller logic, and that function is also pretty simple:

private function logUnexpectedFailures(
    AddressService\Response $response,
    string $postcode
): void {
    $statusCode = $response->getHttpStatus();

    if (array_key_exists($statusCode, self::RESPONSES_TO_LOG)) {
        $this->logger->log(
            self::RESPONSES_TO_LOG[$statusCode],
            AddressService\Adapter::ERROR_MESSAGES[$statusCode],
            ['postcode' => $postcode, 'message' => $response->getMessage()]
        );
    }
}

That also refers to this lot:

private const RESPONSES_TO_LOG = [
    HttpStatusCode::HTTP_UNAUTHORIZED => Level::Critical,
    HttpStatusCode::HTTP_FORBIDDEN => Level::Critical,
    HttpStatusCode::HTTP_TOO_MANY_REQUESTS => Level::Warning,
    HttpStatusCode::HTTP_INTERNAL_SERVER_ERROR => Level::Warning
];

Level is an enum, which are new to PHP since the last time I used it. I like. I'll need to look into those in another article maybe (not least of all cos the docs are not as good as they could be).

All the code here is doing is checking if there's a case that we want to log via comparing the returned HTTP status code in that RESPONSES_TO_LOG array, and if one is there, log a message with the defined log level.

What's logged comes from the Adapter:

public const ERROR_MESSAGES = [
    HttpFoundationResponse::HTTP_UNAUTHORIZED => "API key is not valid",
    HttpFoundationResponse::HTTP_FORBIDDEN => "Permission denied",
    HttpFoundationResponse::HTTP_TOO_MANY_REQUESTS  => "Too many requests",
    HttpFoundationResponse::HTTP_INTERNAL_SERVER_ERROR => "Server error"
];

We don't really need too much detail here, we just need to know it's happened.

Again, I wonder if this is an adapter's job to define these. I think I do need a skinny wee service in between the adapter and controller here. I will do that refactor.

And that's it. I mean there are a few use statements about the place I didn't show you, but I'll link through to the code and you can look at everything, and there's really not much to it. It did take quite a while to dig out the docs for all this, given I was working with Symfony and Monolog, and testing of each, and being a newbie didn't help because some of the docs seem to assume whilst I was a n00b at (for example) Monolog then that's fine we'll document it slowly, but not thinking about the fact I also don't know the Symfony side of things either, I found the docs assume a level of knowledge that they shouldn't (at times). Being rusty with PHP (eg: not even knowing PHP did enums!) did not help. But I got there.

All the code is in tag 1.9 of this project on Github.

Righto.

--
Adam

Sunday, 29 January 2023

TDD & Symfony: creating a small web service end point

G'day:

Background

I have a real-world requirement to get a small web service written: one that wraps up calls to the getaddress.io UK postcode look-up service. For those not familiar with UK post codes, they look something like "SW1W 0NY" and the chief conceit is that each postcode "generally represents a street, part of a street…". It's very common on an address-filling form for the first step to be trying to use the postcode to return a range of correct addresses, from which the user selects their own. This reduces keying errors or vagueness on their part. We have a few forms in our web apps that hit this webservice, and we need to migrate them over to PHP.

Yesterday I took a stab at implementing this in my own PHP8 project (the one I've been building in my recent articles), and got so focused I forgot to document what I was doing and why. So today I'm revisiting the code again, and writing this thing up. Warning: it's ended up being a monster.


Implementation plan

I need to produce an endpoint along the lines of /postcode-lookup/XX200X, and from the response from that, derive a reliable list of matching addresses to pass to the UI. I'm going to implement this in two parts:

  • An adapter to sit in front of / around the HTTP call to the getaddress.io webs service. I mean "adapter" in the sense of the "Adapter Pattern"; I'll build a PHP interface to getaddress.io's HTTP one.
  • A controller handler which receives a post-code, calls the adapter, gets a result from it and determines how best to respond to the client based on that result.

There's two approaches I could take with this:

  • Top down: start with the interface of my own endpoint, nail that - mocking the adapter to start with - and then move downwards to the adapter and sort out the HTTP calls to getaddress.io.
  • Bottom up: start with the HTTP calls to getaddress.io, build and adapter around it, finish that and then create a controller that is driven by how the adapter works.

Either would work. I've chosen the latter for a coupla reasons:

Not in themselves great reasons to take that approach, but I doubt I would have bothered to do the work had I started with the Symfony side of things (reminding myself how routing, DI and controllers work). I was also only intending to do the adapter part for this exercise, even though I ultimately wired it up to the controller too (and it was dead easy as it turns out).

So. Time for some PHP code, some TDD via good old unit tests.


getaddress.io's web service

I had never used this web service before, but it's straight forward and well documented (again: documentation.getaddress.io). I clearly need an API key to access this thing, so signed-up for one of those (free, very limited usage), but they give some testing postcodes one can use to emulate the various responses one can get from their end point:

These postcodes yield both successful and unsuccessful responses to your request.

  • XX2 00X or TR19 7AA or KW1 4YT Returns a 'successful' response 200. Your request was successful.
  • XX4 00X Returns 'bad request' response400. Your postcode is not valid.
  • XX4 01X Returns 'unauthorized' response 401. Your api-key is not valid.
  • XX4 03X Returns 'forbidden' response 403. Your api-key is valid but you do not have permission to access to the resource.
  • XX4 29X Returns 'too many requests' response 429. You have made more requests than your allowed limit.
  • XX5 00X Returns 'server error' response 500. Server error, you should never see this.

Request made with these postcodes will not affect your usage.

(ibid.)

Oh, incidentally, they offer far more services than I need to use. I only need to deal with this call: https://api.getAddress.io/find/{postcode}?api-key={API key}.

Adapter

The adapter will need to deal with all those variants in some fashion, and returning the data in PHP rather than JSON; just return a PHP object that reflects success / and relevant failure states, along with a native-PHP representation of the returned address data. It can also through an exception in situations where it couldn't make sense of the response from getaddress.io. As it's only an adapter, it should not be performing any business logic; just interfacing between my app and the getaddress.io service. This is also why it's still returning HTTPish concepts rather than removing that entirely from the mix. That would be the job of a repository/service/some-other-domain-model-object sitting between it and the controller, if I chose to have one. I think an intermediary layer is overkill here, so have not bothered.

Routing and Controller

As mentioned, the public interface to my app needs to be to support GET requests to /postcode-lookup/{postcode}. The controller will be initialised with an adapter instance, the handler method will call its get method, and that will return a Response object, which will have properties for the returned addresses (if any), a message explaining potentially why no addresses were fetchable, and the HTTP status code of the upstream request. Or it - the adapter call - could throw an exception that will also need to be dealt with. The client won't be needing to know about those.


Code

API key

I've added a "missing" integration test here:

public function testEnvironmentVariables($expectedEnvironmentVariable)
{
    $this->assertNotFalse(
        getenv($expectedEnvironmentVariable),
        "Expected environment variable $expectedEnvironmentVariable to exist"
    );
}

public function expectedEnvironmentVariablesProvider() : array
{
    return [
        ["MARIADB_HOST"],
        ["MARIADB_PORT"],
        ["MARIADB_USER"],
        ["MARIADB_DATABASE"],
        ["MARIADB_ROOT_PASSWORD"],
        ["MARIADB_PASSWORD"],
        ["ADDRESS_SERVICE_API_KEY"]
    ];
}

I need all these environment variables to actually exist and be reachable by PHP, so I ought to have an integration test to make sure they do exist. I've added in the ADDRESS_SERVICE_API_KEY on there as this is what I'm about to add, and I want a failing test. You might or might not remember I'm loading my environment variables like this (in docker-compose.yml):

php:
  build:
    context: php
    dockerfile: Dockerfile

  env_file:
    - envVars.public
    - envVars.private

I have two files: the envVars.public one is in source control. It's got stuff like this in it:

MARIADB_HOST=mariadb
MARIADB_PORT=3306
MARIADB_DATABASE=db1
MARIADB_USER=user1

In contrast the envVars.private one is not in source control, as it has stuff like this in it:

# do not commit this file to your repository
MARIADB_ROOT_PASSWORD=[redacted]
MARIADB_PASSWORD=[redacted]
ADDRESS_SERVICE_API_KEY=[redacted]

You can see I've slung the ADDRESS_SERVICE_API_KEY in there. After cycling my containers, that test passes.


Adapter testing

Unit tests

I'm gonna eschew the "lets walk through the TDD steps" here as there's 10 unit tests, two integration tests, and 250 lines of test code that went through a chunk of refactoring after I got it all working. I'm just gonna show the tests - all of them - and discuss them.

/** @testdox It throws an AddressService\Exception if the getaddress.io call returns an unexpected status */
public function testThrowsExceptionOnUnexpectedStatus()
{
    $statusToReturn = Response::HTTP_NOT_IMPLEMENTED;

    $this->assertCorrectExceptionThrown(
        AddressServiceException::class,
        "Unexpected status code returned: $statusToReturn"
    );

    $adapter = $this->getTestAdapter($statusToReturn, "CONTENT_NOT_TESTED");

    $adapter->get("POSTCODE_NOT_TESTED");
}

If you refer back to the list of test postcodes above, and the HTTP response statuses they result in, 501 / not implemented is not one of them. So I am testing my handling of this situation throws an exception, because we have NFI what's going on with getaddress.io if we get one of these back from them, so there's no point continuing processing. The relevant fragment of the Adapter code is:

private const SUPPORTED_SERVICE_RESPONSES = [
    HttpFoundationResponse::HTTP_OK,
    HttpFoundationResponse::HTTP_BAD_REQUEST,
    HttpFoundationResponse::HTTP_UNAUTHORIZED,
    HttpFoundationResponse::HTTP_FORBIDDEN,
    HttpFoundationResponse::HTTP_TOO_MANY_REQUESTS,
    HttpFoundationResponse::HTTP_INTERNAL_SERVER_ERROR
];

// ...

if (!in_array($statusCode, self::SUPPORTED_SERVICE_RESPONSES)) {
    throw new AddressService\Exception("Unexpected status code returned: $statusCode");
}

Don't worry, I'll list the whole thing further down, I just wanna focus on which bits are being tested for now.

The test code above has a few helper methods I've extracted out during refactor:

private function assertCorrectExceptionThrown(string $type, string $message): void
{
    $this->expectException($type);
    $this->expectExceptionMessage($message);
}
private function getTestAdapter(int $statusToReturn, string $content): AddressServiceAdapter
{
    $client = $this->getMockedClient($statusToReturn, $content);

    return new AddressServiceAdapter("NOT_TESTED", $client);
}

private function getMockedClient(int $statusToReturn, string $content): MockObject
{
    $response = $this->getMockedResponse($statusToReturn, $content);

    $client = $this
        ->getMockBuilder(HttpClientInterface::class)
        ->disableOriginalConstructor()
        ->getMock();

    $client
        ->expects($this->once())
        ->method("request")
        ->willReturn($response);

    return $client;
}

private function getMockedResponse(int $status, string $content): MockObject
{
    $response = $this
        ->getMockBuilder(ResponseInterface::class)
        ->disableOriginalConstructor()
        ->getMock();
    $response
        ->expects($this->atLeastOnce())
        ->method("getStatusCode")
        ->willReturn($status);
    $response
        ->expects($this->any())
        ->method("getContent")
        ->willReturn($content);

    return $response;
}

The strategy here is that I am testing the Adapter's logic, and how it deals with different responses from the HTTP call. So I mock the HTTP client to return the various responses I need to exercise the Adapter logic I need to write. As you'll see from the rest of the tests, they all use the mocked client, and all the exception handling tests use that assertCorrectExceptionThrown custom assertion. This refactoring keeps the tests simple and clear, and just removes necessary boilerplate machinery.

It's just as important to refactor one's tests as it is to refactor one's source code. Test code still needs to be read by humans.

/** @testdox It throws an AddressService\Exception if the body is not JSON */
public function testThrowsExceptionOnBodyNotJson()
{
    $this->assertCorrectExceptionThrown(
        AddressServiceException::class,
        "json_decode returned [Syntax error]"
    );

    $adapter = $this->getTestAdapter(Response::HTTP_OK, "NOT_JSON");

    $adapter->get("NOT_TESTED");
}

You can see how simple this test is, and following the same approach as the previous one. Just testing a slightly different facet of the logic.

And the implementation code for this:

$body = $response->getContent(false);
$lookupResult = json_decode($body, JSON_OBJECT_AS_ARRAY);
if (json_last_error() != JSON_ERROR_NONE) {
    throw new AddressService\Exception(
        sprintf("json_decode returned [%s]", json_last_error_msg())
    );
}

I'll just give you the @testdox line from the other exception-handling tests, followed by the implementation code. The test implementations are all similar: just different return codes and body, so no point reproducing them.

/** @testdox Throws an AddressService\Exception if the body is not an array */
if (!is_array($lookupResult)) {
    throw new AddressService\Exception("Response JSON schema is not valid");
}
/** @testdox it throws an AddressService\Exception if there is no address data in the response json */
// …

/** @testdox it throws an AddressService\Exception if the addresses data is not an array */
// …

/** @testdox it throws an AddressService\Exception if the addresses data is not an array of strings */
// …

These three test different subexpressions in the one if expression:

if (
    !array_key_exists("addresses", $lookupResult)
    || !is_array($lookupResult["addresses"])
    || count(array_filter($lookupResult["addresses"], fn($address) => !is_string($address)))
) {
    throw new AddressService\Exception("Response JSON schema is not valid");
}

I'm not normally crazy about compound if expressions like that, but they're all testing variants of garbage that we don't want to pass back to the calling code.

Those are all the exception-handling tests, now for the situations where we can actually return something:

/** @testdox returns empty addresses with status code on a non-200-OK response */
public function testReturnsEmptyAddressesOnNon200Response()
{
    $statusToReturn = Response::HTTP_BAD_REQUEST;

    $adapter = $this->getTestAdapter(
        $statusToReturn,
        '{"Message": "Bad Request: Invalid postcode."}'
    );

    $result = $adapter->get("NOT_TESTED");

    $this->assertEquals($statusToReturn, $result->getHttpStatus());
    $this->assertEquals([], $result->getAddresses());
}
// …

/** @testdox it returns the message on a non-200 response */
// …

/** @testdox it returns a standard message if the non-200 response doesn't include a valid one */
// …

These are pretty similar to the exception-handling tests, just different assertions. And the implementation being tested for all of those is in the one function:

private function handleFailureResponse(
    array $lookupResult,
    int $statusCode
): AddressService\Response {
    if (array_key_exists("Message", $lookupResult) && is_string($lookupResult["Message"])) {
        return new AddressService\Response([], $statusCode, $lookupResult["Message"]);
    }
    return new AddressService\Response([], $statusCode, "No failure message returned from service");
}

And finally the happy path:

/** @testdox it returns a Response object if the response is valid */
public function testReturnsResponseObject()
{
    $statusToReturn = Response::HTTP_OK;
    $expectedAddresses = [
        "TEST_ADDRESS_1",
        "TEST_ADDRESS_2"
    ];

    $adapter = $this->getTestAdapter(
        $statusToReturn,
        sprintf('{"addresses": %s}', json_encode($expectedAddresses))
    );

    $result = $adapter->get("NOT_TESTED");

    $this->assertEquals($statusToReturn, $result->getHttpStatus());
    $this->assertEquals($expectedAddresses, $result->getAddresses());
}

Implemenation:

private function handleSuccessResponse(array $lookupResult): AddressService\Response
{
    if (
        !array_key_exists("addresses", $lookupResult)
        || !is_array($lookupResult["addresses"])
        || count(array_filter($lookupResult["addresses"], fn($address) => !is_string($address)))
    ) {
        throw new AddressService\Exception("Response JSON schema is not valid");
    }

    return new AddressService\Response(
        $lookupResult["addresses"],
        HttpFoundationResponse::HTTP_OK
    );
}

Integration tests

All of these unit tests only test the logic in the Adapter, having mocked-out the HTTP call. They don't actually test it will actually do what it's supposed to, which requires a call to the actual getaddress.io. This is where some integration tests come in.

/** @testdox Tests of the Adapter */
class AdapterTest extends TestCase
{
    private $adapter;

    protected function setUp(): void
    {
        $client = HttpClient::create();
        $this->adapter = new AddressServiceAdapter(getenv("ADDRESS_SERVICE_API_KEY"), $client);
    }

    /** @testdox It can get addresses from a valid postcode */
    public function testCanGetAddress()
    {
        $response = $this->adapter->get(TestConstants::POSTCODE_OK);

        $this->assertEquals(Response::HTTP_OK, $response->getHttpStatus());
        $this->assertGreaterThanOrEqual(1, count($response->getAddresses()));
        $this->assertEmpty($response->getMessage());
    }

    public function provideErrorTestCases(): array
    {
        return [
            [TestConstants::POSTCODE_INVALID, Response::HTTP_BAD_REQUEST],
            [TestConstants::POSTCODE_UNAUTHORIZED, Response::HTTP_UNAUTHORIZED],
            [TestConstants::POSTCODE_FORBIDDEN, Response::HTTP_FORBIDDEN],
            [TestConstants::POSTCODE_OVER_LIMIT, Response::HTTP_TOO_MANY_REQUESTS],
            [TestConstants::POSTCODE_SERVER_ERROR, Response::HTTP_INTERNAL_SERVER_ERROR]
        ];
    }

    /**
     * @testdox It returns the expected HTTP status code and a message but no addresses on an error
     * @dataProvider provideErrorTestCases
     */
    public function testReturnsExpectedHttpStatusAndMessageButNoAddressesOnError($postcode, $expectedHttpStatus)
    {
        $response = $this->adapter->get($postcode);

        $this->assertEquals($expectedHttpStatus, $response->getHttpStatus());
        $this->assertNotEmpty($response->getMessage());
        $this->assertEquals(0, count($response->getAddresses()));
    }
}

These test a happy-path response, as well as how the other "failure" responses from getaddress.io are handled. One might wonder why I don't test the "utter failure" cases here, where I throw an exception. I almost did, but then I thought that's nothing to do with the integration with getaddress.io, it's all down to how I handle that integration, and that's my code, and covered in the unit tests.

The only interesting / non-standard thing in that lot is the code to get the API key:

$this->adapter = new AddressServiceAdapter(getenv("ADDRESS_SERVICE_API_KEY"), $client);

The app never needs to know the key. Just the environment does.

Oh this code references some TestConstants:

<?php

namespace adamcameron\php8\tests\Fixtures\AddressService;

class TestConstants
{
    // provided by https://documentation.getaddress.io/ (these do not impact look-up usage)
    public const POSTCODE_OK = "XX2 00X";
    public const POSTCODE_INVALID = "XX4 00X";
    public const POSTCODE_UNAUTHORIZED = "XX4 01X";
    public const POSTCODE_FORBIDDEN = "XX4 03X";
    public const POSTCODE_OVER_LIMIT = "XX4 29X";
    public const POSTCODE_SERVER_ERROR = "XX5 00X";
}

These are separate from the integration test class as the controller tests need them too. I'll get to that.


Adapter implementation

namespace adamcameron\php8\Adapter\AddressService;

use adamcameron\php8\Adapter\AddressService;
use Symfony\Component\HttpFoundation\Response as HttpFoundationResponse;
use Symfony\Contracts\HttpClient\HttpClientInterface;
use Symfony\Contracts\HttpClient\ResponseInterface;

class Adapter
{

These are the details we need from getaddress.io:

    private const SUPPORTED_SERVICE_RESPONSES = [
        HttpFoundationResponse::HTTP_OK,
        HttpFoundationResponse::HTTP_BAD_REQUEST,
        HttpFoundationResponse::HTTP_UNAUTHORIZED,
        HttpFoundationResponse::HTTP_FORBIDDEN,
        HttpFoundationResponse::HTTP_TOO_MANY_REQUESTS,
        HttpFoundationResponse::HTTP_INTERNAL_SERVER_ERROR
    ];

    private const SERVICE_URL_TEMPLATE = "https://api.getAddress.io/find/{postcode}?api-key={api-key}";
    private string $apiKey;
    private HttpClientInterface $client;

    public function __construct(string $apiKey, HttpClientInterface $client)
    {
        $this->apiKey = $apiKey;
        $this->client = $client;
    }

This is the only public functions in here:

    public function get(string $postCode) : AddressService\Response
    {
        $response = $this->makeRequest($postCode);
        $lookupResult = $this->extractValidLookupResult($response);

        return $this->handleValidatedResponse($response, $lookupResult);
    }

Everything else is just refactoring to keep each task (ie: function) separate:

    private function makeRequest(string $postCode): ResponseInterface
    {
        $url = strtr(
            self::SERVICE_URL_TEMPLATE,
            ["{postcode}" => $postCode, "{api-key}" => $this->apiKey]
        );

        return $this->client->request("GET", $url);
    }

    private function extractValidLookupResult(ResponseInterface $response): array
    {
        $statusCode = $response->getStatusCode();

        if (!in_array($statusCode, self::SUPPORTED_SERVICE_RESPONSES)) {
            throw new AddressService\Exception("Unexpected status code returned: $statusCode");
        }

        $body = $response->getContent(false);
        $lookupResult = json_decode($body, JSON_OBJECT_AS_ARRAY);
        if (json_last_error() != JSON_ERROR_NONE) {
            throw new AddressService\Exception(
                sprintf("json_decode returned [%s]", json_last_error_msg())
            );
        }

        if (!is_array($lookupResult)) {
            throw new AddressService\Exception("Response JSON schema is not valid");
        }
        return $lookupResult;
    }

    private function handleValidatedResponse(
        ResponseInterface $response,
        array $lookupResult
    ): AddressService\Response {
        $statusCode = $response->getStatusCode();

        if ($statusCode == HttpFoundationResponse::HTTP_OK) {
            return $this->handleSuccessResponse($lookupResult);
        }
        return $this->handleFailureResponse($lookupResult, $statusCode);
    }

    private function handleSuccessResponse(array $lookupResult): AddressService\Response
    {
        if (
            !array_key_exists("addresses", $lookupResult)
            || !is_array($lookupResult["addresses"])
            || count(array_filter($lookupResult["addresses"], fn($address) => !is_string($address)))
        ) {
            throw new AddressService\Exception("Response JSON schema is not valid");
        }

        return new AddressService\Response(
            $lookupResult["addresses"],
            HttpFoundationResponse::HTTP_OK
        );
    }

    private function handleFailureResponse(
        array $lookupResult,
        int $statusCode
    ): AddressService\Response {
        if (array_key_exists("Message", $lookupResult) && is_string($lookupResult["Message"])) {
            return new AddressService\Response([], $statusCode, $lookupResult["Message"]);
        }
        return new AddressService\Response([], $statusCode, "No failure message returned from service");
    }
}

Hopefully given the explanations of all that in the tests, that's all clear. The code is pretty straight forward.

I'm using a custom exception so any consuming code can seprate out out "stuff this Adapter might throw" from "anything else that went wrong" (say for example the Symfony HttpClient spat the dummy for some reason):

namespace adamcameron\php8\Adapter\AddressService;

class Exception extends \RuntimeException
{
}

I will get murderous looks from some ppl for having a class that only holds a coupla data points and some accessors for same, but the adapter does simply return data, but it's a specific sort of data, and the consuming code ought only be reading it, not writing it. I think this is legit usage:

namespace adamcameron\php8\Adapter\AddressService;

class Response
{
    private array $addresses;
    private int $httpStatus;
    private string $message;

    public function __construct(array $addresses, int $httpStatus, string $message = "")
    {
        $this->addresses = $addresses;
        $this->httpStatus = $httpStatus;
        $this->message = $message;
    }

    public function getAddresses()
    {
        return $this->addresses;
    }

    public function getHttpStatus()
    {
        return $this->httpStatus;
    }

    public function getMessage()
    {
        return $this->message;
    }
}

If PHP allowed it, I'd probably make this an inner class of the Adapter as well. But PHP can't so just having it in the same package will have to do. I realise PHP now allows for class expressions, or anonymous classes or whatever they are, but I don't think that's quite right for this application. I just need to return an object from the adapter for the calling code to get predictable / save / checked values from.


Controller integration

I cheated with this bit and did not TDD it. I could not remember how controllers in Symfony worked, so I got the controller operational first, and then backfilled the tests. Soz.

Routing and service config

Firstly the routing (in routes.yaml):

controllers:
    resource:
        path: ../src/Controller/
        namespace: adamcameron\php8\Controller
    type: attribute

PostcodeLookup:
    path: /postcode-lookup/{postcode}
    controller: adamcameron\php8\Controller\PostcodeLookupController::doGet
    methods: GET

Then some DI config (in services.yaml):

# …
adamcameron\php8\Adapter\AddressService\Adapter:
    public: true
    arguments:
        $apiKey: '%env(ADDRESS_SERVICE_API_KEY)%'
        $client: '@Symfony\Component\HttpClient\HttpClient'

Symfony\Component\HttpClient\HttpClient:
    factory: ['\Symfony\Component\HttpClient\HttpClient','create']
# …

I only need to put the services that have non-obvious constructor argument requirements, like how I need to specify to use the environment variable for its $apiKey argument, and I need to specify to use the HttpClient::create factory method to create the $client value. Everything else is auto-wired on the basis of the type in the constructor's method signature.

Accordingly, the controller will be taking an instance of the Adapter, but Symfony can work that out from the type specification in its constructor method signature, and know to get the one I have configured here. Dead clever.


Controller implementation
namespace adamcameron\php8\Controller;

use adamcameron\php8\Adapter\AddressService\Adapter as AddressServiceAdapter;
use Symfony\Bundle\FrameworkBundle\Controller\AbstractController;
use Symfony\Component\HttpFoundation\JsonResponse;
use Symfony\Component\HttpFoundation\Response;

class PostcodeLookupController extends AbstractController
{
    private AddressServiceAdapter $addressServiceAdapter;

    public function __construct(AddressServiceAdapter $addressServiceAdapter)
    {
        $this->addressServiceAdapter = $addressServiceAdapter;
    }

    public function doGet(string $postcode)
    {
        try {
            $response = $this->addressServiceAdapter->get($postcode);

            return new JsonResponse(
                [
                    'postcode' => $postcode,
                    'addresses' => $response->getAddresses(),
                    'message' => $response->getMessage()
                ],
                $response->getHttpStatus()
            );
        } catch (\Exception $e) {
            return new JsonResponse(
                [
                    'postcode' => $postcode,
                    'addresses' => [],
                    'message' => $e->getMessage()
                ],
                Response::HTTP_INTERNAL_SERVER_ERROR
            );
        }
    }
}

As all good controller methods ought to be: simple. It receives a value from the request… passes it to the model tier to get some data… based on vagaries of the data, works out how to build the response… and sends it.

Here the conceit is that I really don't care what exceptions the Adapter might throw: I don't want to break the response, so I just swallow them and return an emptyish response, with the appropriate server error.


Controller testing

These are all in PostcodeLookupControllerTest.

private KernelBrowser $client;

protected function setUp(): void
{
    $this->client = static::createClient();
}

/** @testdox It retrieves addresses when the post code is valid */
public function testRetrievesAddressesWhenPostCodeIsValid()
{
    $this->client->request(
        "GET",
        sprintf("/postcode-lookup/%s", TestConstants::POSTCODE_OK)
    );
    $response = $this->client->getResponse();

    $this->assertEquals(
        Response::HTTP_OK,
        $response->getStatusCode()
    );

    $result = json_decode($response->getContent(), false);
    $this->assertObjectHasAttribute('addresses', $result);
    $this->assertGreaterThanOrEqual(1, count($result->addresses));
}

Happy path: it returns addresses. Hurrah. I don't check the contents of the addresses as I am happy that the Adapter code will only let legit ones through, and the content of the addresses is outwith my control: that's on getaddress.io.

/**
 * @testdox It returns an error status code and no addresses when the postcode is invalid
 * @dataProvider provideCasesForClientErrorTests
 */
public function testReturnsErrorStatusCodeAndNoAddressesWhenPostCodeIsInvalid(
    string $postcode,
    int $statusCode
) {
    $this->client->request(
        "GET",
        sprintf("/postcode-lookup/%s", $postcode)
    );
    $response = $this->client->getResponse();

    $this->assertEquals($statusCode, $response->getStatusCode());

    $result = json_decode($response->getContent(), false);
    $this->assertObjectHasAttribute('addresses', $result);
    $this->assertEmpty($result->addresses);
}

public function provideCasesForClientErrorTests() : array
{
    return [
        [TestConstants::POSTCODE_INVALID, Response::HTTP_BAD_REQUEST],
        [TestConstants::POSTCODE_UNAUTHORIZED, Response::HTTP_UNAUTHORIZED],
        [TestConstants::POSTCODE_FORBIDDEN, Response::HTTP_FORBIDDEN],
        [TestConstants::POSTCODE_OVER_LIMIT, Response::HTTP_TOO_MANY_REQUESTS],
        [TestConstants::POSTCODE_SERVER_ERROR, Response::HTTP_INTERNAL_SERVER_ERROR]
    ];
}

Superficial tests of all the expected failure situations, making sure they "work".

This next one is more interesting:

/** @testdox it returns an error status and no addresses when there's been a server error */
public function testReturnsErrorStatusCodeAndNoAddressesWhenServerError()
{
    $container = self::getContainer();
    $mockedAddressServiceAdapter = $this
        ->getMockBuilder(AddressService\Adapter::class)
        ->disableOriginalConstructor()
        ->onlyMethods(['get'])
        ->getMock();
    $mockedAddressServiceAdapter
        ->expects($this->once())
        ->method('get')
        ->willThrowException(new AddressService\Exception("TEST_ERROR_MESSAGE"));
    $container->set(AddressService\Adapter::class, $mockedAddressServiceAdapter);

    $this->client->request(
        "GET",
        sprintf("/postcode-lookup/%s", TestConstants::POSTCODE_OK)
    );

    $response = $this->client->getResponse();

    $this->assertEquals(
        Response::HTTP_INTERNAL_SERVER_ERROR,
        $response->getStatusCode()
    );

    $result = json_decode($response->getContent(), false);
    $this->assertObjectHasAttribute('addresses', $result);
    $this->assertEmpty($result->addresses);
    $this->assertEquals("TEST_ERROR_MESSAGE", $result->message);
}

Here I'm mocking the dependency injection container to mock-out the HttpClient to fake a server error on "their" end. It's good to know how to do that, and it's fairly straight forward (he says, after googling for over an hour to find out how to do it!). I'm actually thinking now that this is a functional test, not an integration test, as it's not actually hitting the external service. Hrm. I shall move it I think. Oh: I think "functional test" not "unit test" because it's cutting across a couple of concerns: Adapter logic and Controller logic, and that's where I make the (admittedly fairly arbitrary) distinction: if the "unit" is in one object: unit test. If it goes across one object into a dependent object: functional test.

I'm pretty sure that's it. Just as well. This is a monster of an article. Sorry.


Conclusions, and "next…"

I need to improve this by paying attention to a couple of those statuses back from getaddress.io, namely the 401, 403 and 429 responses. These might need remediation on our end, so I want to log something if we get those responses. And also log something in the "catch all" catch there. If there's been an unexpected error, whilst the client might not care, we should.

I'm gonna need to mess around with Monolog to do that, so I'll take that as a separate refresher exercise.

Well done, if you got this far. All the code for this is in the 1.7.3 tag on GitHub (I've linked everything relevant directly as well).

Righto.

--
Adam

Tuesday, 24 January 2023

Symfony: getting rid of App namespace and using a well-formed one

G'day:

This is a quick follow-on from the previous article, "Symfony: installing in my PHP8 container (for a second time, as it turns out)".

I was irritated that Symfony uses an invalid PSR-4 namespace for the app: App. That's like calling a variable "variable" and it's a bit shit. Plus, as indicated, it's not even valid, as a PSR-4 namespace is supposed to be <NamespaceName>(\<SubNamespaceNames>), where the first part reflects the vendor, and the (optional) second part is [something else, usually the name of the app]. So Symfony would be valid. Or Symfony/app would be valid, but just App is not valid. In my case I'm not "Symfony", so the namespace for this app should be (and is) adamcameron\php8 (OKOK, "PHP8" is not a great name for a web app, but this is my "messing around with PHP8 app", so kinda makes some sense. More than App does anyhow). </rant>.

How do I fix this? There's only a few touch points where I've needed to change references to App to adamcameron\php8:

That's it. As I had no test coverage of the console, I added one:

<?php

namespace adamcameron\php8\tests\integration;

use GuzzleHttp\Client;
use PHPUnit\Framework\TestCase;
use Symfony\Component\HttpFoundation\Response;

/** @testdox Tests of Symfony installation */
class SymfonyTest extends TestCase
{
    // …

    /** @testdox It can run the console in a shell */
    public function testSymfonyConsoleRuns()
    {
        $appRootDir = dirname(__DIR__, 2);

        exec("{$appRootDir}/bin/console --help", $output, $returnCode);

        $this->assertEquals(0, $returnCode);
    }
}

And that was it. Easy.

Righto.

--
Adam

Symfony: installing in my PHP8 container (for a second time, as it turns out)

G'day:

First up, I've messed around in the last coupla articles setting up some PHP8.2 containers (PHP: returning to PHP and setting up a PHP8 dev environment), adding MariaDB (Docker: adding a MariaDB container to my PHP & Nginx ones) etc and documenting it all… then I realised I've actually done this very exercise before! A coupla years ago when I was looking for PHP work and decided I had better get up to speed with Docker / Symfony / front-end dev. I had forgotten about a lot of it, only really remembering the VueJS part of it. Ha. Dammit. Oh well. Anyhow, that series - and it's def a series, there's a dozen articles - are all tagged with VueJs/Symfony/Docker/TDD series. Still, I am going to do a Symfony installation exercise again, cos I want it to be in this project this time. Because reasons.

Installing the baseline Symfony app

I'm working through Symfony › Installing & Setting up the Symfony Framework.

OK so I already have the Symfony CLI client installed during getting the PHP container up and running to my liking (first article linked-to above), and it seems happy:

/var/www# symfony check:requirement

Symfony Requirements Checker
~~~~~~~~~~~~~~~~~~~~~~~~~~~~

> PHP is using the following php.ini file:
/usr/local/etc/php/php.ini

> Checking Symfony requirements:

....................................

[OK] Your system is ready to run Symfony projects
Note The command console can use a different php.ini file ~~~~ than the one used by your web server. Please check that both the console and the web server are using the same PHP version and configuration. /var/www#

(note I'm running all this in a shell on the container, not on my host machine. I do not have PHP or Composer or anything like that installed on the host machine)

I already know Symfony / Composer will shit itself if I try to install Symfony in a non-empty directory, so I'm going to run the installer in /var/tmp, and I'll re-integrate the files I need into my app directory by hand.

/var/tmp# symfony new my_project_directory --version="6.2.*" --no-git
* Creating a new Symfony 6.2.* project with Composer
  (running /usr/local/bin/composer create-project symfony/skeleton /var/tmp/my_project_directory 6.2.* --no-interaction)

[OK] Your project is now ready in /var/tmp/my_project_directory
/var/tmp#

Note the --no-git there. Without that the installer wants my Git identification otherwise it can't run git init, and I don't need it to do that anyhow, so skip that bit. I found this out via trial and error.

What's installed:

/var/tmp# tree -L 2
.
`-- my_project_directory
    |-- bin
    |-- composer.json
    |-- composer.lock
    |-- config
    |-- public
    |-- src
    |-- symfony.lock
    |-- var
    `-- vendor

7 directories, 3 files
/var/tmp#

BTW I cheated and installed tree without telling you:

/var/tmp# apt-get update
[…]
Reading package lists... Done
/var/tmp# apt-get install tree

Right so a lot of that will copy across fine into my app dir, except I'll need to rename my /var/www/html to be /var/www/public. I'll also need to merge this composer.json file with my own one, as with the .gitignore. I'll just get rid of the vendor directory as I can regenerate all that with composer update. I don't currently know what the symfony.lock file is, so I'll copy it across. I presume it's something along the lines of a Symfony version of composer.lock, and is generated somehow. I'll find out later I guess.

Symfony sets the "PSR-4" namespaces to be App\\ and App\\Tests\\. I'm not having my app called "App": that's ridiculous, plus it's actually invalid according to the PSR-4 standard anyhow!

2. Specification

  1. The term "class" refers to classes, interfaces, traits, and other similar structures.
  2. A fully qualified class name has the following form:
      \<NamespaceName>(\<SubNamespaceNames>)*\<ClassName>
      
    1. The fully qualified class name MUST have a top-level namespace name, also known as a "vendor namespace".
    2. The fully qualified class name MAY have one or more sub-namespace names.

PSR-4, section 2 (part)

They're only using the \<SubNamespaceNames>.

Hopefully this is just a matter of renaming some namespace references in whatever stub / config files it's installed. However I'll actually rename my test directory to be tests to match the Symfony-idiomatic naming standard there.

File changes to make Symfony work with an existing PHP project

There was surprisingly little to do. I'll run through them, point by point


Rename of html directory to public

To fit with Nginx's default settings, I had my webroot set to be html, ie: /var/www/html. Symfony uses public, so to change that, I had to make the following file tweaks:

# docker/docker-compose.yml
version: "3"
services:
  nginx:
    build:
      context: nginx
      dockerfile: Dockerfile

    ports:
      - "8008:80"

    stdin_open: true
    tty: true

    volumes:
      - ../html:/usr/share/nginx/html/
      - ../public:/usr/share/nginx/html/

    depends_on:
      - php

Note that as far as Nginx is concerned, its web root is still /usr/share/nginx/html/, I'm just attaching the /public directory on the host machine to provide its files.

Also I've added the depends_on there because I was finding Nginx was now starting before PHP was ready, so Nginx was exiting due to not finding PHP to proxy to.

# docker/nginx/sites/default.conf
server {
    # …

    location ~ \.php$ {
        try_files $uri /index.php =404;
        fastcgi_pass php-upstream;
        fastcgi_index index.php;
        fastcgi_buffers 16 16k;
        fastcgi_buffer_size 32k;
        fastcgi_param SCRIPT_FILENAME /var/www/html/$fastcgi_script_name;
        fastcgi_param SCRIPT_FILENAME /var/www/public/$fastcgi_script_name;
        fastcgi_read_timeout 600;
        include fastcgi_params;
    }

    location ~ /\.ht {
        deny all;
    }
}

But I need to still pass the file from Nginx through to the /var/www/public/ directory now, in the PHP container.


.gitignore

/.idea
/vendor
/.phpunit.result.cache

###> symfony/framework-bundle ###
/.env.local
/.env.local.php
/.env.*.local
/config/secrets/prod/prod.decrypt.private.php
/public/bundles/
/var/
###< symfony/framework-bundle ###

###> phpunit/phpunit ###
/phpunit.xml
.phpunit.result.cache
###< phpunit/phpunit ###

A bunch of Symfony specific stuff. Seems inoccuous.


composer.json

{
    "name" : "adamcameron/php8",
    "description" : "PHP8 containers",
    "type" : "project",
    "license" : "LGPL-3.0-only",
    "require": {
        …
        "monolog/monolog": "^3.2.0",
        "symfony/console": "6.2.*",
        "symfony/dotenv": "6.2.*",
        "symfony/flex": "^2",
        "symfony/framework-bundle": "6.2.*",
        "symfony/http-client": "^6.2.2",
        "symfony/runtime": "6.2.*",
        "symfony/yaml": "6.2.*"
    },
    "require-dev": {
        "phpunit/phpunit": "^9.5.28",
        "phpmd/phpmd": "^2.13.0",
        "squizlabs/php_codesniffer": "^3.7.1"
    },
    "config": {
        "allow-plugins": {
            "symfony/flex": true,
            "symfony/runtime": true
        },
        "sort-packages": true
    },
    "autoload": {
        "psr-4": {
            "adamcameron\\php8\\": "src/",
            "App\\": "src/"
        }
    },
    "autoload-dev": {
        "psr-4": {
            "adamcameron\\php8\\testtests\\": "testtests/"
        }
    },
    "replace": {
        "symfony/polyfill-ctype": "*",
        "symfony/polyfill-iconv": "*",
        "symfony/polyfill-php72": "*",
        "symfony/polyfill-php73": "*",
        "symfony/polyfill-php74": "*",
        "symfony/polyfill-php80": "*",
        "symfony/polyfill-php81": "*"
    },
    "scripts" : {
        "test": "phpunit --testdox testtests",
        "phpmd": "phpmd src,testtests text phpmd.xml",
        "phpcs": "phpcs src testtests",
        "test-all": [
            "@test",
            "@phpmd",
            "@phpcs"
        ],
        "auto-scripts": {
            "cache:clear": "symfony-cmd",
            "assets:install %PUBLIC_DIR%": "symfony-cmd"
        },
        "post-install-cmd": [
            "@auto-scripts"
        ],
        "post-update-cmd": [
            "@auto-scripts"
        ]
    },
    "conflict": {
        "symfony/symfony": "*"
    },
    "extra": {
        "symfony": {
            "allow-contrib": false,
            "require": "6.2.*"
        }
    }
}

Here I took the stuff from the Symfony-install-generated composer.json file and merged it into mine.

I needed to continue to use Symfony's App namespace too, as it has hard dependencies on being able to find src/Kernel.php via that namespace. Suck. All my own code will continue to use a proper, correctly-defined namespace.

There's also a bit of the rename of the test directory to tests in the autoload-dev section. I also had to similarly update phpunit.xml.dist and all the namespace references. As that's just adding an s about the place, I'll not bore you with all those changes. For the completeists: it's all in Github for you to look at.


.env and .env.test

# In all environments, the following files are loaded if they exist,
# the latter taking precedence over the former:
#
#  * .env                contains default values for the environment variables needed by the app
#  * .env.local          uncommitted file with local overrides
#  * .env.$APP_ENV       committed environment-specific defaults
#  * .env.$APP_ENV.local uncommitted environment-specific overrides
#
# Real environment variables win over .env files.
#
# DO NOT DEFINE PRODUCTION SECRETS IN THIS FILE NOR IN ANY OTHER COMMITTED FILES.
# https://symfony.com/doc/current/configuration/secrets.html
#
# Run "composer dump-env prod" to compile .env files for production use (requires symfony/flex >=1.2).
# https://symfony.com/doc/current/best_practices.html#use-environment-variables-for-infrastructure-configuration

###> symfony/framework-bundle ###
APP_ENV=dev
APP_SECRET=369a2db7c3f19ae9cad11dd95777674e
###< symfony/framework-bundle ###

and

# define your env variables for the test env here
KERNEL_CLASS='App\Kernel'
APP_SECRET='$ecretf0rt3st'
SYMFONY_DEPRECATIONS_HELPER=999999
PANTHER_APP_ENV=panther
PANTHER_ERROR_SCREENSHOT_DIR=./var/error-screenshots

Symfony stuff. I will need to put those APP_SECRET values out of the codebase and put them into environment variables. They're not very "secret" sitting around in source control like that. I'll find out what they're for first. I am also looking forward to finding out what a "panther error" is. Ooh I wonder if I can change that KERNEL_CLASS reference to point to adamcameron\php8\Kernel instead, and get rid of that App namespace? Will look into that (might read my own article from last time I did this to see what I did about this…?).


symfony.lock

{
    "phpunit/phpunit": {
        "version": "9.5",
        "recipe": {
            "repo": "github.com/symfony/recipes",
            "branch": "main",
            "version": "9.3",
            "ref": "a6249a6c4392e9169b87abf93225f7f9f59025e6"
        },
        "files": [
            ".env.test",
            "phpunit.xml.dist",
            "tests/bootstrap.php"
        ]
    },
    "squizlabs/php_codesniffer": {
        "version": "3.7",
        "recipe": {
            "repo": "github.com/symfony/recipes-contrib",
            "branch": "main",
            "version": "3.6",
            "ref": "1019e5c08d4821cb9b77f4891f8e9c31ff20ac6f"
        }
    },
    "symfony/console": {
        "version": "6.2",
        "recipe": {
            "repo": "github.com/symfony/recipes",
            "branch": "main",
            "version": "5.3",
            "ref": "da0c8be8157600ad34f10ff0c9cc91232522e047"
        },
        "files": [
            "bin/console"
        ]
    },
    "symfony/flex": {
        "version": "2.2",
        "recipe": {
            "repo": "github.com/symfony/recipes",
            "branch": "main",
            "version": "1.0",
            "ref": "146251ae39e06a95be0fe3d13c807bcf3938b172"
        },
        "files": [
            ".env"
        ]
    },
    "symfony/framework-bundle": {
        "version": "6.2",
        "recipe": {
            "repo": "github.com/symfony/recipes",
            "branch": "main",
            "version": "6.2",
            "ref": "af47254c5e4cd543e6af3e4508298ffebbdaddd3"
        },
        "files": [
            "config/packages/cache.yaml",
            "config/packages/framework.yaml",
            "config/preload.php",
            "config/routes/framework.yaml",
            "config/services.yaml",
            "public/index.php",
            "src/Controller/.gitignore",
            "src/Kernel.php"
        ]
    },
    "symfony/routing": {
        "version": "6.2",
        "recipe": {
            "repo": "github.com/symfony/recipes",
            "branch": "main",
            "version": "6.2",
            "ref": "e0a11b4ccb8c9e70b574ff5ad3dfdcd41dec5aa6"
        },
        "files": [
            "config/packages/routing.yaml",
            "config/routes.yaml"
        ]
    }
}

This does seem like a Symfony-specific composer.lock file. I wonder why it needs one of its own?


bin/console

#!/usr/bin/env php
<?php

use App\Kernel;
use Symfony\Bundle\FrameworkBundle\Console\Application;

if (!is_file(dirname(__DIR__).'/vendor/autoload_runtime.php')) {
    throw new LogicException('Symfony Runtime is missing. Try running "composer require symfony/runtime".');
}

require_once dirname(__DIR__).'/vendor/autoload_runtime.php';

return function (array $context) {
    $kernel = new Kernel($context['APP_ENV'], (bool) $context['APP_DEBUG']);

    return new Application($kernel);
};

I wonder why I need a console shell script?


config directory

There's a bunch of stuff in here, some obvious, some less so, but seems to be standard frameworked-app config like routing and the like. I'll not bother repeating it here, as I have nothing to add - commentary-wise - about any of it. Go have a look on Github perhaps.


public/index.php

<?php

use App\Kernel;

require_once dirname(__DIR__).'/vendor/autoload_runtime.php';

return function (array $context) {
    return new Kernel($context['APP_ENV'], (bool) $context['APP_DEBUG']);
};

That's all that's needed in the public directory to load the app. Nice.


src/Kernel.php

<?php

namespace App;

use Symfony\Bundle\FrameworkBundle\Kernel\MicroKernelTrait;
use Symfony\Component\HttpKernel\Kernel as BaseKernel;

class Kernel extends BaseKernel
{
    use MicroKernelTrait;
}

I guess I will need to change this to be app specific at some point, otherwise I don't really know why it needs to be in my app's src directory instead of Symfony's. Time will tell.


src/Controller/.gitignore

It's created an empty .gitignore here. Unsure why. If I was to guess, there's a reference to it here…

config/routes.yaml
controllers:
    resource:
        path: ../src/Controller/
        namespace: App\Controller
    type: attribute

… and the app will error if the directory doesn't exist?


tests/bootstrap.php

<?php

use Symfony\Component\Dotenv\Dotenv;

require dirname(__DIR__).'/vendor/autoload.php';

if (file_exists(dirname(__DIR__).'/config/bootstrap.php')) {
    require dirname(__DIR__).'/config/bootstrap.php';
} elseif (method_exists(Dotenv::class, 'bootEnv')) {
    (new Dotenv())->bootEnv(dirname(__DIR__).'/.env');
}

I'm not actually loading a botostrap file in my phpunit.xml.dist yet as I didn't need one. I'm guessing I'll need this if I'm doing any functional tests that need the framework infrastructure?


tests/integration/SymfonyTest.php

This is my code, not Symfony's. I want a test to check the app is up and running. I'm checking I can curl the homepage, and get the Symfony splash screen.

<?php

namespace adamcameron\php8\tests\integration;

use GuzzleHttp\Client;
use PHPUnit\Framework\TestCase;
use Symfony\Component\HttpFoundation\Response;

/** @testdox Tests of Symfony installation */
class SymfonyTest extends TestCase
{
    /** @testdox It serves the default welcome page after installation */
    public function testSymfonyWelcomeScreenDisplays()
    {

        $client = new Client([
            'base_uri' => 'http://nginx/'
        ]);

        $response = $client->get(
            "/",
            ['http_errors' => false]
        );
        $this->assertEquals(Response::HTTP_NOT_FOUND, $response->getStatusCode());

        $html = $response->getBody();
        $document = new \DOMDocument();
        $document->loadHTML($html, LIBXML_NOWARNING | LIBXML_NOERROR); // not ideal, but libxml can't handle the SVG in the Symfony logo

        $xpathDocument = new \DOMXPath($document);

        $hasTitle = $xpathDocument->query('/html/head/title[text() = "Welcome to Symfony!"]');
        $this->assertCount(1, $hasTitle);
    }
}

That's it. Everything else is just dealing with the rename of test to tests. I'm gonna push this lot and draw a line under this article, and then come back and see if I can get rid of that App namespace.

Righto.

--
Adam

Sunday, 22 January 2023

Docker: adding a MariaDB container to my PHP & Nginx ones

G'day:

I'm pretty much just noting down how I've progressed my PHP8 test app in this one (see PHP: returning to PHP and setting up a PHP8 dev environment and other articles around this date tagged with the PHP8 label, around this date). I need a DB added to the PHP8 and Nginx containers I already have, for the next bit of stuff I want to look at.


docker/docker-compose.yml

I've added a mariadb service, and set some environment variables in the PHP8 service as well:

version: "3"
services:
  nginx:
  	# […]

  php:
    build:
      context: php
      dockerfile: Dockerfile

    environment:
      - MARIADB_DATABASE=${MARIADB_DATABASE}
      - MARIADB_USER=${MARIADB_USER}
      - MARIADB_PASSWORD=${MARIADB_PASSWORD}

    stdin_open: true
    tty: true

    volumes:
      - ..:/var/www

  mariadb:
    build:
      context: mariadb
      dockerfile: Dockerfile

    environment:
      - MARIADB_ROOT_PASSWORD=${MARIADB_ROOT_PASSWORD}
      - MARIADB_DATABASE=${MARIADB_DATABASE}
      - MARIADB_USER=${MARIADB_USER}
      - MARIADB_PASSWORD=${MARIADB_PASSWORD}

    ports:
      - "3382:3306"

    stdin_open: true
    tty: true

    volumes:
      - mariaDbData:/var/lib/mariadb

volumes:
  mariaDbData:

Those env vars are ones the MariaDB image docs on Dockerhub mandate. I'm also passing them into the PHP container so that it doeesn't need them recorded anywhere.


docker/.env

COMPOSE_PROJECT_NAME=php8

MARIADB_DATABASE=db1
MARIADB_USER=user1

# the following are to be provided to `docker-compose up`
MARIADB_ROOT_PASSWORD=
MARIADB_PASSWORD=

The only notable thing here is that - because this file is going into source control - I am not specifying the passwords; I'm just signifiying they need to exist.


docker/mariadb/Dockerfile

FROM mariadb:latest

COPY ./docker-entrypoint-initdb.d/ /docker-entrypoint-initdb.d/

CMD ["mysqld"]

EXPOSE 3306

And in ./docker-entrypoint-initdb.d/ I have these:

# docker/mariadb/docker-entrypoint-initdb.d/1.createAndPopulateTestTable.sql

USE db1;

CREATE TABLE test (
    id INT NOT NULL,
    value VARCHAR(50) CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci NOT NULL,

    PRIMARY KEY (id)
) ENGINE=InnoDB;

INSERT INTO test (id, value)
VALUES
    (101, 'Test row 1'),
    (102, 'Test row 2')
;

ALTER TABLE test MODIFY COLUMN id INT auto_increment;
# docker/mariadb/docker-entrypoint-initdb.d/2.createAndPopulateNumbersTable.sql
USE db1;

CREATE TABLE numbers (
    id INT NOT NULL,
    en VARCHAR(50) CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci NOT NULL,
    mi VARCHAR(50) CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci NOT NULL,

    PRIMARY KEY (id)
) ENGINE=InnoDB;

INSERT INTO numbers (id, en, mi)
VALUES
    (1, 'one', 'tahi'),
    (2, 'two', 'rua'),
    (3, 'three', 'toru'),
    (4, 'four', 'wha'),
    (5, 'five', 'rima'),
    (6, 'rima', 'ono')
;

ALTER TABLE numbers MODIFY COLUMN id INT auto_increment;

I'm seeding the DB with some test data. Any files dropped into that /docker-entrypoint-initdb.d/ directory in the image will be picked up by the MariaDB process when it first creates the DB (see the docs for the image again: Docker hub › MariaDB › Initializing a fresh instance).


Shell scripts

Because this rig requires one to pass passwords to docker-compose up, I've created a coupla shell scripts to remind me to do it right:

#!/bin/bash
# docker/bin/rebuildContainers.sh

# usage
# cd to directory containing docker-compose.yml
# bin/rebuildContainers.sh [DB root password] [DB user password]
# EG:
# cd ~/src/php8/docker
# bin/rebuildContainers.sh 123 1234

clear; printf "\033[3J"
docker-compose down --remove-orphans --volumes
docker-compose build --no-cache
MARIADB_ROOT_PASSWORD=$1 MARIADB_PASSWORD=$2 docker-compose up --force-recreate --detach
#!/bin/bash
# docker/bin/restartContainers.sh

# usage
# cd to directory containing docker-compose.yml
# bin/restartContainers.sh [DB root password] [DB user password]
# use same passwords as when initially calling rebuildContainers.sh

# EG:
# cd ~/src/php8/docker
# bin/restartContainers.sh 123 1234

clear; printf "\033[3J"
docker-compose stop
docker-compose up --detach nginx
MARIADB_PASSWORD=$2 docker-compose up --detach php
MARIADB_ROOT_PASSWORD=$1 MARIADB_PASSWORD=$2 docker-compose up --detach mariadb

This just saves some typing. In all honesty I am using 123 and 1234 for the respective passwords, but it doesn't matter. It's a good practice to not have passwords anywhere in source code, and this seems a reasonable way to me to make sure the values end up being where they need to be.


readme.md

I'll spare you the content here (the heading there is linked to the file), all I did to that was update my installation instructions to use the docker/bin/rebuildContainers.sh script instead of individual statements, and added a section about docker/bin/restartContainers.sh.


Test

Where would I be without a test. I've thrown a quick one together to test that the test data is there. And I will run this in conjunction with the rest of the tests, to make sure I have not caused any regressions.

// test/integration/DbTest.php

namespace adamcameron\php8\test\integration;

use Doctrine\DBAL\Connection;
use Doctrine\DBAL\DriverManager;
use PHPUnit\Framework\TestCase;
use \stdClass;

/** @testdox Tests the stub DB */
class DbTest extends TestCase
{

    /** @testdox it can fetch records from the test table */
    public function testFetchRecords()
    {
        $expectedRecords = [
            ["id" => 101, "value" => "Test row 1"],
            ["id" => 102, "value" => "Test row 2"]
        ];

        $connection = $this->getDbalConnection();
        $result = $connection->executeQuery("SELECT id, value FROM test ORDER BY id");

        $actualRecords = $result->fetchAllAssociative();

        $this->assertEquals($expectedRecords, $actualRecords);
    }

    private function getDbalConnection() : Connection
    {
        $parameters = $this->getConnectionParameters();
        return DriverManager::getConnection([
            'dbname' => $parameters->database,
            'user' => $parameters->username,
            'password' => $parameters->password,
            'host' => $parameters->host,
            'port' => $parameters->port,
            'driver' => 'pdo_mysql'
        ]);
    }

    private function getConnectionParameters() : stdClass
    {
        return (object) [
            "host" => "mariadb",
            "port" => "3306",
            "database" => getenv("MARIADB_DATABASE"),
            "username" => getenv("MARIADB_USER"),
            "password" => getenv("MARIADB_PASSWORD")
        ];
    }
}

All pretty straight forward. Note how I'm reading the DB info from the environment variables in getConnectionParameters. Take my word for it for now on the DB-handling code. I'll get to that in a different article.


That's it. Nothing insightful. I'm just documenting what I've done and why.

Righto.

--
Adam