Skip to content
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

Setting attribute_data has inconsistent behavior based on model persistance #333

Open
Repox opened this issue Dec 11, 2020 · 1 comment
Open

Comments

@Repox
Copy link
Contributor

Repox commented Dec 11, 2020

The HasAttributes trait allows for setting attributes on models. But the attributes are being managed differently via a mutator based on whether or not the model exists in the database.

This causes an issue when trying to manage models and their attributes manually, e.g. in an importing script.

Expected Behavior

I would expect that setting attribute_data on a model behaves consistent whether or not a model is persisted.

Current Behavior

When a model is new (not persisted), added attribute_data is being run through a mapper, before adding it to the model's attributes.

When a model exists (persisted), added attribute_data is being inserted "as is" (although, JSON encoded).

Possible Solution

A possible solution would presumably rely on (or affect) Admin Hub implementation. It would be beneficial for transparency to decide whether or not to use the mapping method consistently or to use "as is" data.

The issue with "as is" (as I see it) is ensuring portability of attribute groups and attributes to Elasticsearch and other implemented usage of these throughout the API and implementations with Admin Hub and possibly storefronts, as this introduces a possibility of implementation developers to provide wrongfully structured data.

The issue with using the mapper method is lack of transparency when setting the attributes. Without transparency, "magic" happens and that can be difficult to implement correctly.

Maybe dynamically generated JSON schemas to help fill and validate attribute_data based on Attribute Groups and their Attributes could be a simple yet powerful solution.

Steps to Reproduce

A simple sample code run in a Console Command:

$category = \GetCandy\Api\Core\Categories\Models\Category::firstOrNew();
$category->attribute_data = [
    'name' => [
        'en' => 'Test',
    ]
];
$category->save(); // attribute_data contains different data in database depending on the initial persistent state of the model

Context (Environment)

Detailed Description

Possible Implementation

@glennjacobs
Copy link
Contributor

Having discussed with Alec, we agreed.

The best approach would be to set the model back to using raw attribute data on inserts and updates and then have a Laravel Action to handle the mapAttributes functionality.

In the interim, don't use firstOrNew :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants