nemiah / phpFinTS

PHP library to communicate with FinTS/HBCI servers
MIT License
132 stars 42 forks source link

getTransactions() returns future transactions in newer version of phpFinTS #463

Open chrisabnorm opened 2 weeks ago

chrisabnorm commented 2 weeks ago

Hi,

we're currently running two instances of phpFinTS: one older one under PHP 7.3 and a newer one (the current one here on github) under PHP 8.3. We've noticed a difference between the result of getTransactions() between the two: in the older version transactions like direct debit which are already registered but will be executed some days in the future (generally shown in light grey in online banking software) are not being returned as if they would not exist. In the newer version they are returned as any other transaction and even return isBooked = true. So it's not possible to know anymore of these transactions have really been booked yet except for manually comparing their valutaDate with the current date. Is this the intended procedure to identify these transactions in the newer version or is there a flag or method to avoid them being returned by getTransactions()? We don't want to use these transactions if we cannot be sure of the direct debit to be successful.

Thank you in adavance!

Philipp91 commented 2 weeks ago

includeUnbooked defaults to false, so these transactions should not be included in the output unless you explicitly flip that bit.

PHP 7.3 is not supported by this library, so perhaps we shouldn't spend our time debugging why it behaves weirdly.

chrisabnorm commented 2 weeks ago

@Philipp91 Thank you! That parameter was what I was looking for.

One weird thing: This parameter removes the not yet booked transactions, which is correct and expected. But enabling it again the not yet booked transactions return "true" for getBooked(). Shouldn't they return "false"? This is related to the latest version of the library and PHP 8.3. That's why I was asking in the first place, because there was no differentiation possible.

The older library with PHP 7.3 definitely does not need to be debugged. It was just unexpected that the newer version would return them while the older one did not.

Philipp91 commented 2 weeks ago

Is this from GetStatementOfAccount or GetStatementOfAccountXML?

Philipp91 commented 2 weeks ago

Try inserting print_r($result); around here, then test with both PHP versions (ideally pick a date range that produces as little data as possible, just enough to observe the different behaviors). Then put the two outputs into a text differ tool, maybe you find something interesting.

chrisabnorm commented 2 weeks ago

We're using GetStatementOfAccount where I am experiencing that includeUnbooked = false removes these transactions, but using includeUnbooked = true the same transactions returns true for getBooked(). These same transactions should also return false for getBooked() right?

That's a general observation I've made with the current version. It has nothing to do with the older version.

This example transaction is not correctly not being returned for includeUnbooked = false. But when using includedUnbooked = true it's returned with booked = true:

 object(Fhp\Model\StatementOfAccount\Transaction)#1467 (16) {
        ["bookingDate":protected]=>
        object(DateTime)#1468 (3) {
          ["date"]=>
          string(26) "2024-11-14 00:00:00.000000"
          ["timezone_type"]=>
          int(3)
          ["timezone"]=>
          string(3) "UTC"
        }
        ["valutaDate":protected]=>
        object(DateTime)#1469 (3) {
          ["date"]=>
          string(26) "2024-11-18 00:00:00.000000"
          ["timezone_type"]=>
          int(3)
          ["timezone"]=>
          string(3) "UTC"
        }
        ["amount":protected]=>
        float(99.99)
        ["creditDebit":protected]=>
        string(6) "credit"
        ["isStorno":protected]=>
        bool(false)
        ["bookingCode":protected]=>
        string(3) "171"
        ["bookingText":protected]=>
        string(23) "EINZELLASTSCHRIFTEINZUG"
        ["description1":protected]=>
        string(144) "XXXXXXXXXXXXXXXXXXXXXXXXX"
        ["description2":protected]=>
        string(0) ""
        ["structuredDescription":protected]=>
        array(4) {
          ["KREF"]=>
          string(10) "1731594028"
          ["MREF"]=>
          string(17) "XXXXXXXXXXXXX"
          ["CRED"]=>
          string(18) "XXXXXXXXXXXXXX"
          ["SVWZ"]=>
          string(80) "XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX"
        }
        ["bankCode":protected]=>
        string(11) "XXXXXXXXXXXXXX"
        ["accountNumber":protected]=>
        string(20) "XXXXXXXXXXXXXXXXXXXXXXX"
        ["name":protected]=>
        string(24) "XXXXXXXXXXXXXXXXX"
        ["booked":protected]=>
        bool(true)
        ["pn":protected]=>
        int(9309)
        ["textKeyAddition":protected]=>
        int(997)
      }
    }
