bpostshm.php enhancements

[tchauviere]

Here are some few enhancements that can be done to improve module stability:

• https://github.com/PrestaShop/bpostshm/blob/master/bpostshm.php#L223: Please use Tools::file_exists_cache() instead of file_exists
• https://github.com/PrestaShop/bpostshm/blob/master/bpostshm.php#L257: Cast $group['id_group']; into (int)
• https://github.com/PrestaShop/bpostshm/blob/master/bpostshm.php#L263-264 https://github.com/PrestaShop/bpostshm/blob/master/bpostshm.php#L452 $id_carrier is not cast to (int) Once you have done “new Carrier()”, please check if the object is well loaded with “Validate::isLoadedObject($carrier)” and add error message if needed
• https://github.com/PrestaShop/bpostshm/blob/master/bpostshm.php#L271-277: Cast “$lang_fields['name'];” to (string) Cast “$carrier->save()” to (int)
• https://github.com/PrestaShop/bpostshm/blob/master/bpostshm.php#L302 Missing double (int) cast “if ($country->id_zone != $id_zone_be)”
• https://github.com/PrestaShop/bpostshm/blob/master/bpostshm.php#L478: OrderState::getOrderStates($this->context->language->id); Missing (int) cast + Method can return false since it depends of the result of a Db::getInstance()->executeS(); you should verify that $order_states is the array you expect and add error message if needed
• https://github.com/PrestaShop/bpostshm/blob/master/bpostshm.php#L487: $id_order_state is not cast to (int) Once you have done “new OrderState()”, please check if the object is well loaded with “Validate::isLoadedObject($order_state)” and add error message if needed
• https://github.com/PrestaShop/bpostshm/blob/master/bpostshm.php#L645: $id_tab is not cast to (int) Once you have done “new Tab()”, please check if the object is well loaded with “Validate::isLoadedObject($id_tab)” and add error message if needed
• https://github.com/PrestaShop/bpostshm/blob/master/bpostshm.php#L662: Consider using Tools::copy instead of copy()
• https://github.com/PrestaShop/bpostshm/blob/master/bpostshm.php#L824-828: Maybe here you could use constants to make the code more readable.
• https://github.com/PrestaShop/bpostshm/blob/master/bpostshm.php#L885-889: You should check the return value of object->save() method and display errors if needed
• https://github.com/PrestaShop/bpostshm/blob/master/bpostshm.php#L1098: Once you have done “new Address()”, please check if the object is well loaded with “Validate::isLoadedObject($delivery_address)” and skip to the next iteration if required
• https://github.com/PrestaShop/bpostshm/blob/master/bpostshm.php#L1275/1277: (int) cast missing
• https://github.com/PrestaShop/bpostshm/blob/master/bpostshm.php#L1405-1411: getOrderShippingCostExternal() method empty and unsued, remove it if unecessary

[Stigmi]

• Please use Tools::file_exists_cache() instead of file_exists >> OK https://github.com/PrestaShop/bpostshm/blob/master/bpostshm.php#L218
• Cast $group['id_group']; into (int) >> OK https://github.com/PrestaShop/bpostshm/blob/master/bpostshm.php#L252
• $id_carrier is not cast to (int) >>NOK expected to be int or null to create new carriers initially. https://github.com/PrestaShop/bpostshm/blob/master/bpostshm.php#L259 https://github.com/PrestaShop/bpostshm/blob/master/bpostshm.php#L347
• Once you have done “new Carrier()”, please check if the object is well loaded with “Validate::isLoadedObject($carrier)” and add error message if needed >> NOK (see above) new objectModel has no id before save, the check is misplace at this time and will fail to install. https://github.com/PrestaShop/bpostshm/blob/master/bpostshm.php#L260
• Cast “$lang_fields['name'];” to (string) >> OK https://github.com/PrestaShop/bpostshm/blob/master/bpostshm.php#L270
• Cast “$carrier->save()” to (int) >> NOK bool or int expected here, but anything that's not false is fine. https://github.com/PrestaShop/bpostshm/blob/master/bpostshm.php#L276
• Missing double (int) cast “if ($country->id_zone != $id_zone_be)” >> NOK we don't understand https://github.com/PrestaShop/bpostshm/blob/master/bpostshm.php#L301
• Missing (int) cast OrderState::getOrderStates($this->context->language->id); >> NOK (check placed for the returned Array) https://github.com/PrestaShop/bpostshm/blob/master/bpostshm.php#L375
• Method can return false since it depends of the result of a Db::getInstance()->executeS(); you should verify that $order_states is the array you expect and add error message if needed >> OK https://github.com/PrestaShop/bpostshm/blob/master/bpostshm.php#L376-377
• $id_order_state is not cast to (int) >> NOK int or NULL expected here (same answer as id_carrier above). https://github.com/PrestaShop/bpostshm/blob/master/bpostshm.php#L387
• Once you have done “new OrderState()”, please check if the object is well loaded with “Validate::isLoadedObject($order_state)” and add error message if needed >> NOK (same answer as id_carrier above) https://github.com/PrestaShop/bpostshm/blob/master/bpostshm.php#L387
• $id_tab is not cast to (int) Once you have done “new Tab()”, please check if the object is well loaded with “Validate::isLoadedObject($id_tab)” and add error message if needed >> OK https://github.com/PrestaShop/bpostshm/blob/master/bpostshm.php#L543-547
• Consider using Tools::copy instead of copy() >> NOK Tools::copy() doesn't exist in 1.4 https://github.com/PrestaShop/bpostshm/blob/master/bpostshm.php#L563
• Maybe here you could use constants to make the code more readable. >> Code removed with "zone creation" functionality
• You should check the return value of object->save() method and display errors if needed >> Code removed with "zone creation" functionality
• Once you have done “new Address()”, please check if the object is well loaded with “Validate::isLoadedObject($delivery_address)” and skip to the next iteration if required >> OK with behavior modified since last time https://github.com/PrestaShop/bpostshm/blob/master/bpostshm.php#L908-912
• (int) cast missing >> NOK where? https://github.com/PrestaShop/bpostshm/blob/master/bpostshm.php#L1080/L1082
• getOrderShippingCostExternal() method empty and unsued, remove it if unecessary >> it is used https://github.com/PrestaShop/bpostshm/blob/master/bpostshm.php#L1205/L1220

