Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Base modifications #144

Open
wants to merge 17 commits into
base: master
Choose a base branch
from
73 changes: 72 additions & 1 deletion src/ApiConnectors/BaseApiConnector.php
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,11 @@ public function __construct(AuthenticatedConnection $connection)
$this->connection = $connection;
}

public function getConnection()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a typehint for the return type? Why is this method needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The modified InvoicesDocument addInvoice function uses the InvoiceTypeApiConnector and ArticleApiConnector to request additional information on the selected InvoiceType and Articles in the Invoice, which means it needs access to the current connection. The InvoiceApiConnector sendAll function calls the parent BaseApiConnector getConnection function and passes the connection as an argument to addInvoice.

Typehint added

{
return $this->connection;
}

/**
* @see sendXmlDocument()
* @throws Exception
Expand Down Expand Up @@ -161,4 +166,70 @@ protected function getFinderService(): FinderService
{
return $this->connection->getAuthenticatedClient(Services::FINDER());
}
}

/**
* Convert options array to an ArrayOfString which is accepted by Twinfield.
*
* @param array $options
* @param array|null $forcedOptions
willemstuursma marked this conversation as resolved.
Show resolved Hide resolved
* @return array
* @throws Exception
*/
public function convertOptionsToArrayOfString(array $options, array $forcedOptions = null): array {
willemstuursma marked this conversation as resolved.
Show resolved Hide resolved
if (isset($options['ArrayOfString'])) {
return $options;
} else {
willemstuursma marked this conversation as resolved.
Show resolved Hide resolved
$optionsArrayOfString = array('ArrayOfString' => array());
willemstuursma marked this conversation as resolved.
Show resolved Hide resolved

if (isset($forcedOptions)) {
foreach ($forcedOptions as $key => $value) {
unset($options[$key]);
$optionsArrayOfString['ArrayOfString'][] = array($key, $value);
}
}

foreach ($options as $key => $value) {
$optionsArrayOfString['ArrayOfString'][] = array($key, $value);
}

return $optionsArrayOfString;
}
}

/**
* Map the response of a listAll to an array of the requested class
*
* @param string $className
* @param $data
* @param array $objectListAllTags
willemstuursma marked this conversation as resolved.
Show resolved Hide resolved
* @return array
* @throws Exception
*/
public function mapListAll(string $className, $data, array $objectListAllTags): array {
if ($data->TotalRows == 0) {
return [];
}

$objects = [];

foreach ($data->Items->ArrayOfString as $responseArrayElement) {
$class = "\\PhpTwinfield\\" . $className;

$object = new $class();

if (isset($responseArrayElement->string[0])) {
$elementArray = $responseArrayElement->string;
} else {
$elementArray = $responseArrayElement;
}

foreach ($objectListAllTags as $key => $method) {
$object->$method($elementArray[$key]);
}

$objects[] = $object;
}

return $objects;
}
}
17 changes: 14 additions & 3 deletions src/DomDocuments/BaseDocument.php
Original file line number Diff line number Diff line change
Expand Up @@ -156,13 +156,24 @@ protected function appendPerformanceTypeFields(\DOMElement $element, $object): v
* Use this instead of createElement().
*
* @param string $tag
* @param string $textContent
* @param string|null $textContent
* @param $object
* @param array|null $attributes
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add how this argument would be used? Maybe rename to $methodToAttributeMap?

Copy link
Contributor Author

@iranl iranl Jun 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Argument is used as follows:

$this->createNodeWithTextContent('billable', $quantity->getBillableToString(), $quantity, ['locked' => 'getBillableLockedToString'])

Method renamed

* @return \DOMElement
*/
final protected function createNodeWithTextContent(string $tag, string $textContent): \DOMElement
final protected function createNodeWithTextContent(string $tag, ?string $textContent, $object = null, array $attributes = null): \DOMElement
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, the last argument can default to [].

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

{
$element = $this->createElement($tag);
$element->textContent = $textContent;

if ($textContent != null) {
$element->textContent = $textContent;
}

if (isset($object) && isset($attributes)) {
foreach ($attributes as $attributeName => $method) {
$element->setAttribute($attributeName, $object->$method());
}
}

return $element;
}
Expand Down
145 changes: 144 additions & 1 deletion src/Mappers/BaseMapper.php
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
namespace PhpTwinfield\Mappers;

use Money\Currency;
use Money\Money;
use PhpTwinfield\Message\Message;
use PhpTwinfield\Office;
use PhpTwinfield\Util;
use Webmozart\Assert\Assert;
Expand Down Expand Up @@ -62,17 +64,158 @@ protected static function getValueFromTag(\DOMDocument $document, string $tag):
return $element->textContent;
}

