5
\$\begingroup\$

I'm currently developing a TYPO3 Extension in PHP and developed a Hook. TYPO3 has the concept of Hooks where you can execute your own code as specific points. As defined in our definition of done, we want to PHPUnit-test our code base.

I did some small tests before, so I'm familiar with the basic concepts. Below you can find the class to test, and all tests for the class. With the contained annotations, it will cover 100%. But as I'm pretty new to this topic, it would be helpful to get feedback whether the code, and specially tests are okay. Do they cover what I need? Can the code be improved, especially the tests? They are messed by a huge setup.

Class to test Classes/Hook/DataMapHook.php:

<?php
namespace WebVision\WvFeuserLocations\Hook;

/*
 * This file is part of the TYPO3 CMS project.
 *
 * It is free software; you can redistribute it and/or modify it under
 * the terms of the GNU General Public License, either version 2
 * of the License, or any later version.
 *
 * For the full copyright and license information, please read the
 * LICENSE.txt file that was distributed with this source code.
 *
 * The TYPO3 project - inspiring people to share!
 */

use WebVision\WvFeuserLocations\Service\Configuration;

/**
 * Hook to process updated records.
 *
 * Will geocode adresses for fe_users.
 *
 * @author Daniel Siepmann <d.siepmann@web-vision.de>
 */
class DataMapHook
{
    /**
     * Fieldnames that trigger geo decode.
     *
     * @var array
     */
    protected $fieldsTriggerUpdate = ['address', 'city', 'country', 'zip'];

    /**
     * Table to work on. Only this table will be processed.
     *
     * @var string
     */
    protected $tableToProcess = 'fe_users';

    /**
     * Hook to add latitude and longitude to locations.
     *
     * @param string $action The action to perform, e.g. 'update'.
     * @param string $table The table affected by action, e.g. 'fe_users'.
     * @param int $uid The uid of the record affected by action.
     * @param array $modifiedFields The modified fields of the record.
     *
     * @return void
     */
    public function processDatamap_postProcessFieldArray( // @codingStandardsIgnoreLine
        $action, $table, $uid, array &$modifiedFields
    ) {
        if(!$this->processGeocoding($table, $action, $modifiedFields)) {
            return;
        }

        $geoInformation = $this->getGeoinformation(
            $this->getAddress($modifiedFields, $uid)
        );
        $modifiedFields['lat'] = $geoInformation['geometry']['location']['lat'];
        $modifiedFields['lng'] = $geoInformation['geometry']['location']['lng'];
    }

    /**
     * Check whether to fetch geo information or not.
     *
     * NOTE: Currently allwayd for fe_users, doesn't check the type at the moment.
     *
     * @param string $table
     * @param string $action
     * @param array $modifiedFields
     *
     * @return bool
     */
    protected function processGeocoding($table, $action, array $modifiedFields)
    {
        // Do not process if foreign table, unintended action,
        // or fields were changed explicitly.
        if ($table !== $this->tableToProcess || $action !== 'update') {
            return false;
        }

        // If fields were cleared we force geocode
        if (isset($modifiedFields['lat']) && $modifiedFields['lat'] === ''
            && isset($modifiedFields['lng']) && $modifiedFields['lng'] === ''
        ) {
            return true;
        }

        // Only process if one of the fields was updated, containing new information.
        foreach (array_keys($modifiedFields) as $modifiedFieldName) {
            if (in_array($modifiedFieldName, $this->fieldsTriggerUpdate)) {
                return true;
            }
        }

        return false;
    }

    /**
     * Get address of the given record.
     *
     * Merges information from database with modified ones.
     *
     * @param array $modifiedFields Modified fields for overwrite.
     * @param int $uid Uid to fetch record from db.
     *
     * @return string
     */
    protected function getAddress(array $modifiedFields, $uid)
    {
        $record = $this->getDatabaseConnection()
            ->exec_SELECTgetSingleRow(
                implode(',', $this->fieldsTriggerUpdate),
                $this->tableToProcess,
                'uid = ' . (int) $uid
            );

        \TYPO3\CMS\Core\Utility\ArrayUtility::mergeRecursiveWithOverrule(
            $record,
            $modifiedFields
        );

        return $record['address'] . ' ' . $record['zip'] . ' ' . $record['city'] . ' ' . $record['country'];
    }

