search

aerospike-community / aerospike-client-php

Aerospike client for PHP7
Apache License 2.0
29 stars 28 forks source link

Add missing ZEND_ARG_... definitions to Aerospike methods #37

Closed sustmi closed 5 years ago

sustmi commented 5 years ago

Fixes: https://github.com/aerospike/aerospike-client-php/issues/36

sustmi commented 5 years ago

I fixed all the failing tests but one by removing the type hints because even though the Aerospike class contains methods with type hints, the tests clearly disobey the argument types. The last failing test is \Udf::testUdfNegativeDeregisterUnknownModule but this test fails even without changes introduced in this PR. I can see that you fixed this in: https://github.com/aerospike/aerospike-client-php/commit/05cbf2d50ae23935e9fea18e3445f7e8bcb8eea4 . Should I create a PR to a different branch?

lmathukumalli commented 5 years ago

Any updates on when this fix will be merged with master?

I am running into the following PHPStan error... 'Class Aerospike constructor invoked with 3 parameters, 0 required.'

arrowplum commented 5 years ago

Hi sorry for the delay looking into this now.

arrowplum commented 5 years ago

Could you please rebase from master? I have merged in the travis fix from the branch. Hopefully that will let this build.

lmathukumalli commented 5 years ago

Thanks for the help!

sustmi commented 5 years ago

Sorry for the delay. I did not notice that it was waiting for my rebase. I rebased the code but now the tests fail on errors that seems not relevant to the changes I made (eg. https://travis-ci.org/aerospike/aerospike-client-php/jobs/576713694#L722 )

sustmi commented 5 years ago

Confirmed, the tests fail the same way on the current upstream master (without my changes): https://travis-ci.org/sustmi/aerospike-client-php/jobs/576721335#L850

sustmi commented 5 years ago

It looks like the CI is alright again. Also, I rebased the PR on the current master. Can we merge it now? :innocent:

dwelch-spike commented 5 years ago

It's merged.

Thank you for the contribution. :)

lmathukumalli commented 5 years ago

Sorry for my late reply.

Thank you for working on this request. I checked it and the change looks good.

Appreciate it!

On Fri, Sep 13, 2019 at 7:13 PM dwelch-spike notifications@github.com wrote:

It's merged.

Thank you for the contribution. :)

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/aerospike/aerospike-client-php/pull/37?email_source=notifications&email_token=ALSYK3VDI7RHI4OVQBI47HTQJQNCZA5CNFSM4IDCBWZ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD6WM2GI#issuecomment-531418393, or mute the thread https://github.com/notifications/unsubscribe-auth/ALSYK3TBG7L6BEXIEWTKHKLQJQNCZANCNFSM4IDCBWZQ .