nemiah / phpFinTS

PHP library to communicate with FinTS/HBCI servers
MIT License
131 stars 40 forks source link

fix #370: GetStatementOfAccountXML receiving multiple CAMT XML files #371

Closed 2konrad closed 2 years ago

2konrad commented 2 years ago

The HICAZTest provides two test cases to test parsing of several XMLs that are returned within an HICAZ message by the bank

test 1: two segments seperated by "+" , each containing one XML (currently works fine) test 2: one of the sgments containing 2 XMLs , seperated by ":" (does not work)

2konrad commented 2 years ago

I am very happy that these tests finally passed. i am new to this and it took some time to get accustomed to the checking process...

ampaze commented 2 years ago

I think, the following line needs to be made compatible with the changes to HICAZv1, so that it always contains a flat array of XML strings

https://github.com/nemiah/phpFinTS/blob/283833cf9cd9f3df595fe596083d60ae988d25a4/lib/Fhp/Action/GetStatementOfAccountXML.php#L172

2konrad commented 2 years ago

yes you are right. There is not test case yet for getGebuchteUmsaetze of GetStatementOfAccountXML, therefor i did not notice that so far. I will look into that.

ampaze commented 2 years ago

Thanks, but this will still not result in a flat array of XML strings.

Could you do something like

foreach ($responseHicaz[0]->getGebuchteUmsaetze()->getData() as $xml) {
 $this->xml[] = $xml; 
}

? This way the consumer of the GetStatementOfAccountXML data can always deal with a single array of xml strings, independend of what a bank sends.

2konrad commented 2 years ago

Hi, i have that foreachinside the Bins getData function:

        foreach ($this->bins as $bin) {
            $xml[] = $bin->getData();
        }
        return $xml;

is that not doing the job? It returns an array of strings.

ampaze commented 2 years ago

is that not doing the job? It returns an array of strings.

And is then assigned as another array entry

$this->xml[] =

So you end up with an array of array.

2konrad commented 2 years ago

I removed the brackets - now it results in an string array

Philipp91 commented 2 years ago

It's great that you got this working!

Philipp91 commented 2 years ago

i am new to this and it took some time to get accustomed to the checking process...

An IDE with auto-formatting functionality helps, and the cs-fix command.

2konrad commented 2 years ago

In the meantime I tested GetStatementOfAccountXML with Volksbank and Sparkasse - it runs fine so far

philippdormann commented 2 years ago

Thanks so much for this PR/ your fork @2konrad , helped me a lot!

What's blocking this from being merged @Philipp91? 🤓

Philipp91 commented 2 years ago

What's blocking this from being merged

Sorry, I think there's no notification from GitHub when new commits were pushed, so it would be helpful if you could ping the thread after addressing all the comments (or tell me how to better configure notifications on my end).

philippdormann commented 2 years ago

Not a problem 😃

There might still be an issue in this PR.

A successful run looks something like this:

</Document>:@5982@<?xml version="1.0" encoding="UTF-8"?><Document xmlns="urn:iso:std:iso:20022:tech:xsd:camt.052.001.02" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="urn:iso:std:iso:20022:tech:xsd:camt.052.001.02 camt.052.001.02.xsd"><BkToCstmrAcctRpt>...</BkToCstmrAcctRpt></Document>:@3009@<?xml version="1.0" encoding="UTF-8"?><Document xmlns="urn:iso:std:iso:20022:tech:xsd:camt.052.001.02" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="urn:iso:std:iso:20022:tech:xsd:camt.052.001.02 camt.052.001.02.xsd"><BkToCstmrAcctRpt>...</BkToCstmrAcctRpt></Document>

The contents of BkToCstmrAcctRpt are obviously redacted but look great 👍

However sometimes I get this error I think this might be related to the timespan?

</Document>
    [224] => @1985@<?xml version="1.0" encoding="UTF-8"?><Document xmlns="urn:iso:std:iso:20022:tech:xsd:camt.052.001.02" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="urn:iso:std:iso:20022:tech:xsd:camt.052.001.02 camt.052.001.02.xsd"><BkToCstmrAcctRpt>...</BkToCstmrAcctRpt></Document>
    [225] => @5982@<?xml version="1.0" encoding="UTF-8"?><Document xmlns="urn:iso:std:iso:20022:tech:xsd:camt.052.001.02" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="urn:iso:std:iso:20022:tech:xsd:camt.052.001.02 camt.052.001.02.xsd"><BkToCstmrAcctRpt>...</BkToCstmrAcctRpt></Document>
)
 in /projects/sandbox/fints/vendor/nemiah/php-fints/lib/Fhp/Syntax/Parser.php:206
Stack trace:
#0 /projects/sandbox/fints/vendor/nemiah/php-fints/lib/Fhp/Syntax/Parser.php(440): Fhp\Syntax\Parser::parseDeg()
#1 /projects/sandbox/fints/vendor/nemiah/php-fints/lib/Fhp/Syntax/Parser.php(364): Fhp\Syntax\Parser::parseSegmentElement()
#2 /projects/sandbox/fints/vendor/nemiah/php-fints/lib/Fhp/Syntax/Parser.php(474): Fhp\Syntax\Parser::parseSegment()
#3 [internal function]: Fhp\Syntax\Parser::detectAndParseSegment()
#4 /projects/sandbox/fints/vendor/nemiah/php-fints/lib/Fhp/Syntax/Parser.php(491): array_map()
#5 /projects/sandbox/fints/vendor/nemiah/php-fints/lib/Fhp/Protocol/Message.php(319): Fhp\Syntax\Parser::parseSegments()
#6 /projects/sandbox/fints/vendor/nemiah/php-fints/lib/Fhp/FinTs.php(956): Fhp\Protocol\Message::parse()
#7 /projects/sandbox/fints/vendor/nemiah/php-fints/lib/Fhp/FinTs.php(307): Fhp\FinTs->sendMessage()
#8 /projects/sandbox/fints/finalStatementOfAccount.php(20): Fhp\FinTs->execute()
#9 {main}

Next Fhp\Syntax\InvalidResponseException: Invalid response from server in /projects/sandbox/fints/vendor/nemiah/php-fints/lib/Fhp/FinTs.php:959
Stack trace:
#0 /projects/sandbox/fints/vendor/nemiah/php-fints/lib/Fhp/FinTs.php(307): Fhp\FinTs->sendMessage()
#1 /projects/sandbox/fints/finalStatementOfAccount.php(20): Fhp\FinTs->execute()
#2 {main}
  thrown in /projects/sandbox/fints/vendor/nemiah/php-fints/lib/Fhp/FinTs.php on line 959

The only difference/ problem is in the separators not being parsed (e.g. [224] => @1985@)

Here's the basic code to reproduce this (using the latest commit of this PR: https://github.com/2konrad/phpFinTS/tree/f93e4c69d8190ef9b3065cef39484ce6284bec65):

<?php
$fints = require_once 'login.php';
$getSepaAccounts = \Fhp\Action\GetSEPAAccounts::create();
$fints->execute($getSepaAccounts);
if ($getSepaAccounts->needsTan()) {
    handleStrongAuthentication($getSepaAccounts);
}
$account = $getSepaAccounts->getAccounts()[1];
$from = new \DateTime('2021-01-01');
$to = new \DateTime('2022-06-16');
$getStatement = \Fhp\Action\GetStatementOfAccountXML::create($account, $from, $to);
$fints->execute($getStatement);
if ($getStatement->needsTan()) {
    handleStrongAuthentication($getStatement);
}
$bookedXML = $getStatement->getBookedXML();
print_r($bookedXML);

When changing from 2021-01-01 to 2022-01-01, everything works great.

Any idea on this?

2konrad commented 2 years ago

The reason for this problem is most likely the dimension of gebuchteCamtUmsaetze inside class GebuchteCamtUmsaetze extends BaseDeg. It has a size of 99.

/** @var Bin[] @Max(99) */ public $gebuchteCamtUmsaetze;

You are downoading more than 200 XML statements. The banks send one XML per day. If you download more than 99 days, this dimension wont be enough. Can you try to change the 99to 299?

By the way which bank is that? The banks i know only send transactions up to 3 months in the past, so 99 was enough.

nemiah commented 2 years ago

I'll happily merge this PR as soon as @Philipp91 is fine with it :relaxed:

philippdormann commented 2 years ago

thanks, @2konrad will try shortly and get back to you with the results

Bank is Sparkasse Erlangen Höchstadt Herzogenaurach, they give you 510 days of history 🤓

2konrad commented 2 years ago

@philippdormann can you try now. I increased to 299. We might even go to 510... if it works

philippdormann commented 2 years ago

Just tried with the max set to 999, loading transactions from 2021-01-22 to 2022-06-16 (510 days) now works.

going beyond that (loading from 2021-01-21), the logger seems fine - resulting array is of length 0 though.

so yeah, we might want to set the max to 510?

this is awesome 🥳

2konrad commented 2 years ago

@Philipp91 I reviewed your comments - please have a look.

2konrad commented 2 years ago

@Philipp91 reviewed as suggetsed - sorry i tried but i didnt manage to put everything in one commit. I had difficulties resetting the unchanged files ... but now it should be ok

Philipp91 commented 2 years ago

Ok thanks, now it looks all good. @nemiah I guess it's a matter of preference -- you could use "Squash and commit" or so in GitHub, which would avoid recording the "blame" on all the files that were touched and then reverted while this PR was being developed.

nemiah commented 2 years ago

Ok, done, i think :thinking: :blush:

2konrad commented 2 years ago

Thank you - my first pull request 🚀