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.

Sign up for GitHub

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
Open

Base modifications #144

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
ea0edc7
Merge pull request #1 from php-twinfield/master
iranl Apr 26, 2019
7ba2191
Base Modifications
iranl Jun 10, 2019
cab4a8e
Update BaseMapper.php
iranl Jun 10, 2019
830302f
Update Read.php
iranl Jun 10, 2019
ff10cfd
Update Read.php
iranl Jun 10, 2019
09693d4
Base Modifications 2
iranl Jun 10, 2019
b02c246
Base Modifications 3
iranl Jun 10, 2019
723407d
Base Modifications 4
iranl Jun 10, 2019
72ceb18
Base Modifications Request Changes
iranl Jun 10, 2019
1e0691f
Upload
iranl Jun 10, 2019
2efa087
Update BaseMapper.php
iranl Jun 10, 2019
c7e3123
Requested Changes 2
iranl Jun 11, 2019
fed96db
Update BaseMapper.php
iranl Jun 16, 2019
4c86ea9
Update BaseMapper.php
iranl Jun 20, 2019
5221c52
Update BaseMapper.php
iranl Jun 20, 2019
ff947fd
Upload
iranl Jun 23, 2019
7a8c313
More type hinting
iranl Jun 26, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 26 additions & 21 deletions src/ApiConnectors/BaseApiConnector.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,13 @@ public function __construct(AuthenticatedConnection $connection)
$this->connection = $connection;
}

public function getConnection()
/**
* Will return the current connection
*
* @return \PhpTwinfield\Secure\AuthenticatedConnection
* @throws Exception
*/
public function getConnection(): AuthenticatedConnection
{
return $this->connection;
}
Expand Down Expand Up @@ -169,53 +175,52 @@ protected function getFinderService(): FinderService

