Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
base: master
Are you sure you want to change the base?
Base modifications #144
Changes from 9 commits
ea0edc7
7ba2191
cab4a8e
830302f
ff10cfd
09693d4
b02c246
723407d
72ceb18
1e0691f
2efa087
c7e3123
fed96db
4c86ea9
5221c52
ff947fd
7a8c313
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
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
.There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 methodaddMessage(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 evenIsBazable
to indicate the class supports some functionality.There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?There was a problem hiding this comment.
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.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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):
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.There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
)?There was a problem hiding this comment.
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 callingparseObjectAttribute
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 followingswitch
There was a problem hiding this comment.
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