Skip to content
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

implementation for schema namespace definition like SDD #188

Conversation

linushstge
Copy link
Contributor

@linushstge linushstge commented Jan 26, 2024

Harmonization of the Document Builder to support custom schema namespaces

Breaking change

$schema attribute added for CCT and CIP (like in CustomerDirectDebitBuilder.php)

This change harmonize all transaction builders to support the schema attribute for all order types as already implemented in SDD

Additional changes

  • Remove deprecated utf8_encode method

@linushstge
Copy link
Contributor Author

linushstge commented Jan 26, 2024

Hey @andrew-svirin, in your commit 1692d4d you implemented the $schema variable as first method parameter, which breaks the default behaviour of this library and requires changes to all implementations.

In my opinion, we should move the $schema attribute to the last parameter, with a default value urn:iso:std:iso:20022:tech:xsd:pain.001.001.03 for CCT and CIP and urn:iso:std:iso:20022:tech:xsd:pain.008.001.02 for SDD.

This wouldn't result in another breaking change, and also doesn't require setting the schema namespace by each user.

If you agree with that, I'm happy to change this behaviour as in this comment suggested instead of harmonizing the CCT and CIP builders.

Let me know your opinion.

@@ -15,7 +15,10 @@ final class DocumentFactory
public function create(string $content): Document
{
$document = new Document();
$document->loadXML(utf8_encode($content));

$content = mb_convert_encoding($content, 'UTF-8', mb_list_encodings());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mb_convert_encoding($content, 'UTF-8', mb_list_encodings()); I believe this logic can be moved outside library. So into create method should income normalized content. Some description can be added to method.
$document->loadXML($content); should be good.

Copy link
Contributor Author

@linushstge linushstge Jan 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have removed any encodings and updated the method description.

Additionally, I moved the schema attribute for all builders to the end with the current default scheme for better upwards compatibility, as suggested in my pull request description.

@andrew-svirin
Copy link
Member

@linushstge I have little bit updated pull request. Do not know how to add one more commit to current pull request, so created a new one - https://github.com/andrew-svirin/ebics-client-php/pull/189/commits

Check it if it that what you intend to add.

@andrew-svirin andrew-svirin merged commit f70c7c9 into ebics-api:2.x Jan 30, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants