New issue

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

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

Already on GitHub? Sign in to your account

Make override of object models easy #8853

Merged
merged 1 commit into from Mar 29, 2018

Conversation

Projects
None yet
5 participants
@mickaelandrieu
Contributor

mickaelandrieu commented Mar 14, 2018

Questions Answers
Branch? develop
Description? Adding new fields to Objects models (Product mainly) is really painful and ass we need to override Core classes every time, this can't be re-usable modules. More 2 modules can't override the same object model. A small update enables the feature.
Type? improvement
Category? CO
BC breaks? no
Deprecations? no
Fixed ticket? (optional) If this PR fixes a Forge ticket, please add its complete Forge URL.
How to test? Use the module provided in develop branch and in this branch. The main difference with current ways to do it: no more Product class override, no more AdminProductAdapter override class, we can re-use Symfony forms instead of writing our own templates. this can enable new modules on Addons, as for now modules with overrides of Core classes are forbidden. Also, 2 modules can add definitions to the same Object Model instance without conflicts :)

ps_test.zip

Important guidelines

    // map a new field is now really easy => 0 overrides
    /**
     * Update Product class definition
     */
    public function hookActionDispatcherBefore()
    {
        Product::$definition['fields']['my_field'] = ['type' => Product::TYPE_STRING, 'required' => false, 'size' => 255];
    }

    /**
     * Display "my_field" in Product page.
     * @param type $hookParams
     * @return string
     */
    public function hookDisplayAdminProductsMainStepLeftColumnMiddle($hookParams) {
        $product = new Product($hookParams['id_product']);

        $form = $this->get('form.factory')
            ->createNamedBuilder('my_field', TextType::class, $product->my_field)
            ->getForm()
        ;

        // you don't need to design your form, call only form_row(my_field) in
        // your template.
        return $this->get('twig')->render('@PrestaShop/Foo/form.html.twig', [
            'my_field' => $form->createView()
        ]);
    }

    /**
     * Add a field to product table
     *
     * @return bool success of the update
     */
    public function addAFieldToProduct()
    {
        return Db::getInstance()->execute("ALTER TABLE " . _DB_PREFIX_ . "product " . "ADD my_field VARCHAR(255) NULL");
    }

    /**
     * Remove a field to product table
     *
     * @return bool success of the update
     */
    public function removeFieldToProduct()
    {
        return Db::getInstance()->execute("ALTER TABLE " . _DB_PREFIX_ . "product " . "DROP my_field");
    }

This change is Reviewable

@mickaelandrieu mickaelandrieu requested a review from Quetzacoalt91 Mar 14, 2018

@@ -87,7 +87,7 @@ public function load($id, $id_lang, $entity, $entity_defs, $id_shop, $should_cac
}
$entity->id = (int)$id;
foreach ($object_datas as $key => $value) {
if (array_key_exists($key, $entity)) {
if (array_key_exists($key, $entity_defs['fields'])) {

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Mar 14, 2018

Contributor

this is the most painful part of adding a field to Product|Customer|... entity: we have to define public property in the class when in PHP it's totally valid (and fast) to define public properties dynamically.

So, as we already have to describe a new Object Model definition (the mapping to DB), why not re-use the information?

@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented Mar 14, 2018

ping @kpodemski wdyt? ping @PrestaEdit @prestamodule can this help in your day 2 day work?

@kpodemski

This comment has been minimized.

Contributor

kpodemski commented Mar 16, 2018

what am i thinking? YEAH, finally :D

well done @mickaelandrieu

@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented Mar 16, 2018

@kpodemski of course, as the hook ActionDispatcherBefore, you can display the new field in Front office easily (you may need to "whitelist" the new field ;) ). I have the feeling you'll love 1.7.4 even more than 1.7.3 👼

@kpodemski

This comment has been minimized.

Contributor

kpodemski commented Mar 16, 2018

@mickaelandrieu

yeah, it is something to remember of course, i think that it should be noted in docs

so, overall we have ability to override object easily, back-office forms, services... this start to look really great

@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented Mar 16, 2018

@kpodemski I'm also workin' on docs => PrestaShop/docs#64

Help is very welcome as I'm human and can't be cloned easily according to my mum xD

Edit: you forgot templates, by functional blocks ;)

@jf-viguier

This comment has been minimized.

Contributor

jf-viguier commented Mar 16, 2018

I'm waiting for this improvement for long time ago.
It will be VERY usefull, please add it to the next realease.

@mickaelandrieu mickaelandrieu added this to the 1.7.4.0 milestone Mar 16, 2018

@Quetzacoalt91

I'd like you to take care about what you write in the documentation, although I approve that PR.

This simple case, where you add columns in the product table, should not be recommended to everybody for the same reason we don't recommend to modify the core files directly.
Modifying them increase the risks of failure during an upgrade, which is already complicated enough today. :)

@marionf marionf added QA ✔️ and removed waiting for QA labels Mar 29, 2018

@Quetzacoalt91 Quetzacoalt91 merged commit 38e4877 into PrestaShop:develop Mar 29, 2018

2 checks passed

Codacy/PR Quality Review Good work! A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@mickaelandrieu mickaelandrieu deleted the mickaelandrieu:ease-object-model-override branch Mar 29, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment