Update testsuite; normalizing/parsing fixes #1
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Alter OFXTest to test all .ofx files in /tests/fixtures Fix issue with oneline testOfxDataOneline/OFXUtils::normalizeOfx() - If no matching <?xml tag, parse on existence of <OFX> tags; don't return the str_replace value, replace the $ofxSgml variable and continue to normalize
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Thanks for picking this up from asgrim and continuing on!
I'm using this library for a personal project, and ran into issues with the OFX::parse() method, specifically with the parseDate(). The OFX data from my financial institution (credit card) outputs the date and offset, but not the timezone abbreviation, so the preg_match() call was failing, but then tried to pass a date like '20240501094851[-8]' to the DateTime::__construct(). I altered the preg_match a bit to allow the optional bits - that might be purely an error with the data my FI provides, but feel the changes are in line with the library's motto of "Our aim is to make the process of importing OFX files as straightforward and hassle-free as possible. "
I looked at the test suite, but noticed that not all fixtures were being tested, so it's been updated to test parsing all fixtures, including a new fixture, based on my FI export.
There was also an issue in OFXUtils::normalizeOfx() where a preg_match on /.*</OFX>/ was returning too early, making the test on tests/fixtures/ofxdata-oneline.ofx fail.
All tests are passing, but I haven't looked to see if they're actually valid OFX documents - some of the other alterations in OFX::parseBankAccount() deal with missing xml nodes (BANKACCTFROM->{ACCTID, ACCTTYPE, BRANCHID, BANKID), and simply assign 'N/A' to the BankAccount constructor for those values.