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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #36 on php5.6 #37

Merged
merged 2 commits into from
Aug 17, 2017
Merged

Fixes #36 on php5.6 #37

merged 2 commits into from
Aug 17, 2017

Conversation

OGKevin
Copy link
Contributor

@OGKevin OGKevin commented Aug 16, 2017

This should fix the error shown in #36.

However the avatar fatal is still present when using /sdk_php/src/Model/Generated/MonetaryAccountBank.php 馃

Tests passes onphp7.1.

@OGKevin OGKevin added this to the v0.10.0 milestone Aug 16, 2017
@OGKevin OGKevin self-assigned this Aug 16, 2017
@@ -15,5 +15,6 @@
const DEVICE_DESCRIPTION = 'Server 1';
const PERMITTED_IPS = [];

date_default_timezone_set('UTC'); // When using php5.6
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh btw... Are server responses indeed in UTC? Which timezone do we want to stick to in there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dnl-blkv it appears that UTC is returned. But this is not documenten on the docs 馃.

Copy link
Contributor

@dnl-blkv dnl-blkv Aug 16, 2017

Choose a reason for hiding this comment

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

hmm, then we probably want to set this as a default for all times the date is used... In some entry point of the SDK perhaps, or just create a date wrapper with UTC to use there %)

Copy link
Contributor

Choose a reason for hiding this comment

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

@OGKevin let's remove this line for now and address it as a separate issue! :)

@OGKevin OGKevin requested review from dnl-blkv and removed request for qurben and andrederoos August 17, 2017 07:31
Copy link
Contributor

@dnl-blkv dnl-blkv left a comment

Choose a reason for hiding this comment

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

Looks good!

@dnl-blkv dnl-blkv merged commit c672ea6 into develop Aug 17, 2017
@dnl-blkv
Copy link
Contributor

@andrederoos

@dnl-blkv dnl-blkv deleted the hotfix/#36 branch August 17, 2017 07:34
@andrederoos
Copy link
Contributor

@OGKevin the fatal still exists in /sdk_php/src/Model/Generated/MonetaryAccountBank.php then how can this fix works? We need MonetaryAccountBank (or at least the ID) for quite a lot of other calls?

@OGKevin
Copy link
Contributor Author

OGKevin commented Aug 22, 2017

@andrederoos Wel the particular error in #36 is fixed with this pr, however the structure we've chosen for the SDK doesn't seem to like php 5.6. We were debating on dropping 5.6 or not but have't come to a decision yet. Based on this decision we can see what we can do with the fatal.

/cc @dnl-blkv @qurben

@dnl-blkv
Copy link
Contributor

@OGKevin What is the fatal happening here?

@OGKevin
Copy link
Contributor Author

OGKevin commented Aug 22, 2017

@dnl-blkv The avatar fatal

Fatal error: Cannot use bunq\Model\Generated\Object\Avatar as Avatar because the name is already in use in /sdk_php/src/Model/Generated/MonetaryAccountBank.php on line 8

Because there are 2 Avatar in the same namespace 馃

@dnl-blkv
Copy link
Contributor

@OGKevin Could you please create an issue for it? (No there are no two Avatars in the same namespace, the namespaces are distinct. The problem is that one of the avatars is available without namespace specification.)

@OGKevin
Copy link
Contributor Author

OGKevin commented Aug 22, 2017

ack, #38

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants