Base modifications #144

iranl · 2019-06-10T15:15:19Z

Comprises part of the changes in #142

Add additions to current base files (e.g. BaseApiConnector, BaseDocuments, BaseMapper)
Other small modifications that have no effect on existing functionality

BaseApiConnector

Adds getConnection function which is used to call other ApiConnectors from within an ApiConnector
Adds convertOptionsToArrayOfString function to convert a simple $options array to a Twinfield acceptable ArrayOfString when using the Finder
Adds mapListAll function to allow easy mapping of listAll results to objects

BaseDocument

Extends CreateNodeWithTextContent function to optionally add attributes to a node

BaseMapper

Extends getField function to optionally allow checking for (error) messages on the field
Adds getAttribute function to read attributes of a node
Adds parse functions for Boolean, Date, DateTime, Enum Money and Object fields

Read

Adds some child functionality to Read, so these common functions can later be removed from the children

OpenIdConnectAuthentication

Extends __construct function with the optional ability to connect using a known access token and access cluster for quicker connections

BaseService

Modification to switch WSDL from __call to __soapcall because of deprecation of the former (see comments in [WIP] Rewrite/Extend all resources #142)

Other

Removal of unnecessary whitespaces and typos

Bring fork up to date

willemstuursma

Thank you so much @iranl for taking the time to create this PR.

Some generic feedback:

[] can be the default argument for array typed parameters, this allows you to remove the isset check
The usage of some method parameters is not clear to me, can you update the relevant phpdocs.
Please do not treat EUR currency special, always have it be made explicit
Full class names are superior to shortened classnames: they can be autocompleted by the IDE, type checked and their usages can easily be found
The Office type has been removed in some places, I think this is a merge error?

In my company we use PhpStorm and we rely heavily on type hints to prevent bugs, hence the focus on types. Basically, any method call that you cannot click in the IDE is frowned upon.

Please let me know if you have any questions or would like to discuss.

willemstuursma · 2019-06-10T17:34:52Z

src/ApiConnectors/BaseApiConnector.php

@@ -47,6 +47,11 @@ public function __construct(AuthenticatedConnection $connection)
        $this->connection = $connection;
    }

+    public function getConnection()


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

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

src/ApiConnectors/BaseApiConnector.php

src/Mappers/BaseMapper.php

src/Request/Read/BrowseDefinition.php

src/Request/Read/Transaction.php

src/ApiConnectors/BaseApiConnector.php

willemstuursma

Thanks @iranl! We're making good progress.

willemstuursma · 2019-06-11T08:00:02Z