Philipp91 commented 1 week ago

The logic to detect (un)booked transactions was added by @nemiah in 4d0f74d8 (7 years ago). Do you happen to recall how that preg_match("/^\+\@[0-9]+\@$/", trim($day[$i])) condition is supposed to work? It seems to match sequences like +@42@. But when I look at the examples in our unit tests or in the sepcification, I see no @ anywhere, let alone a sequence like that. Also, why does the logic flip $booked=false for this and all further transactions? And why does it default to true at the beginning?

The GetStatementOfAccount implementation simply concatenates the booked and unbooked transactions into one long string. But according to the specification, the former is MT940 and the latter is MT942. Now perhaps they're similar enough (??) to run through the same parser (which we happen to call "MT940 parser" even though it understands both). But even if that assumption is true, it seems to me like the provenance of each transaction (i.e. whether it came from the one or the other blob field in the outer FinTs message) determines whether it is booked or not -- and not an arbitrary +@ sequence within the data. Also, since we concatenate booked + unbooked in this order, I think the bool in that function should, if anything, should initialize to true and then flip to false whenever we reach the unbooked section. Or it should reset to its default value for every iteration of the outer loop instead of keeping state across days.

Philipp91 commented 1 week ago

Personally, I always use this library with includeUnbooked=false, so we don't concatenate any MT942, and I ignore the Transaction.booked field entirely. I don't want unbooked transactions, because in my experience, banks change the wording, transaction IDs and sometimes even amounts when they book a transaction. So if my application performed any actions / side effects for the unbooked transactions, then I have no way of undoing those once the booked transaction appears, because I don't know which one it is. It's not always possible to match them. For booked transactions, it's much easier: they never change anymore, so hashing works. Also, the day plus their index within the list of transactions for that day uniquely identify each transaction, as they don't swap positions anymore.

After reading the code, I would think we get booked=true always, no matter what kind of transaction it is, because the +@ condition never matches anything. And includeBooked=true includes them without any way to distinguish them later.

We could:

  1. Remove Transaction->getBooked(), because it's broken. This would be a breaking change for anyone who reads this field, though as per above, that would be pointless for them anyway.
  2. Try to fix it by not conatenating the MT940 itself. Instead we should parse the two MT94* fields separately, apply booked=false/true to them respectively, and only then concatenate the resulting transaction lists before returning them to the user. This would be a breaking change for users of GetStatementOfAccount->getRawMT940() or ->getParsedMT940().
  3. Do nothing and hope for MT940 to disappear. While researching this, I found rumors online that it's going away sometime in 2025. Is that true for most banks? Should we all be migrating to GetStatementOfAccountXML instead of worrying about this?
chrisabnorm commented 1 week ago

This seems to be a bigger issue... Without knowing the code of the library: How does includeUnbooked differentiate between the two types? Or is this just a different FinTS API request that returns only the booked ones?

Philipp91 commented 1 week ago

How does includeUnbooked differentiate between the two types? Or is this just a different FinTS API request that returns only the booked ones?

It's the same request, only one request. It always returns booked and unbooked transactions from the server in separate fields of a single response. The includeUnbooked option only controls whether the unbooked field is also evaluated (in addition to the booked field) when reading the server's response.

chrisabnorm commented 1 week ago

Okay, so there is a booked field and an unbooked field. Since getBooked() does not work at all in the moment and one transaction probably cannot be booked and unbooked at the same time, wouldn't it be a solution to just use the inverted vale of the unbooked fields for getBooked()?

Philipp91 commented 1 week ago

just use the inverted vale of the unbooked fields for getBooked()?

That would be what I proposed as option (2.) above.