Add strictly typed responses; fix GET in extended models [#52] #53

dnl-blkv · 2017-09-25T09:46:26Z

Fixes #51
Fixes #52

DennisSnijder

Impressive PR!

Added some minor comments / Improvements.

DennisSnijder · 2017-09-25T12:28:51Z

src/Http/BunqResponse.php

+     */
+    public static function castFromBunqResponse(BunqResponse $bunqResponse): BunqResponse
+    {
+        return new static($bunqResponse->getValue(), $bunqResponse->getHeaders(), $bunqResponse->getPagination());


According to PSR-2 standard you should make this multiline.
Also makes it a bit more readable in my opinion

return new static( $bunqResponse->getValue(), $bunqResponse->getHeaders(), $bunqResponse->getPagination() );

@DennisSnijder Thanks, fixed!

@dnl-blkv Awesome! 😄

DennisSnijder · 2017-09-25T12:31:08Z

src/Model/BunqModel.php

     *
     * @return BunqResponse
     * @throws BunqException   When the result is not expected.
     */
-    protected static function fromJson(BunqResponseRaw $responseRaw)
+    protected static function fromJson(BunqResponseRaw $responseRaw, string $wrapper = null)


Strictly typing the return value will make the PHPDoc unnecessary.

@DennisSnijder there are some things which, as of PHP 7.0 (and partially 7.1), can be expressed with PHPDoc, but not with the strict typing of PHP. Examples are nullable return statements (PHP 7.1+),throws (not present even in PHP 7.1) or late static binding.

Therefore, for now we would like to keep it to PHPDoc for now, until PHP has full and proper support of the strict typing :).

DennisSnijder · 2017-09-25T12:31:37Z

src/Model/BunqModel.php

@@ -270,7 +251,7 @@ private static function determineJsonResponseValue(array $responseArray)
    protected static function processForUuid(BunqResponseRaw $responseRaw)


Return type? 😄

This is [WIP]; return types are coming :P

DennisSnijder · 2017-09-25T12:32:00Z

src/Model/BunqResponseInstallation.php

+use bunq\Http\BunqResponse;
+
+/**
+ */


Unnecessary PHPDoc ?

Also occurs in a lot of other models within this PR

…d missing strict type for listings [#52]

andrederoos · 2017-10-05T12:06:40Z

example/customer_statement_export_example.php

@@ -32,12 +32,10 @@
 $apiContext = ApiContext::restore();

 // Retrieve the active user.


extend the comment that if you are not a bunq business user, change ...->getUserCompany() to ...->getUserPerson()?

andrederoos · 2017-10-05T12:06:59Z

example/monetary_account_example.php

@@ -16,12 +16,10 @@
 $apiContext = ApiContext::restore(ApiContext::FILENAME_CONFIG_DEFAULT);

 // Retrieve the active user.


same as previous comment

wel, basically for every example.

andrederoos · 2017-10-05T12:15:25Z

src/Http/Pagination.php

@@ -114,10 +114,10 @@ private static function parsePaginationBody(array $paginationResponse)
     */
    private static function updatePaginationBodyIdFieldFromResponseField(
        array &$paginationBody,


why? now the static function, looks like you forgot to return the updated paginationBody

This piece of logic did not change with this PR, and it is done exactly consistent with our SDKs in all the other languages. This probably should be moved to a class later, but let us keep it for a separate issue and PR! :)

andrederoos · 2017-10-05T12:44:06Z

src/Context/ApiContext.php

+    {
+        $jsonString = FileUtil::getFileContents($fileName);
+
+        if ($jsonString === false) {


there is already an exception thrown in FileUtil

Thanks for pointing out! I added try/catch around the call to getFileContents, so that we can still throw a context-specific exception :).

dnl-blkv · 2017-10-05T21:03:11Z

@andrederoos all answered/addressed

DennisSnijder · 2017-10-06T08:14:37Z

Thanks! @dnl-blkv 😄

Add strictly typed responses; fix GET in extended models [#52]

00e223b

dnl-blkv self-assigned this Sep 25, 2017

dnl-blkv added 3 commits September 25, 2017 13:36

Add strict BunqResponse typecasts [#52]

b48fd00

remove extra constructs [#52]

5d3b6ba

cleanup JsonParserTest; fix doc return types for mixed arrays [#52]

76d4949

DennisSnijder reviewed Sep 25, 2017

View reviewed changes

dnl-blkv added 4 commits September 25, 2017 15:20

fix namespaces [#52]

f547178

fix docs for generated objects [#52]

5410548

fix a few phpstan errors [#52]

3ad99fd

fix minor formatting issues [#52]

706b0f8

OGKevin added the enhancement label Sep 25, 2017

dnl-blkv added 14 commits September 26, 2017 10:38

fix location of tests [#52]

716eb9a

fix reference to attachment [#52]

33d529a

Fix types of Context [#52]

fdb382b

update types in Http [#52]

fc0e1cd

rearrange code [#52]

b123489

fix types for Model [#52]

51b1900

Add missing phpcd; fix types of Security [#52]

60dc6b1

fix typing in the rest of src/ and example/ [#52]

c7d31a3

Use strict typing for all the non-generated code [#52]

fe6a1c1

exclude Generated from phpcpd [#52]

b2634e1

Add strict types for BunqResponse's and endpoint methods [#52]

b918975

fix gitignore [#52]

4b4277c

adjust grumphp timeout [#52]

f879802

Temporarily remove strict types from getters/setters/constructors; ad…

4ea10a0

…d missing strict type for listings [#52]

dnl-blkv changed the title ~~[WIP] Add strictly typed responses; fix GET in extended models [#52]~~ Add strictly typed responses; fix GET in extended models [#52] Sep 26, 2017

dnl-blkv assigned andrederoos Sep 26, 2017

dnl-blkv requested a review from andrederoos September 26, 2017 16:04

andrederoos requested changes Oct 5, 2017

View reviewed changes

fix attachments of ScheduleRequestInquiryEntry [#52]

76f8273

improve import ordering; utilize try/ctach [#52]

53870a9

organize imports [#52]

2aceb13

andrederoos approved these changes Oct 6, 2017

View reviewed changes

andrederoos merged commit 6c670c2 into develop Oct 6, 2017

andrederoos deleted the 52-strict-response branch October 6, 2017 07:33

OGKevin added a commit to bunq/sdk_python that referenced this pull request Feb 1, 2018

Removed unneeded doc block. (bunq/sdk_php#53)

440bd22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add strictly typed responses; fix GET in extended models [#52] #53

Add strictly typed responses; fix GET in extended models [#52] #53

dnl-blkv commented Sep 25, 2017 •

edited

DennisSnijder left a comment •

edited

DennisSnijder Sep 25, 2017

dnl-blkv Sep 25, 2017

DennisSnijder Sep 25, 2017

DennisSnijder Sep 25, 2017

dnl-blkv Sep 25, 2017

DennisSnijder Sep 25, 2017

dnl-blkv Sep 25, 2017

DennisSnijder Sep 25, 2017

DennisSnijder Sep 25, 2017

andrederoos Oct 5, 2017

andrederoos Oct 5, 2017

andrederoos Oct 5, 2017

andrederoos Oct 5, 2017

dnl-blkv Oct 5, 2017

andrederoos Oct 5, 2017

dnl-blkv Oct 5, 2017

dnl-blkv commented Oct 5, 2017

DennisSnijder commented Oct 6, 2017

		@@ -270,7 +251,7 @@ private static function determineJsonResponseValue(array $responseArray)
		protected static function processForUuid(BunqResponseRaw $responseRaw)

		@@ -32,12 +32,10 @@
		$apiContext = ApiContext::restore();

		// Retrieve the active user.

		@@ -16,12 +16,10 @@
		$apiContext = ApiContext::restore(ApiContext::FILENAME_CONFIG_DEFAULT);

		// Retrieve the active user.

Add strictly typed responses; fix GET in extended models [#52] #53

Add strictly typed responses; fix GET in extended models [#52] #53

Conversation

dnl-blkv commented Sep 25, 2017 • edited

DennisSnijder left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnl-blkv commented Oct 5, 2017

DennisSnijder commented Oct 6, 2017

dnl-blkv commented Sep 25, 2017 •

edited

DennisSnijder left a comment •

edited