Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Is it possible to have my PR merged? #14

Open
skurvish opened this issue Sep 21, 2020 · 10 comments
Open

Is it possible to have my PR merged? #14

skurvish opened this issue Sep 21, 2020 · 10 comments

Comments

@skurvish
Copy link
Contributor

We update via composer so any composer update on our system will wipe these changes until the PR is merged and version released.

@thomasbnielsen
Copy link
Owner

Yeah we are looking at it now, hopefully we can merge soon

@rasmniel
Copy link
Collaborator

rasmniel commented Sep 22, 2020

Hey there, skurvish
I've looked at the PR and tested it and we are receiving warnings and notices on our side.
The offending code is this function from Response.php

	public function isSuccessful()
	{
		if ($this->getResponseBody()){
			$response_body = json_decode($this->getResponseBody());
			$data = end($response_body->operations);
			if ($response_body->accepted && $data->qp_status_code=="20000") {
				return true;
			}
		}

		return false;
	}

Line 16 have the following issues:

  • [Warning] Attempt to modify property 'operations' of non-object
  • [Warning] end() expects parameter 1 to be array, null given

Line 17 have this issue:

  • [Notice] Trying to get property 'accepted' of non-object

I assume the JSON inference in getResponseBody() causes a false return, but I can't be sure.
Let me know when you have fixed this.

@skurvish
Copy link
Contributor Author

skurvish commented Sep 22, 2020

That isn't code I added, it was there before I started any work. If the function getResponseBody() is returning false then those lines of code would not run. Quickpay must be returning a string which is not valid json data. I am not getting these warnings on my system.

@skurvish
Copy link
Contributor Author

Will you be able to make a release that is available via composer?

@rasmniel
Copy link
Collaborator

I think I may have referenced an incorrect file, but as far as I could tell from the commit history, you wrote code like the snippet I linked above.

I am only getting the errors after your changes are installed, so something must have changed...
I am sorry, that I don't have more time to help you with your update, but I will have to ask you to test the implementation as it is, preferably in a project that already successfully runs the previous version.

I sincerely doubt that Quickpay returns an incorrect response, but that remains to be determined 😉

Once again, apologies for being a bit vague. I have very little experience with the actual codebase within this repo .

We will make the release available for composer when completed 👍

@rasmniel
Copy link
Collaborator

I have merged #13 and #15 into master, so you can check it there.

@skurvish
Copy link
Contributor Author

Do you have a sample codebase that calls this and shows the error that I can get so I can try and figure out what is happening? The codebase I am using has not run on anything prior to my changes and I am not seeing the error. I suspect you are calling different methods.

@skurvish
Copy link
Contributor Author

Or is it one of the samples in the test section?

@rasmniel
Copy link
Collaborator

We do not have any sample codebase we are able to disclose. I am sorry that I do not currently have enough time to test and debug this, so you are on your own I am afraid.

My initial comment above details exactly the errors raised upon payment. As mentioned, I am unsure which parts of the code we actually execute, but I am only experiencing the errors after your update. If you have not tried calling every method, you should probably start with that.

It does not seem like the error causes an actual problem in the resulting payment, however we have too many sites running this library to risk releasing an erroneous version.

Please have another look at the code, and see if you can't find the cause. Let me know your findings and we can figure out how to move forward based on that.

Good luck 🍀

@skurvish
Copy link
Contributor Author

Is it possible to know which Omnipay/Quickpay function classes you are calling in your code? I can see if I can figure it out from there.

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

No branches or pull requests

3 participants