    /**
     * Get geo information from Google for given address.
     *
     * @param string $address
     *
     * @return array
     */
    protected function getGeoinformation($address)
    {
        $response = json_decode($this->getGoogleGeocode($address), true);

        if ($response['status'] === 'OK') {
            return $response['results'][0];
        }

        throw new \Exception(
            'Could not geocode address: "' . $address . '". Return status was: "' . $response['status'] . '".',
            1450279414
        );
    }

    /**
     * Get pure geocode API result from Google.
     *
     * @codeCoverageIgnore Just wrap Google API.
     *
     * @param string $address
     *
     * @return string
     */
    protected function getGoogleGeocode($address)
    {
        return \TYPO3\CMS\Core\Utility\GeneralUtility::getUrl(
            'https://maps.googleapis.com/maps/api/geocode/json?address=' .
            urlencode($address) . '&key=' .
            Configuration::getGoogleApiKey()
        );
    }

    /**
     * Get TYPO3 database connection.
     *
     * @codeCoverageIgnore Just wraps TYPO3 API.
     *
     * @return \TYPO3\CMS\Core\Database\DatabaseConnection
     */
    protected function getDatabaseConnection()
    {
        return $GLOBALS['TYPO3_DB'];
    }
}

First test class Tests/Unit/Hook/DataMapHookNotExecutedTest.php:

<?php
namespace WebVision\Tests\Unit\Hook;

/*
 * This file is part of the TYPO3 CMS project.
 *
 * It is free software; you can redistribute it and/or modify it under
 * the terms of the GNU General Public License, either version 2
 * of the License, or any later version.
 *
 * For the full copyright and license information, please read the
 * LICENSE.txt file that was distributed with this source code.
 *
 * The TYPO3 project - inspiring people to share!
 */

/**
 * Test different circumstances in which the hook should not be executed.
 *
 * @author Daniel Siepmann <coding@daniel-siepmann.de>
 */
class DataMapHookNotExecutedTest extends \TYPO3\CMS\Core\Tests\UnitTestCase
{
    protected $subject;

    public function setUp()
    {
        $this->subject = new \WebVision\WvFeuserLocations\Hook\DataMapHook;
    }

    /**
     * @test
     */
    public function dontProcessForeignTables()
    {
        $expectedResult = ['title' => 'test'];
        $modifiedFields = $expectedResult;

        $this->subject->processDatamap_postProcessFieldArray(
            'update',
            'pages',
            5,
            $modifiedFields
        );

        $this->assertEquals(
            $expectedResult,
            $modifiedFields,
            'Processing "pages" table modified the fields.'
        );
    }

    /**
     * @test
     */
    public function dontProcessFurtherActions()
    {
        $expectedResult = ['title' => 'test'];
        $modifiedFields = $expectedResult;

        $this->subject->processDatamap_postProcessFieldArray(
            'new',
            'fe_users',
            5,
            $modifiedFields
        );

        $this->assertEquals(
            $expectedResult,
            $modifiedFields,
            'Processing "edit" action modified the fields.'
        );
    }

    /**
     * @test
     */
    public function dontProcessOnUnimportantInformation()
    {
        $expectedResult = ['title' => 'test'];
        $modifiedFields = $expectedResult;

        $this->subject->processDatamap_postProcessFieldArray(
            'update',
            'fe_users',
            5,
            $modifiedFields
        );

        $this->assertEquals(
            $expectedResult,
            $modifiedFields,
            'Processing unimportant fields modified the fields.'
        );
    }
}

Second test class Tests/Unit/Hook/DataMapHookExecutedTest.php:

<?php
namespace WebVision\Tests\Unit\Hook;

/*
 * This file is part of the TYPO3 CMS project.
 *
 * It is free software; you can redistribute it and/or modify it under
 * the terms of the GNU General Public License, either version 2
 * of the License, or any later version.
 *
 * For the full copyright and license information, please read the
 * LICENSE.txt file that was distributed with this source code.
 *
 * The TYPO3 project - inspiring people to share!
 */

