AbcAeffchen / Sephpa

PHP class to create SEPA xml files for credit transfer and direct debit
GNU Lesser General Public License v3.0
71 stars 31 forks source link

checkAndSanitize false -- different xml structure #6

Closed sargac closed 7 years ago

sargac commented 7 years ago

Hello, I've noticed that by merely turning checkAndSanitize value to false, the whole XML structure becomes modified, and does not validate anymore.

For instance, when checkAndSanitize is true (default), there is a s <BtchBookg>false</BtchBookg> tag, and <AmdmntInd>false</AmdmntInd> and when checkAndSanitize is false, there is no BtchBookg tag at all, and Amdmntlnd ist empty -- <Amdmntlnd/>.

Please advise.

AbcAeffchen commented 7 years ago

Hi,

I looked into it. I think you can fix this by providing 'amdmntInd' => 'false' and 'btchbookg' => 'false' in your input. I assume that you did not provide this at the moment, did you? If this is the case, you should use 'btchbookg' => 'true' instead. This also means that there might be an other bug, that sets the wrong default value for BtchBookg. Maybe you can provide a little example code that demonstrates the bug, so I can better see whats going on. Also include the Sepa File Version you use.

Also may I ask why you want to disable checkAndSanitize?

I will fix this issue in the next days, so you can use the dev-master version until 1.4.0 is released.

sargac commented 7 years ago

Hi, thank you very much for quick feedback! I've tried with your updated version, and I face the same issue.

checkAndSanitize test

I uploaded 2 files, one with checkAndSanitize set to FALSE and the other set to TRUE (these are XML files, renamed to TXT in order to be able to post them here, and with somewhat modified data):

  1. checkAndSanitize-FALSE.txt
  2. checkAndSanitize-TRUE.txt

I tried these Sephpa versions: 1.3.0 (Stable), Master (a day or two ago), Master (the last one).

what I am trying to achieve

I am using Sephpa for Austrian Sepa DD scheme (ISO:pain.008.001.02:APC:STUZZA:payments:003 ISO.pain.008.001.02.austrian.003.xsd) and since BIC is now optional, we tried to omit the BIC number from generated XML file.

