patternphpMinor
Geocoding hook for a TYPO3 extension
Viewed 0 times
geocodingextensionforhooktypo3
Problem
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:
```
*/
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
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:
```
*/
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
Solution
You shouldn't be needing to have formatter comments like these:
Don't ignore coding standards, follow them.
You shouldn't be calling static methods like this:
That's just messy. Consider importing the parent class instead.
Instead of this messy string concatenation, consider using an array join instead:
Into
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.
Magic numbers
This one gets a header because I have a few points on it:
You shouldn't have scenarios like these:
What is at the first index of
Try to make your code intentions a little more clear.
What is
Try to make your code's intentions and reasoning a little bit more clear:
There, that's better.
Functions and variables in PHP are named in
This is common across all functions in:
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.
// @codingStandardsIgnoreLineDon'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:processGeocodingprocess_geocodingThis 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',Code Snippets
// @codingStandardsIgnoreLine\TYPO3\CMS\Core\Utility\ArrayUtility::mergeRecursiveWithOverrule(
$record,
$modifiedFields
);return $record['address'] . ' ' . $record['zip'] . ' ' . $record['city'] . ' ' . $record['country'];return [implode(",", array($record['address'], $record['zip'], $record['city'], $record['country']));\TYPO3\CMS\Core\Utility\ArrayUtility::mergeRecursiveWithOverrule(
$record,
$modifiedFields
);Context
StackExchange Code Review Q#114871, answer score: 2
Revisions (0)
No revisions yet.