/**
 * Test different kinds of calls where the hook get's executed.
 *
 * @author Daniel Siepmann <coding@daniel-siepmann.de>
 */
class DataMapHookExecutedTest extends \TYPO3\CMS\Core\Tests\UnitTestCase
{
    protected $subject;

    public function setUp()
    {
        $dbConnection = $this->getMock(
            '\TYPO3\CMS\Core\Database\DatabaseConnection',
            ['exec_SELECTgetSingleRow']
        );
        $dbConnection->expects($this->once())
            ->method('exec_SELECTgetSingleRow')
            ->will(self::returnValue([
                'address' => 'An der Eickesmühle 38',
                'zip' => '41238',
                'city' => 'Mönchengladbach',
                'country' => 'Germany',
            ]));
        $this->subject = $this
            ->getMockBuilder('\WebVision\WvFeuserLocations\Hook\DataMapHook')
            ->setMethods(['getDatabaseConnection', 'getGoogleGeocode'])
            ->getMock();
        $this->subject->expects($this->once())
            ->method('getDatabaseConnection')
            ->will(self::returnValue($dbConnection));
        $this->subject->expects($this->once())
            ->method('getGoogleGeocode')
            ->with('An der Eickesmühle 38 41238 Mönchengladbach Germany')
            ->will(self::returnValue(
                json_encode([
                    'status' => 'OK',
                    'results' => [
                        [
                            'geometry' => [
                                'location' => [
                                    'lat' => 18.23,
                                    'lng' => 1.23,
                                ]
                            ]
                        ]
                    ]
                ])
            ));
    }

    /**
     * @test
     */
    public function updateRecordWithGeocodeOnForcedUpdate()
    {
        $expectedResult = ['lat' => 18.23, 'lng' => 1.23];
        // Force update by removing geocode information
        $modifiedFields = ['lat' => '', 'lng' => ''];

        $this->subject->processDatamap_postProcessFieldArray(
            'update',
            'fe_users',
            5,
            $modifiedFields
        );

        $this->assertEquals(
            $expectedResult,
            $modifiedFields,
            'Did not update modified fields with geocoding information for persistence in DB, forced by empty geocode.'
        );
    }

    /**
     * @test
     */
    public function updateRecordWithGeocodeOnUpdate()
    {
        $expectedResult = ['lat' => 18.23, 'lng' => 1.23, 'address' => 'An der Eickesmühle 38'];
        // Trigger update with change in address.
        $modifiedFields = ['address' => 'An der Eickesmühle 38'];

        $this->subject->processDatamap_postProcessFieldArray(
            'update',
            'fe_users',
            5,
            $modifiedFields
        );

        $this->assertEquals(
            $expectedResult,
            $modifiedFields,
            'Did not update modified fields with geocoding information for persistence in DB, triggered with new address.'
        );
    }
}

Third test class Tests/Unit/Hook/DataMapHookExceptionTest.php:

<?php
namespace WebVision\Tests\Unit\Hook;

/*
 * This file is part of the TYPO3 CMS project.
 *
 * It is free software; you can redistribute it and/or modify it under
 * the terms of the GNU General Public License, either version 2
 * of the License, or any later version.
 *
 * For the full copyright and license information, please read the
 * LICENSE.txt file that was distributed with this source code.
 *
 * The TYPO3 project - inspiring people to share!
 */

/**
 * Test exceptions within hook.
 *
 * @author Daniel Siepmann <coding@daniel-siepmann.de>
 */
class DataMapHookExceptionTest extends \TYPO3\CMS\Core\Tests\UnitTestCase
{
    protected $subject;