[tchauviere]

Update with latest commits

• $id_carrier is not cast to (int)

  NOK expected to be int or null to create new carriers initially. https://github.com/PrestaShop/bpostshm/blob/master/bpostshm.php#L259 https://github.com/PrestaShop/bpostshm/blob/master/bpostshm.php#L347

  Casting null value to int will make the Object to return a new instance, just like null value itself does. So you can cast it to int

• Once you have done “new Carrier()”, please check if the object is well loaded with “Validate::isLoadedObject($carrier)” and add error message if needed

  NOK (see above) new objectModel has no id before save, the check is misplace at this time and will fail to install. https://github.com/PrestaShop/bpostshm/blob/master/bpostshm.php#L260

  Casting null value to int will make the Object to return a new instance, just like null value itself does. So you can cast it to int

• Missing (int) cast “if ($country->id_zone != $id_zone_be)”

  NOK we don't understand https://github.com/PrestaShop/bpostshm/blob/master/bpostshm.php#L301

  I meant the following `if ((int)$country->id_zone != (int)$id_zone_be)``

• $id_order_state is not cast to (int)

  NOK int or NULL expected here (same answer as id_carrier above). https://github.com/PrestaShop/bpostshm/blob/master/bpostshm.php#L387

  Casting null value to int will make the Object to return a new instance, just like null value itself does. So you can cast it to int

• Once you have done “new OrderState()”, please check if the object is well loaded with “Validate::isLoadedObject($order_state)” and add error message if needed

  NOK (same answer as id_carrier above) https://github.com/PrestaShop/bpostshm/blob/master/bpostshm.php#L387

  Casting null value to int will cause the Object to return a new instance, just like null value itself does. So you can cast it to int

• Consider using Tools::copy instead of copy()

  NOK Tools::copy() doesn't exist in 1.4 https://github.com/PrestaShop/bpostshm/blob/master/bpostshm.php#L563

  Please do the following then: (FYI 1.5.5.0 is the version where appeared Tools::copy() according to Validator PS Guide)

  if (version_compare(PS_VERSION, '<', '1.5.5.0')
   copy()
  else
  Tools:copy()

[tchauviere]

Update with latest commits

• $id_carrier is not cast to (int)

  NOK expected to be int or null to create new carriers initially. https://github.com/PrestaShop/bpostshm/blob/master/bpostshm.php#L259 https://github.com/PrestaShop/bpostshm/blob/master/bpostshm.php#L348

  Casting null value to int will make the Object to return a new instance, just like null value itself does. So you can cast it to int

=> Tested on 1.5 / 1.6 no problem doing so, it works like I said (null or 0 return a new empty object)

• Missing (int) cast “if ($country->id_zone != $id_zone_be)”

  NOK we don't understand https://github.com/PrestaShop/bpostshm/blob/master/bpostshm.php#L301

  I meant the following `if ((int)$country->id_zone != (int)$id_zone_be)``

• $id_order_state is not cast to (int)

  NOK int or NULL expected here (same answer as id_carrier above). https://github.com/PrestaShop/bpostshm/blob/master/bpostshm.php#L388

  Casting null value to int will make the Object to return a new instance, just like null value itself does. So you can cast it to int

=> Tested on 1.5 / 1.6 no problem doing so, it works like I said (null or 0 return a new empty object)

• Consider using Tools::copy instead of copy()

  NOK Tools::copy() doesn't exist in 1.4 https://github.com/PrestaShop/bpostshm/blob/master/bpostshm.php#L563

  Please do the following then: (FYI 1.5.5.0 is the version where appeared Tools::copy() according to Validator PS Guide)

  if (version_compare(PS_VERSION, '<', '1.5.5.0')
   copy()
  else
  Tools:copy()

[Stigmi]

Thibaud, I realize that there was a misunderstanding here, the part that was NOK for us was that:

https://github.com/PrestaShop/bpostshm/blob/master/bpostshm.php#L259 https://github.com/PrestaShop/bpostshm/blob/master/bpostshm.php#L348 Once you have done “new Carrier()”, please check if the object is well loaded with “Validate::isLoadedObject($carrier)” and add error message if needed

https://github.com/PrestaShop/bpostshm/blob/master/bpostshm.php#L387 Once you have done “new OrderState()”, please check if the object is well loaded with “Validate::isLoadedObject($order_state)” and add error message if needed

We'll add (int) and that Tools:copy()

[tchauviere]

Hi @Stigmi,

Yes I realized the misunderstanding this morning too. That's why I removed these lines in the update ;)

[Stigmi]

OK last commit have taken all your comments.

[tchauviere]

Fixed!