Undefined property: Proxy::getMethods- payment checkout
condor2 opened this issue · 27 comments
OC 4 also has it. May be each methods must be created accordingly even though it must be known if the method is callable or not.
Possible is another issue because shipping method use "same" code
if (is_callable([$this->{'model_extension_shipping_' . $result['code']}, 'getQuote'])) {
LE. I think is related with getMethods from checkout, and getMethod from payment extensions models
I did wondered when adding the lookup. Possible conflicts of instantiation.
Dumped each callable magic methods into variables: f6535fe .
Tested with: 6f92c76 , it looks like even though the is_callable would look for a variable instead of using a direct condition, it will still fail to find the targeted method name if it doesn't exist from its condition.
@AJenbo: The solution provided here: https://github.com/opencart/opencart/blob/master/upload/catalog/controller/cron/subscription.php#L361 would, therefore, not provide any difference in regard to a variable vs. direct condition with a is_callable function.
I didn't write that code:
Probably the reason to put it in a variable would be to have shorter lines but idk.
is_callable() is needed when the object isn't well defined and the existence of the method can only be determined at runtime. Since OC refrains from using interfaces a lot of it's code is not well defined.
The issue reported by condor2 appears to be there because the code isn't consistent in what it tests and what it calls. It should either do both directly or both on the variable.
Theoretical
$callable = [$this->{'model_extension_payment_' . $result['code']}, 'getMethods'];
is not like ?
$callable = [$this->{'model_extension_payment_cod}, 'getMethods'];
and getMethods is not read from payment model?
But in model I don't see where is write getMethods
https://github.com/opencart/opencart-3/blob/main/upload/catalog/model/extension/payment/cod.php#L15
First problem, line 361 uses this:
{'model_extension_' . $extension_info['extension'] . '_payment_' . $extension_info['code']}
But line 365 calls this:
{'model_extension_' . $order_data['payment_method']['extension'] . '_payment_' . $order_data['payment_method']['code']}
The first one is done to check that the second won't fail so they need to be identical.
Next issue is that the analyzer can't know that building the name twice will give the same result, so use $callable twice instead of building the name twice.
if (is_callable($callable)) {
// Process payment
$response_info = $callable($this->customer->getId(), $this->session->data['order_id'], $order_info['total'], $order_data['payment_method']['code']);First problem, line 361 uses this:
{'model_extension_' . $extension_info['extension'] . '_payment_' . $extension_info['code']}But line 365 calls this:{'model_extension_' . $order_data['payment_method']['extension'] . '_payment_' . $order_data['payment_method']['code']}The first one is done to check that the second won't fail so they need to be identical.
Next issue is that the analyzer can't know that building the name twice will give the same result, so use
$callabletwice instead of building the name twice.if (is_callable($callable)) { // Process payment $response_info = $callable($this->customer->getId(), $this->session->data['order_id'], $order_info['total'], $order_data['payment_method']['code']);
Ok, will take care of that. As for OC 4, however, the same way should be used with the $callable since an IF statement is used but then recalls the models dynamically which means the analyzer may not like it any better on a higher diagnostic level than it may not already like it on a lower level from this repository. If one is concerned about using a variable while OCMod has been re-integrated in OC 4 as opposed to constantly throwing errors in PHPStan while using a variable may both solve these errors, the probability that users will be able to make those changes with XML in the future are probably going to be higher on anyhow, therefore, $callable should also be considered in OC 4.
OC 3 has class name collisions because it does not use namespaces which makes it unable to pass level 2.
OC 4 has namespaces. But is unable to pass level 7 because it isn't clear if it's loading a Model from the Admin or Catalog namespace when you access something like $this->model_sale_order->getOrder();.
Should be fixed, now: c03237f
@TheCartpenter from where getMethods is taken in payment_method?
https://github.com/opencart/opencart-3/blob/main/upload/catalog/model/checkout/payment_method.php
$callable = [$this->{'model_extension_payment_' . $result['code']}, 'getMethods'];
Is not like this?
Example: (cod payment)
Load the file.... model_extension_payment_cod and read getMethods function? But in all payments models there is no getMethods fucntion
https://github.com/opencart/opencart-3/blob/main/upload/catalog/model/extension/payment/cod.php#L15
On the shipping_method you have used 'getQuote' but funtion exist in all shipping model.
And yes, in OC 4 is used getMethods
https://github.com/opencart/opencart/blob/master/upload/extension/opencart/catalog/model/payment/cod.php#L14
But 3.2 still has getMethod, without S
That is why I am using is_callable so to know if the getMethods() does exist prior to use it. As for the singular vs. the plural method conventional names, I am not done, yet. Work still needs to be done on that side.
Should PRs be addressed about the getMethods() in the meantime, by all means.
OC 3 has class name collisions because it does not use namespaces which makes it unable to pass level 2.
OC 4 has namespaces. But is unable to pass level 7 because it isn't clear if it's loading a
Modelfrom theAdminorCatalognamespace when you access something like$this->model_sale_order->getOrder();.
Will the $callable also respond to the total quotes ($quote) by using referenced variables like $total_data and $total also when putting the getTotal() ?
I made a PR https://github.com/opencart/opencart-3/pull/283/files , to replace getMethod with getMethods , but on checkout doesn't show payment method name :)
That's because the payment_method and shipping_method model from checkout/payment_method and shipping_method now takes the lead for the $callable. Therefore, each $callable need to be replaced to the same calls as OC 4 from controller/checkout/payment_method and controller/checkout/shipping_method from now on.
Take note that the extension/extension folder using $callable is not reflected with the suggested change above nor should it be with the totals.
From what I see you created the callable using the array syntax so it will go through the Proxy::__call() method which is not made to handle variables by reference. But if you want to avoid that you could simply create the variable in another way, like:
if (isset($this->{'model_' . $blabla}->getMethods)) {
$callable = $this->{'model_' . $blabla}->getMethods;From what I see you created the callable using the array syntax so it will go through the
Proxy::__call()method which is not made to handle variables by reference. But if you want to avoid that you could simply create the variable in another way, like:if (isset($this->{'model_' . $blabla}->getMethods)) { $callable = $this->{'model_' . $blabla}->getMethods;
I was hoping to avoid the use of isset and property_exists for those calls. Would bringing those specific calls to their previous state still create a conflict with the totals whenever there are referenced variables in-place?
What previous state?
What previous state?
Reverted totals with original calls: 656aba7
Yes, you need to do the following if you want to avoid the magic __call():
$callable = $this->{'model_extension_total_' . $result['code']}->getTotal;
$callable($total_data);Yes, you need to do the following if you want to avoid the magic __call():
$callable = $this->{'model_extension_total_' . $result['code']}->getTotal; $callable($total_data);
Would that also prevent conflicts between the getTotal methods during the same instantiation?
What do you mean by conflict?
What do you mean by conflict?
By following this, using the same scenarios from your above solution, is there a possible scenario where the comment from @condor2 may be reproduced with $callable($total_data) ?
I'll leave it to the original calls, for now. Order totals seem to work with the referenced variables.