nemiah / phpFinTS

PHP library to communicate with FinTS/HBCI servers
MIT License
130 stars 39 forks source link

Move field types from phpdoc to native property types #404

Closed Philipp91 closed 1 year ago

Philipp91 commented 1 year ago

This is dependent on #403. It's also a potentially breaking change, so I propose we roll this into a new major together with that other PR, and test it before release (can we do a pre-release, release-candiate or so?).


This PR modernizes the code by replacing fields types in phpdoc (/** @var int */ private foo;) with native PHP properties (private int foo). This is less verbose to read, better supported by IDEs, and actually type-checked by PHP. However, this could lead to new failures in multiple ways:

  1. The runtime enforcement could lead to new failures due to type mismatches that were previously hidden/ignored, but are real issues that need fixing.
  2. The use of PHP properties means that PHP won't implicitly initialize the fields with null anymore. Instead, reading such properties before writing to them leads to an error. For nullable fields, this PR therefore adds an = null initializer, but other fields might still be affected. There's also at least one case where the FinTS specification declares a field as mandatory, but in practice many banks leave it out, so we need to make it nullable in order not to get (new) errors.

The latter point in particular requires some extensive testing in practice before releasing this. I'm happy to deploy a pre-release to my server for a few months, and perhaps a few other people could do the same.


This PR is best reviewed in pieces:

nemiah commented 1 year ago

It looks like I can do a pre-release, indicating that this release is not production-ready. I'm not sure if it is useful though, if people can't install it or don't install, they won't test it.

We can let the changes hang a while on the master branch and see what happens.

At the latest when I include the changes in my software, we will have some testers. But this is still a bit in the future as I have no control over the environments my customers install my software in. So I'll stick with PHP 7 probably until November.

Philipp91 commented 1 year ago

if people can't install it

Well, they can, right? A pre-release version can be specified like any other and then it will be installed. But it won't be auto-recommended or even auto-picked by tools like composer outdated, it won't be used when someone wants the latest version, and it will be marked explicitly as unstable.

We can let the changes hang a while on the master branch

Good idea, that might work. I don't recall how difficult or easy it is to use dev-master as the "version" with Composer. At least with NPM, the reason for a pre-release is that it's pretty much the only way for people to install it into their applications. But with Composer it might be trivial to just put dev-master, which is as good as a pre-release. I'll try after this got merged, and I'll ask for a pre-release again in case the it doesn't work.