src/Mappers/BaseMapper.php

    {
-        if ($className == "DimensionGroupDimension" || $className == "UnknownDimension") {
-            if ($className == "DimensionGroupDimension") {
+        if ($objectClass == "DimensionGroupDimension" || $objectClass == "UnknownDimension") {


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

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

Moved this part of the parseObjectAttribute function to a different function, seems like a cleaner solution

willemstuursma · 2019-06-11T08:04:14Z

src/Mappers/BaseMapper.php

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

        return $fieldElement->textContent;
    }
+
+    protected static function checkForMessage($object, \DOMElement $element): void


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.

willemstuursma · 2019-06-11T08:06:21Z

src/Mappers/BaseMapper.php

+        return $fieldElement->getAttribute($attributeName);
+    }
+
+    protected static function parseBooleanAttribute(?string $value): ?bool


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.

iranl · 2019-06-11T11:50:08Z

@willemstuursma Moving along quite nicely. Thanks for all the reviewing!

iranl · 2019-06-27T11:39:45Z

@willemstuursma Do you have any unsolved requests concerning the current PR?

If not, I would like to continue with the next part of #142

willemstuursma · 2019-06-28T12:17:18Z

Sorry @iranl, I noticed some changes but it is really super busy in my day job, so I didn't have time to do any further reviewing yet.

iranl · 2019-06-28T19:15:10Z

@willemstuursma no problem. Just thought I'd check you weren't waiting on more changes from me. Will continue the conversation when you have the time.

iranl · 2019-08-22T10:13:18Z

@willemstuursma I see you're actively reviewing PR's again. Will you be able to help this PR along? I am still willing and able to break up #142 as previously agreed.

ericclaeren · 2022-12-21T10:15:55Z

Hi,

I'm new to this package, running into the access token issue where it's logging in for each request while I already have a valid token, which results in quite some additional requests.

I could help you with the access token part, although it would be breaking for current implementations, we could use AccessTokenInterface to pass the access + refresh token + expires to let this package check if it's valid and requesting a new access token when it's invalid. Or use a setter for it,

Or possible add a tokenRefresh callback method to call when token is refreshed so it can be stored externally. Because there's no way to get the access token from the current connection.

Don't mean to barge in, but would love have this feature :) and can get some time from my company to help with this.

Cheers, Eric

iranl added 8 commits April 26, 2019 21:19

Merge pull request #1 from php-twinfield/master

ea0edc7

Bring fork up to date

Base Modifications

7ba2191

Update BaseMapper.php

cab4a8e

Update Read.php

830302f

Update Read.php

ff10cfd

Base Modifications 2

09693d4

Base Modifications 3

b02c246

Base Modifications 4

723407d

willemstuursma requested changes Jun 10, 2019

View reviewed changes

iranl added 3 commits June 10, 2019 22:37

Base Modifications Request Changes

72ceb18

Upload

1e0691f

Update BaseMapper.php

2efa087

willemstuursma reviewed Jun 11, 2019

View reviewed changes

src/ApiConnectors/BaseApiConnector.php Outdated Show resolved Hide resolved

willemstuursma reviewed Jun 11, 2019

View reviewed changes

Requested Changes 2

c7e3123

iranl added 5 commits June 16, 2019 22:04

Update BaseMapper.php

fed96db

Update BaseMapper.php

4c86ea9

Update BaseMapper.php

5221c52

Upload

ff947fd

More type hinting

7a8c313

ericclaeren mentioned this pull request Feb 8, 2023

Support existing accessToken on OpenIdConnectAuthentication #220

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Base modifications #144

Base modifications #144

iranl commented Jun 10, 2019 •

edited

Loading

willemstuursma left a comment

willemstuursma Jun 10, 2019

iranl Jun 10, 2019

willemstuursma left a comment

willemstuursma Jun 11, 2019

iranl Jun 11, 2019 •

edited

Loading

iranl Jun 20, 2019

willemstuursma Jun 11, 2019

willemstuursma Jun 11, 2019

iranl commented Jun 11, 2019

iranl commented Jun 27, 2019

willemstuursma commented Jun 28, 2019

iranl commented Jun 28, 2019

iranl commented Aug 22, 2019

ericclaeren commented Dec 21, 2022

Base modifications #144

Are you sure you want to change the base?

Base modifications #144

Conversation

iranl commented Jun 10, 2019 • edited Loading

willemstuursma left a comment

Choose a reason for hiding this comment

willemstuursma Jun 10, 2019

Choose a reason for hiding this comment

iranl Jun 10, 2019

Choose a reason for hiding this comment

willemstuursma left a comment

Choose a reason for hiding this comment

willemstuursma Jun 11, 2019

Choose a reason for hiding this comment

iranl Jun 11, 2019 • edited Loading

Choose a reason for hiding this comment

iranl Jun 20, 2019

Choose a reason for hiding this comment

willemstuursma Jun 11, 2019

Choose a reason for hiding this comment

willemstuursma Jun 11, 2019

Choose a reason for hiding this comment

iranl commented Jun 11, 2019

iranl commented Jun 27, 2019

willemstuursma commented Jun 28, 2019

iranl commented Jun 28, 2019

iranl commented Aug 22, 2019

ericclaeren commented Dec 21, 2022

iranl commented Jun 10, 2019 •

edited

Loading

iranl Jun 11, 2019 •

edited

Loading