/**
* Convert options array to an ArrayOfString which is accepted by Twinfield.
*
* In some cases you are not allowed to change certain options (such as the dimtype, which should always be DEB when using CustomerApiConnector->ListAll()),
* in which case the $forcedOptions parameter will be set by the ApiConnector for this option, which will override any user settings in $options
*
* @param array $options
* @param array|null $forcedOptions
* @param array $forcedOptions
* @return array
* @throws Exception
*/
public function convertOptionsToArrayOfString(array $options, array $forcedOptions = null): array {
public function convertOptionsToArrayOfString(array $options, array $forcedOptions = []): array {
if (isset($options['ArrayOfString'])) {
return $options;
} else {
$optionsArrayOfString = array('ArrayOfString' => array());

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

$optionsArrayOfString = ['ArrayOfString' => []];

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

return $optionsArrayOfString;
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 string $objectClass
* @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 {
public function mapListAll(string $objectClass, $data, array $objectListAllTags): array {
if ($data->TotalRows == 0) {
return [];
}

$objects = [];

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

$object = new $class();
$object = new $objectClass();

if (isset($responseArrayElement->string[0])) {
$elementArray = $responseArrayElement->string;
Expand Down
8 changes: 4 additions & 4 deletions src/DomDocuments/BaseDocument.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,19 +158,19 @@ protected function appendPerformanceTypeFields(\DOMElement $element, $object): v
* @param string $tag
* @param string|null $textContent
* @param $object
* @param array|null $attributes
* @param array $methodToAttributeMap
* @return \DOMElement
*/
final protected function createNodeWithTextContent(string $tag, ?string $textContent, $object = null, array $attributes = null): \DOMElement
final protected function createNodeWithTextContent(string $tag, ?string $textContent, $object = null, array $methodToAttributeMap = []): \DOMElement
{
$element = $this->createElement($tag);

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

if (isset($object) && isset($attributes)) {
foreach ($attributes as $attributeName => $method) {
if (isset($object)) {
foreach ($methodToAttributeMap as $attributeName => $method) {
$element->setAttribute($attributeName, $object->$method());
}
}
Expand Down
97 changes: 46 additions & 51 deletions src/Mappers/BaseMapper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,25 +64,6 @@ protected static function getValueFromTag(\DOMDocument $document, string $tag):
return $element->textContent;
}

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')) {
Expand Down Expand Up @@ -110,48 +91,65 @@ protected static function getAttribute(\DOMElement $element, string $fieldTagNam
return $fieldElement->getAttribute($attributeName);
}

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 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)) {
if (false !== strtotime($value)) {
return Util::parseDate($value);
} else {
return null;
}

return null;
}

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

return null;
}

protected static function parseEnumAttribute(string $enumName, ?string $value)
protected static function parseEnumAttribute(string $enumClass, ?string $value)
{
if ($value === null) {
return null;
}

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

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

foreach ($classConstants as $classConstant) {
if ($value == $classConstant) {
willemstuursma marked this conversation as resolved.
Show resolved Hide resolved
return new $enum($value);
return new $enumClass($value);
}
}
} catch (\ReflectionException $e) {
return null;
throw new \Exception("Non existant Enum, got \"{$enumClass}\".");
}

return null;
Expand All @@ -166,54 +164,51 @@ protected static function parseMoneyAttribute(?float $value): ?Money
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 •
edited
Loading

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 •
edited
Loading

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.

Isn't it in the header part of the XML document? https://c3.twinfield.com/webservices/documentation/#/ApiReference/SalesInvoices

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)
/** @var SomeClassWithMethodsetCode $object2 */
protected static function parseObjectAttribute(string $objectClass, $object, \DOMElement $element, string $fieldTagName, array $attributes = [])
{
if ($className == "DimensionGroupDimension" || $className == "UnknownDimension") {
if ($className == "DimensionGroupDimension") {
if ($objectClass == "DimensionGroupDimension" || $objectClass == "UnknownDimension") {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should these strings be updated to class references (::class)?

Copy link
Contributor Author

@iranl iranl Jun 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

No, they cannot, as they are non existent stubs that tell this function to determine the real class from an argument contained in the XML element or sibling XML element.

parseObjectAttribute is usually used when the Twinfield entity contained in the XML is static (such as the <user> element which will always contain a user (or nothing)). In this case ::class is used when calling parseObjectAttribute

In DimensionGroupDimension, CustomerFinancials, FixedAssetTransactionLine and SupplierFinancials there are fields that can contain multiple or any of the 7 dimensionTypes. In these cases $className == "DimensionGroupDimension" or $className == "UnknownDimension" is used to tell the function to change $objectClass to the real class name in the following switch

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 this part of the parseObjectAttribute function to a different function, seems like a cleaner solution

if ($objectClass == "DimensionGroupDimension") {
$type = self::getField($element, "type", $object);
} elseif ($className == "UnknownDimension") {
} elseif ($objectClass == "UnknownDimension") {
$type = self::getAttribute($element, $fieldTagName, "dimensiontype");
}

switch ($type) {
case "ACT":
$className = "Activity";
$objectClass = \PhpTwinfield\Activity::class;
break;
case "AST":
$className = "FixedAsset";
$objectClass = \PhpTwinfield\FixedAsset::class;
break;
case "BAS":
$className = "GeneralLedger";
$objectClass = \PhpTwinfield\GeneralLedger::class;
break;
case "CRD":
$className = "Supplier";
$objectClass = \PhpTwinfield\Supplier::class;
break;
case "DEB":
$className = "Customer";
$objectClass = \PhpTwinfield\Customer::class;
break;
case "KPL":
$className = "CostCenter";
$objectClass = \PhpTwinfield\CostCenter::class;
break;
case "PNL":
$className = "GeneralLedger";
$objectClass = \PhpTwinfield\GeneralLedger::class;
break;
case "PRJ":
$className = "Project";
$objectClass = \PhpTwinfield\Project::class;
break;
default:
return null;
throw new \InvalidArgumentException("parseObjectAttribute function does not accept \"{$objectClass}\" as valid input for the \$object argument");
}
}

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

$object2 = new $class();
$object2 = new $objectClass();
$object2->setCode(self::getField($element, $fieldTagName, $object));

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

return $object2;
Expand Down
41 changes: 9 additions & 32 deletions src/Request/Read/Article.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
<?php
namespace PhpTwinfield\Request\Read;

use PhpTwinfield\Office;

/**
* Used to request a specific custom from a certain
* office and code.
*
* Used to request a specific Article from a certain office and code.
*
* @package PhpTwinfield
* @subpackage Request\Read
* @author Willem van de Sande <[email protected]>
Expand All @@ -14,15 +15,17 @@ class Article extends Read
{
/**
* Sets office and code if they are present.
*
*
* @access public
* @param Office|null $office
* @param string $code
*/
public function __construct($office = null, $code = null)
public function __construct(?Office $office = null, $code = null)
{
parent::__construct();

$this->add('type', 'article');

if (null !== $office) {
$this->setOffice($office);
}
Expand All @@ -31,30 +34,4 @@ public function __construct($office = null, $code = null)
$this->setCode($code);
}
}

/**
* Sets the office code for this Article request.
*
* @access public
* @param $office
* @return \PhpTwinfield\Request\Read\Article
*/
public function setOffice($office)
{
$this->add('office', $office);
return $this;
}

/**
* Sets the code for this Article request.
*
* @access public
* @param string $code
* @return \PhpTwinfield\Request\Read\Article
*/
public function setCode($code)
{
$this->add('code', $code);
return $this;
}
}
Loading