In that case (without BIC number), Sephpa (probably) correctly adds <FinInstnId><Othr><Id>NOTPROVIDED</Id></Othr></FinInstnId> instead of a BIC number, but Austrian rules slightly differ, or they may have a custom solution, so the XML in this format is not accepted by the banks. The BIC field must stay, and there should be a value of NOTAVAIL. (I confirmed that with our bank Raiffeisen and I saw that in the UniCreditBank manual on page 29: https://www.hypovereinsbank.de/content/dam/hypovereinsbank/unternehmer/pdf/sepa/HVB_SEPA_Techn_Spezifikationen_Sept_2013.pdf)

So, using checkAndSanitize that is not possible to achieve, because NOTAVAIL BIC doesn't exist, and doesn't belong to any IBAN number. That's why I tried to achieve that by setting the checkAndSanitize to false, because users enter only IBAN, which is checked at the input time, and other values are software generated and within the rules, so we could disable checkAndSanitize at the XML generation.

Any help in achieving that would be appreciated.

AbcAeffchen commented 7 years ago

Hi,

Thanks for providing the XML files and for the PDF from the HVB. As far as I can see that, both XML files are valid for German banks. The only problem with the files is, that IBANs and BICs are not valid at the moment, probably because you used some test data.

The document from the HVB reads like using NOTAVAIL is the standard for all countries but Germany. But I think turning checkAndSanitize off and providing NOTAVAIL as BIC should work. If not, it would be nice if you can post the code to generate your files.

I will add NOTAVAIL as a valid BIC for all IBANs, so you also can turn on checkAndSanitize. I will also add a function that automatically uses NOTAVAIL if the file is generated for an Austrian bank, so you don't have to provide NOTAVAIL your self.

sargac commented 7 years ago

Hi, thanks a lot for a feedback. I did use test data (by mistake I said I didn't), and it is true, that by turning the checkAndSanitize (false) the NOTAVAIL BIC value appears in XML, but then the structure of XML changes (as I explained in the my first post), and the banking software doesn't want to accept the generated XML file. I compared two files, and I noticed these two differences, mentioned in post #1.

I am going to prepare a test case and the code used to generate the files shortly, but I would like to ask you, if there is an option for you to add the Austrian XML Scheme to the list of Supported file versions.

I am attaching the current version of php files from src/ and scr/payment-collections (renamed to txt) we used until now (probably not the right solution, but it helped us), if that can be of help.

  1. SepaDirectDebit00800102Austrian003.txt
  2. SephpaDirectDebit.txt

Best!

sargac commented 7 years ago

Hi, I'm attaching the sephpa code I'm using (sephpaXML.php, renamed to txt) with its 3 outputs (xml files, renamed to txt). I've tested it on a new master Sephpa version.

Here, the difference between versions with checkAndSanitize set to true vs. false can be noted. In these examples I've added 'amdmntInd' => false, but I didn't notice any difference using this value or not.

  1. sephpaXML.txt
  2. check_false-bic_notavail.txt
  3. check_false-bic_valid.txt
  4. check_true-bic_valid.txt
AbcAeffchen commented 7 years ago

I will look into it as soon as possible. I have already looked into your SepaDirectDebit00800102Austrian003.txt and SephpaDirectDebit.txt. Comparing this to my files made me aware of two smaller issues no one mentioned yet. I fixed them already and currently I write some more unit tests.

To the problems you mentioned:

Did your bank send you an error message why your file is not valid? I could think of two reasons:

  1. They do not accept the file version pain.008.001.02 but only pain.008.001.02.austrian.003
  2. You set BtchBookg to false, but the default is true and maybe you have to talk to your bank first if you want to set it to false.

At the moment I try to avoid adding the Austrian XML scheme to the supported XML versions, but I take a quick look into pain.008.001.02.austrian.003.

I also wanted to mention that you can join the Gitter chat for Sephpa. If you experiment yourself and find something interesting it is maybe easier.

AbcAeffchen commented 7 years ago

Please have a look at the current dev-master.

I would like to ask you to verify the following:

I hope this works for you.

sargac commented 7 years ago

Thank you very much for thorough help. I needed some time to check and answer all that, so here's a brief summary.

Summary

I can verify the last commit to be working correctly. The missing BIC is correctly replaced by NOTAVAIL value, and the newly added pain.008.001.02.austria.003 file works flawlessly (at least banking software doesn't complain). Thank you!

This issue can be closed, as everything is more than solved -- there is only a b flag at fopen to be checked, if that is to be changed to wb.

I've joined the Gitter as well, and I'll post my findings as soon as I stumble upon something noteworthy.

AmdmntInd tag

Currently, as you already pointed out, no issues with the AmdmntInd tag in new version. The formentioned errors raised by the banking software can be seen on the example below, probably arising from the empty AmdmntInd tag.

Ungültiger XML Datenträger
'' is not a valid value for boolean.
<MndtRltdInf>
    <MndtId>1236548811</MndtId>
    <DtOfSgntr>2016-11-10</DtOfSgntr>
    <AmdmntInd/>
</MndtRltdInf>

src/Sephpa.php: storeSepaFile()

The XML wasn't generated by the new version and the error was raised -- so I've added w flag to fopen within storeSepaFile() function. I didn't test anywhere else, but I've noticed it was set to w in previous versions (v. 1.3.0 and previous master version).

// previous master version, and v. 1.3.0
// $file = fopen($filename, 'w');

// last master version
// $file = fopen($filename, 'b');

$file = fopen($filename, 'wb');

disappearing BtchBookg tag

The thing to check here is, why BtchBookg tag disappears by merely setting the checkAndSanitize (false), although the 'btchBookg' => false is present within addCollection array.

It disappears as well for the checkAndSanitize (true) case, but only if it's not present within the addCollection array.

Additionally, I've read (page 9) for PAIN.008.001.02 the parameter BtchBookg to be optional, as well as the payments will always be batched regardless of what creditor populates here.

AbcAeffchen commented 7 years ago

Thanks for your tests.

So thanks for your help on this. I think it improved Sephpa a lot.

Please open a new issue if you face any other bugs.

sargac commented 6 years ago

Hello, I would kindly ask you to update the src/Sephpa.php file -- the function storeSepaFile() around line 176 so the line would read $file = fopen($filename, 'wb'); as discussed. It seems the 'b' (binary) flag cannot exist alone, and it raises an file permission error when alone, causing the XML failure. Best!

ps. I see the fix is already applied to development branch, so I guess the best would be to wait for the new version.