    public function setUp()
    {
        $dbConnection = $this->getMock(
            '\TYPO3\CMS\Core\Database\DatabaseConnection',
            ['exec_SELECTgetSingleRow']
        );
        $dbConnection->expects($this->once())
            ->method('exec_SELECTgetSingleRow')
            ->will(self::returnValue([
                'address' => 'An der Eickesmühle 38',
                'zip' => '41238',
                'city' => 'Mönchengladbach',
                'country' => 'Germany',
            ]));
        $this->subject = $this
            ->getMockBuilder('\WebVision\WvFeuserLocations\Hook\DataMapHook')
            ->setMethods(['getDatabaseConnection', 'getGoogleGeocode'])
            ->getMock();
        $this->subject->expects($this->once())
            ->method('getDatabaseConnection')
            ->will(self::returnValue($dbConnection));
        $this->subject->expects($this->once())
            ->method('getGoogleGeocode')
            ->with('An der Eickesmühle 38 41238 Mönchengladbach Germany')
            ->will(self::returnValue(
                json_encode([
                    'status' => 'Failure',
                ])
            ));
    }

    /**
     * @test
     *
     * @expectedException \Exception
     * @expectedExceptionMessageRegExp #Could not geocode address.* "Failure".#
     * @expectedExceptionCode 1450279414
     */
    public function throwExceptionOnNonSuccessfullReturn()
    {
        $modifiedFields = ['address' => 'An der Eickesmühle 38'];

        $this->subject->processDatamap_postProcessFieldArray(
            'update',
            'fe_users',
            5,
            $modifiedFields
        );
    }
}
\$\endgroup\$
2
\$\begingroup\$

You shouldn't be needing to have formatter comments like these:

// @codingStandardsIgnoreLine

Don't ignore coding standards, follow them.


You shouldn't be calling static methods like this:

    \TYPO3\CMS\Core\Utility\ArrayUtility::mergeRecursiveWithOverrule(
        $record,
        $modifiedFields
    );

That's just messy. Consider importing the parent class instead.


Instead of this messy string concatenation, consider using an array join instead:

return $record['address'] . ' ' . $record['zip'] . ' ' . $record['city'] . ' ' . $record['country'];

Into

return [implode(",", array($record['address'], $record['zip'], $record['city'], $record['country']));

Adding to the above point, why do you have no problem concatenating all those strings on one line, but you have a problem with two parameters on one line?

Stick to a standard throughout please.

\TYPO3\CMS\Core\Utility\ArrayUtility::mergeRecursiveWithOverrule(
    $record,
    $modifiedFields
);

Magic numbers

This one gets a header because I have a few points on it:

You shouldn't have scenarios like these:

    if ($response['status'] === 'OK') {
        return $response['results'][0];
    }

What is at the first index of $response['results']? Why do you need to return it?

Try to make your code intentions a little more clear.

    throw new \Exception(
        'Could not geocode address: "' . $address . '". Return status was: "' . $response['status'] . '".',
        1450279414
    );

What is 1450279414?

Try to make your code's intentions and reasoning a little bit more clear:

$cupsOfCoffeeIveDrank = 1450279414;
throw new \Exception(
    'Could not geocode address: "' . $address . '". Return status was: "' . $response['status'] . '".',
    $cupsOfCoffeeIveDrank
);

There, that's better.


Functions and variables in PHP are named in snake_case, not camelCase:

processGeocoding
process_geocoding

This is common across all functions in:

    $expectedResult = ['title' => 'test'];
    $modifiedFields = $expectedResult;

Consider moving it to a more accessible place so it doesn't need to be replicated.


You shouldn't be having actual data hardcoded in, pass it in from a database or config file for better maintainability.

        ->will(self::returnValue([
            'address' => 'An der Eickesmühle 38',
            'zip' => '41238',
            'city' => 'Mönchengladbach',
            'country' => 'Germany',
|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ Thanks for your work and feedback. While I follow most of your feedback, and will modify the code accordingly, there are some points I can't: Coding Standards I follow PSR-2, but the API from TYPO3, calls this methods, so how can I rename it? So using namespace import feature for a method, It's no longer a call to static method, but a function? And much shorter? How to overcome external data structure like the response from Google? PSR-2 defined lowerCamelCase methods. I'll change everything else, if you update the answer for the above points it would be awesome. \$\endgroup\$ – Daniel Dec 25 '15 at 10:49

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.