protected static function getField(\DOMElement $element, string $fieldTagName): ?string
protected static function getField(\DOMElement $element, string $fieldTagName, $object = null): ?string
{
$fieldElement = $element->getElementsByTagName($fieldTagName)->item(0);

if (!isset($fieldElement)) {
return null;
}

if (isset($object)) {
self::checkForMessage($object, $fieldElement);
}

if ($fieldElement->textContent === "") {
return null;
}

return $fieldElement->textContent;
}

protected static function checkForMessage($object, \DOMElement $element): void
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The object should have a typehint or implement an interface, e.g. HasMessage.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not really up to date on using interfaces and typehinting for multiple classes. Can you give me some help on this? Or is this something that we can address at a later stage?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, this method calls addMessage() on the object, so it cannot take any object right. It has to take an object that supports messages and the required method call.

So ideally, these objects would have an interface HasMessage that defines the method addMessage(Message $message): void so that you can use this typehint in the method parameter list.

You can read a bit about interfaces here: https://www.zentut.com/php-tutorial/php-interface/ Please ignore the crappy naming in the article, interfaces should have names like SupportFoo, CanBar or even IsBazable to indicate the class supports some functionality.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done and believe it works as intended now

{
if ($element->hasAttribute('msg')) {
$message = new Message();
$message->setType($element->getAttribute('msgtype'));
$message->setMessage($element->getAttribute('msg'));
$message->setField($element->nodeName);

$object->addMessage($message);
}
}

protected static function getAttribute(\DOMElement $element, string $fieldTagName, string $attributeName): ?string
{
$fieldElement = $element->getElementsByTagName($fieldTagName)->item(0);

if (!isset($fieldElement)) {
return null;
}

if ($fieldElement->getAttribute($attributeName) === "") {
return null;
}

return $fieldElement->getAttribute($attributeName);
}

protected static function parseBooleanAttribute(?string $value): ?bool
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, but there is a method in Util that does this already?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, but I like this one better as it uses built in PHP functionality and is more versatile. Could probably be moved to Util, but functionality is not 1 on 1 comparable and could then have the potential of breaking something.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not entirely convinced - I think that it for new functionality it would be hard to decide which boolean parsing function should be used. The existence of two is confusing if there is no clear guideline to pick one over the other.

Please consolidate into one method, its fine to change the one in Util, that's why we have tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved new method to Util

{
return filter_var($value, FILTER_VALIDATE_BOOLEAN);
}

protected static function parseDateAttribute(?string $value): ?\DateTimeImmutable
{
if ((bool)strtotime($value)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a false !== strtotime() would be better.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

return Util::parseDate($value);
} else {
return null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove the unneeded level of indentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

}
}

protected static function parseDateTimeAttribute(?string $value): ?\DateTimeImmutable
{
if ((bool)strtotime($value)) {
return Util::parseDateTime($value);
} else {
return null;
}
}

protected static function parseEnumAttribute(string $enumName, ?string $value)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better to pass in the entire class name, that way you can use ::class and can easily find usages.

These string classname concatination patterns make that very hard and offer little value.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

{
if ($value === null) {
return null;
}

$enum = "\\PhpTwinfield\\Enums\\" . $enumName;

try {
$classReflex = new \ReflectionClass($enum);
$classConstants = $classReflex->getConstants();

foreach ($classConstants as $classConstant) {
if ($value == $classConstant) {
willemstuursma marked this conversation as resolved.
Show resolved Hide resolved
return new $enum($value);
}
}
} catch (\ReflectionException $e) {
return null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the class does not exist, it should be an error. Its probably a typo then, so the developer should be alerted.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, now throws exception

}

return null;
}

protected static function parseMoneyAttribute(?float $value): ?Money
{
if ($value === null) {
return null;
}

return Util::parseMoney($value, new Currency('EUR'));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not good, we use Twinfield in a many currencies environment and a special treatment of EUR leads to bugs.

Please allow to currency to be passed as an argument, or remove this method.

Copy link
Contributor Author

@iranl iranl Jun 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is no specials treatment of EUR. There are no value fields in Twinfield directly linked to a currency, so whichever currency is used here has zero effect on functionality.

Money for PHP just needs some currency to construct an object. Could be any ISO currency.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if I understand this correctly - Twinfield does not pass a currency for some amounts?

Copy link
Contributor Author

@iranl iranl Jun 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Twinfield does not pass currency for any amount

Money for PHP is used when a field type in the API Documentation equals money. For example in Articles (from the API documentation):

Name Type Description
unitspriceexcl money Price excluding VAT

If you look at the XML the Money Type equals just a decimal number in Twinfield:

<unitspriceexcl>750.00</unitspriceexcl>

It does not include or involve a Currency.

The most basic usage in the library would be to just cast these decimals as float instead of using Money for PHP, but this would mean you don't get access to the added functionality (such as add, subtract, formater etc.). Money for PHP needs some currency code to construct so any currency code can be used, I used EUR because it was already used this way in Transactions. Currency operations within the money object should not be performed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another thing to realise is that Twinfield Currency codes don't have to be ISO Currency codes, they can be anything you like. If you try to set a non existing ISO currency in Money you will get an error.

I'm also ok with just removing Money for PHP entirely and treating money fields in Twinfield as floats. Is probably the cleanest solution.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@willemstuursma can you comment on this? My preference would be to remove Money for PHP entirely.

After deciding on this, I think we might be ready to continue with the next bit of #142.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @iranl. We want to keep MoneyPHP\Money for sure.

I think you should delete this method and rely on the method in Util, and always pass a currency. But it depends on the situation where it comes from.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did some more testing and MoneyPHP\Money does in fact accept non ISO Currencies as input. So that's one (non-existent) problem out of the way.

This function in BaseMapper does have a use though as the function in Util does not accept null input for value or currency (which are possibilities when reading from Twinfield). This functions filters those out (and returns null) and passes all other input to the function in Util.

So I believe the function should stay and I have modified it to take a currency string as input instead of hardcoding it to EUR.

There are some problems along the way with currencies though (such as values in the reporting currency instead of the base currency). But those have solutions in other places.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a function to BaseMapper to retrieve the Base and Reporting currency from the selected office for use in the specific mappers when working with currencies that are not linked to the currency in the specific object or when there is no currency field in the object

}

