Bukimedia / PrestaSharp

CSharp .Net client library for the PrestaShop API via web service
GNU General Public License v3.0
155 stars 151 forks source link

Fix error adding first product_feature (when no product feature exists) #381

Closed robinbittner closed 4 years ago

robinbittner commented 4 years ago

When creating the first product_feature in the database, the response from PrestaShop API is OK, but contains an empty "position" element: <position></position>

This raises "object null reference error" in PrestaSharpDeserializer, because "position" element is expected to contain any non-empty INT value. The error does not appear when creating 2nd, 3rd, etc. product_feature.

mowcixo commented 4 years ago

Hello @robinbittner, thanks for the PR. As it bug is produced by a bug in PrestaShop, I don't think we should address it, in fact I've reported this bug in the PrestaShop repository time ago as you can see in https://github.com/PrestaShop/PrestaShop/issues/14439

We are going to think about that, maybe we should address empty values in all deserialization process.

robinbittner commented 4 years ago

Hi, I know it is a bug in PrestaShop. I use latest PrestaShop 1.7.6.1 and is not corrected yet. But users of PrestaSharp do not know it and for them using PrestaSharp can be tricky. From my point of view, everthing that makes PrestaSharp more stable is valuable.

But I accept your point of view as well,

mowcixo commented 4 years ago

Totally true. Can you review then, If it could be possible to have another bugs in PrestaSharpDeserializer.cs related to parsing primitive types and if there are, use this PR to fix it?

robinbittner commented 4 years ago

sure, I will be glad to contribute. This week I was debugging PrestaSharpDeserializer.cs several times, and I did not discover any other error. Next 2 months I will be using your PrestaSharp very intensively, because I migrate winery eshop to PrestaShop. If I find an error, I will create PR. At the moment I have some small improvements (few code lines) and I will create PR for them.

mowcixo commented 4 years ago

That's great. Just try to make the PR as atomic as you can, avoiding use the same PR for multiple unrelated things.

Merging this.

robinbittner commented 4 years ago

ok, I will do small PR - they are better to review and merge.