Custom Product fields are not working after migration to TB

[moaai]

Hi,

I've been following TB right from the start, and finally decided to quit Persta this year. So far, I am very happy because most of the stuff works fine even though I have a lot changes in shop core.

Some things unfortunately are not working.

In my store, I have additional fields added to the Product class that work with Presta, but not with TB.

I am blaming that change:

https://github.com/thirtybees/thirtybees/commit/e563ae2e5aa13a15a06acbe3552621bfdfb15384

in particular removal of that line:

$this->object = new Product($this->id_object);

Since it was changed > 3 years ago, I started to think there was something wrong with my code as there are definitely other stores that work with TB and have a product class extension.

But at the moment I'm not entirely sure if there is anything wrong with my code.

Things I found while looking for a solution:

Below I am referring to the code from version 1.2.0. I disabled non-TB modules but left "overrides" set to yes.

A) AdminProductsController::initProcess

a) For 'submitAddproductAndStay' and 'submitAddproduct' Product object is not created (removed by the above mentioned commit)
b) For 'submitAddProductAndPreview' Product object is created and my extension is working

Is that intentional, some kind of optimization?

B)

I also noticed that my extension works until someone sets filters in the product list (ie SubmitFilterproduct). This is in fact also result of the changes introduced by the above mentioned commit;

How to reproduce:

1. Set some filter for the Product (for example search for name). Filter is stored in cookie.
2. Enter product and update custom fields than click "Save and Stay"

Simplified flow:

• AdminController::initProcess()[3972] filter is set to true: $this->filter = true; (since there is filter set in cookie). And that is not ok. In my opinion it should not happen in the product, filters are for the product list.
• AdminController::postProcess[589] processFilter method is executed
• ObjectModel::getDefinition($this->className)[665] - ObjectModel::getDefinition pulls 'definition' static variable from ProductCore class and ignores my modifications set in my custom child Product class constructor:

...

static::$definition['fields'] .... etc

...

• AdminController::postProcess[606] ($return = $this->{'process'.Tools::toCamelCase($this->action)}();) - The product is saved without any custom fields. In short: in the AdminController::processSave method, the loadObject method takes a cached(by processFilter) 'definition' variable without my modifications.

How to sovle:

1) Revert above mentioned commit and add "$this->object = new Product($this->id_object);" for the 'submitAddproductAndStay' and 'submitAddproduct' actions.
2) Re-Define "definition" variable in the child className.
3) Prevent setting $this->filter = true in the AdminController::initProcess for the Product view/page.
4) Change caching mechanism in the ObjectModel::getDefinition

Any other ideas? Or maybe I am missing something obvious here?

[yaniv14]

Please open an issue on github.

https://github.com/thirtybees/thirtybees/issues

[datakick]

@moaai the cause of the problem is that you choose wrong way to extend object metadata.

If I understand correctly, you modify static product metadata definition inside constructor. That's really not ok. Instantiating object should not have side effects.

If you depends on product constructor to change metadata, then we have following problem:

echo Product::productFieldExists('my_custom_field'); // prints false
$product = new Product();
echo Product::productFieldExists('my_custom_field'); // prints true

In other words, system is aware of the custom field only after constructor is called. And this is a big problem that can cause a lot of issues.

As an example, consider following two code snippets. Because of the side effect of constructor, both would behave very differently, even though they are both perfectly valid:

$fields = $this->getDefinedFields('product');
$product = new Product($id);
for ($field: $fields) {
  $output->addOutputField($field, $product);
}

$product = new Product($id);
for ($field: $this->getDefinedFields('product')) {
  $output->addOutputField($field, $product);
}

My recommendation is for you to change your code. Unfortunately there is no easy way to extend object model definition (it will be soon, it's on my todo list). Meanwhile, I recommend you use solution #2 - to redefine "definition" variable in the child class.

 

[moaai]

@datakick, I decided to write in this forum after finding your post:

https://forum.thirtybees.com/topic/4678-custom-field-checkbox-admin-product-page/?do=findComment&comment=38020

I believe my extension is compatible with the old "Presta" way and identical to the one you mentioned in the post above.

I think the only side effect is that someone decided to implement it that way and set variable "definition" to static public and use it by executing getDefinition ignoring(or preventing) completely valid parent -> child relationships.

I'm glad that it is on your to-do list because I think that the variable "definition" should not be available directly from the children's classes, or there should be some short comment that we should not extend, but only redefine.

Why for a time being we can't restore : $this->object = new Product((int)$idProduct) in AmindProductsController::initProcess()?

Edited March 7, 2021 by moaai

[datakick]

@moaai Yeah, I know that this is the 'common presta' way to implement custom fields, unfortunately. I have seen this kind of recommendation in many posts on prestashop (and even on thirtybees) forum. But that doesn't make it right. Object model definition is a shared class property, not instance property. Constructor (or any other instance method) should not modify it.

Redefining the whole property in product override is not an ideal. It can cause a lot of problems in case we change the model definition in core. But I believe it's a lesser of two evils. 

Regarding restoring the original code in initProcess method -- I (reluctantly) agree to do that. This code was removed probably because the object model is instantiated later (int processSave or processAdd method) anyway. But the instantiation is conditional -- i the object already exists, it will not be loaded again -- so it should not pose any performance issue. And it will make thirtybees codebase a little bit more compatible. I still don't like it because it feels like rewarding bad behavior 😉 The code will be in bleeding edge soon. 

[moaai]

@datakick thank you for considering my request, and applying it into code. Whenever you find time and start working on a extension process to simplify it, I'll be happy to help.