protected static function parseObjectAttribute(string $className, $object, \DOMElement $element, string $fieldTagName, array $attributes = null)
willemstuursma marked this conversation as resolved.
Show resolved Hide resolved
{
if ($className == "DimensionGroupDimension" || $className == "UnknownDimension") {
if ($className == "DimensionGroupDimension") {
$type = self::getField($element, "type", $object);
} elseif ($className == "UnknownDimension") {
$type = self::getAttribute($element, $fieldTagName, "dimensiontype");
}

switch ($type) {
case "ACT":
$className = "Activity";
break;
case "AST":
$className = "FixedAsset";
break;
case "BAS":
$className = "GeneralLedger";
break;
case "CRD":
$className = "Supplier";
break;
case "DEB":
$className = "Customer";
break;
case "KPL":
$className = "CostCenter";
break;
case "PNL":
$className = "GeneralLedger";
break;
case "PRJ":
$className = "Project";
break;
default:
return null;
willemstuursma marked this conversation as resolved.
Show resolved Hide resolved
}
}

$class = "\\PhpTwinfield\\" . $className;

$object2 = new $class();
willemstuursma marked this conversation as resolved.
Show resolved Hide resolved
$object2->setCode(self::getField($element, $fieldTagName, $object));

if (isset($attributes)) {
foreach ($attributes as $attributeName => $method) {
$object2->$method(self::getAttribute($element, $fieldTagName, $attributeName));
}
}

return $object2;
}
}
14 changes: 7 additions & 7 deletions src/Request/Catalog/Catalog.php
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,13 @@
* Abstract parent class Catalog. Catalog is the name of the request
* for LIST. List is a protected term in PHP so all instances of the word
* catalog are just a replacement.
*
*
* All aspects of LIST request require a parent element called 'list'
*
*
* The constructor makes this element, appends to the itself. All requirements
* to add new elements to this <list> dom element are done through
* the add() method.
*
*
* @package PhpTwinfield
* @subpackage Request\Catalog
* @author Leon Rowland <[email protected]>
Expand All @@ -23,7 +23,7 @@ abstract class Catalog extends \DOMDocument
/**
* Holds the <list> element that all
* additional elements should be a child of.
*
*
* @access private
* @var \DOMElement
*/
Expand All @@ -32,7 +32,7 @@ abstract class Catalog extends \DOMDocument
/**
* Creates the <list> element and adds it to the property
* listElement
*
*
* @access public
*/
public function __construct()
Expand All @@ -45,10 +45,10 @@ public function __construct()

/**
* Adds additional elements to the <list> dom element.
*
*
* See the documentation over what <list> requires to know
* what additional elements you need.
*
*
* @access protected
* @param string $element
* @param mixed $value
Expand Down
8 changes: 4 additions & 4 deletions src/Request/Catalog/Office.php
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

/**
* Used to request a list offices
*
*
* @package PhpTwinfield
* @subpackage Request\Catalog
* @author Leon Rowland <[email protected]>
Expand All @@ -14,14 +14,14 @@ class Office extends Catalog
{
/**
* Adds the only required element for this request.
*
*
* No other methods exist or are required,
*
*
* @access public
*/
public function __construct()
{
parent::__construct();
$this->add('type', 'offices');
}
}
}
2 changes: 1 addition & 1 deletion src/Request/Read/Article.php
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ public function __construct($office = null, $code = null)
* Sets the office code for this Article request.
*
* @access public
* @param int $office
* @param $office
* @return \PhpTwinfield\Request\Read\Article
*/
public function setOffice($office)
Expand Down
Loading