October: Error with saving ThemeExport model object

[kharanenka]

• OctoberCMS Build: 455
• PHP Version: 7.2
• Database Engine: MySQL
• Plugins Installed: irrelevant

Description:

Error occurs when saving \Cms\Models\ThemeExport model object.
Problem with presence of public $ attributes array in model class.

source code:

        $obThemeData = ThemeExport::where('theme', self::THEME_CODE)->first();
        if (empty($obThemeData)) {
            ThemeExport::create([
                'theme' => self::THEME_CODE,
                'data' => $this->sThemeData,
            ]);
        } else {
            $obThemeData->data = $this->sThemeData;
            $obThemeData->save();
        }

Error:

 Unexpected type of array when attempting to save attribute "folders", try adding it to the $jsonable property.

Steps To Reproduce:

[bennothommo]

@kharanenka We need some more info here. Where is the error occurring? Are you extending the ThemeExport model in any way?

[Exodon]

This bug appeared based on this steps:

1. Create database using laravon environment in windows to run October cms.
2. Download the installer from October CMS website and run the setup.
3. After installation of the October CMS, I connect my October account and install the project based on the ID. Automatically it install the plugins and theme attached.
4. Copy the scripts from the _build_ theme folder and past on the root of the October cms project.
5. In cmd Run npm install and then npm run development
6. Run the commandphp artisan shopaholic:generate.fake.data
7. Choose the option sneakers and then choose 10 times to recreate the products
8. Then run php artisan shopaholic:generate.theme.fake.data
9. choose this option Select theme [sneakers]
10. Give the error output of > [Exception]
    Unexpected type of array when attempting to save attribute "folders", try adding it to the $jsonable property.

I hope is enough info to reproduce the error....

[bennothommo]

@Exodon That does not sound like an issue with October - it more looks like an issue somewhere in the Shopaholic plugin. You will need to contact the developer of the plugin to resolve that issue.

[kharanenka]

I am developer of Shopaholic plugins. Please read my message carefully.

You can get error with using easy code:
Example 1 (updating ThemeExport model):

        $obThemeData = ThemeExport::first();
        $arData = (array) $obThemeData->data;
        $arData[] = 'test';

        $obThemeData->data = $arData;
        $obThemeData->save();

Error:

Unexpected type of array when attempting to save attribute "data", try adding it to the $jsonable property.

Example 2 (creating ThemeExport model):

        ThemeExport::create([
            'theme' => 'test',
            'data' => ['test' => 1],
        ]);

Error:

Unexpected type of array when attempting to save attribute "folders", try adding it to the $jsonable property.

P.S. If you close such questions without checking, then there will be no success in our business. I have to pay @daftspunk , @alekseybobkov attention to this incident.

[bennothommo]

@kharanenka With respect, your original issue report had no steps on how to replicate the issue, no reference to Shopaholic and apparently you found it irrelevant to tell us what plugins you have installed, therefore I made the request for more information.

When @Exodon responded, their only references to Shopaholic were two artisan commands referencing Shopaholic CLI tasks which is outside our realm of responsibility unless it can be demonstrated that it is because of some form of code in our core and not because of the plugin.

I am very happy to help as long as you are able to provide as much information up-front as possible. Help us, to help you.

[Exodon]

guys let me know if you need more info to be easy to track the problem....

[kharanenka]

Error is not related to plugins. There is an obvious error in ThemeExport model.

Now core has 3 models (ThemeData, ThemeExport, ThemeImport) for 'cms_theme_data' table. Do I understand correctly that it is not possible to write data to 'cms_theme_data' table with using ThemeExport model?

Should ThemeExport model support the ability to save data in 'cms_theme_data' table?

[Samuell1]

@kharanenka If you try to define folders as empty array does it work?

[kharanenka]

No, i have error with 'data' field

Unexpected type of array when attempting to save attribute "data", try adding it to the $jsonable property. 

[Samuell1]

Oh i see you getting different errors for create and update, if you add manually $jsonable to model with that values does it works?

[kharanenka]

Yes, if i add public $jsonable = ['folders', 'data']; to ThemeExport model, then it works.

But I'm not sure that this is enough to not have problems in future with $attributes array. Because folders field is not in cms_theme_data table. I'm not sure that this is necessary to fix, because I can save theme data if I use ThemeDat model.

[daftspunk]

This appears to be a classic case of XY problem. Why are we saving the ThemeExport model in the first place?

That does not sound like an issue with October - it more looks like an issue somewhere in the Shopaholic plugin.

@bennothommo is correct to suggest this is caused by a plugin, because nowhere does the core ever call save() on the ThemeExport model.

Why is Shopaholic calling save() on the ThemeExport model?

Now core has 3 models (ThemeData, ThemeExport, ThemeImport) for 'cms_theme_data' table. Do I understand correctly that it is not possible to write data to 'cms_theme_data' table with using ThemeExport model?

Should ThemeExport model support the ability to save data in 'cms_theme_data' table?

• ThemeData will save to cms_theme_data
• ThemeExport and ThemeImport are basic model classes that do not map to any database table

Hope this helps!

[kharanenka]

Strange use of classes of models that do not work with the database.Why didn't you take out this logic in separate classes (not model classes)?

The task that I solved is simply to save data in cms_theme_data table. I found 3 models that are used for this table. I took one of them and it worked. Now it turns out model classes does not fulfill its function and only adds confusion. What do you think about it?

[daftspunk]

The table is defined in those classes so we can leverage the FileUpload (System\Models\File) relationship, which is stored in the database and references the related table name. In theory we could put any table in there, but seeing as it is an exisiting one, I understand the confusion.

@w20k @bennothommo @LukeTowers Perhaps we should include an explicit exception on these semi-functional models (ThemeExport, ThemeImport) to make this more obvious

public function save(...) 
{
    throw SystemException("This model is not intended to be saved, please use ThemeData instead.");
}

[LukeTowers]

@kharanenka

Plugins Installed: irrelevant

It is not irrelevant, it is included in the issue template for a reason; proved entirely valid in this case. You may not realize it, but writing that communicates to the core developers a lack of respect for our time. As support is free, I would highly recommend that you make more of an effort to respect the time and contributions of the people volunteering their time to help you solve your issue. As the saying goes, you catch more flies with honey than with vinegar.

This has been closed by https://github.com/octobercms/october/commit/1587c7f49cd92f3b870c118758fafcd4061c4376

[lautsevich]

Hi everyone!

Let me add my 50 cents to this case.

@kharanenka, have found a bug which brokes one third-party products for October CMS. It's definitely a bug because 'it works before' and 'is broken' after some of the latest CMS updates.

Just believe we respect the time of the core team and trying to do our best to make October even better. That's why this bug was reported. You have to be very motivated to spend your time to do report bugs in open-source projects, right?

And if we're talking about the respecting the time of each other, IMO it's not appropriate to close the bug report less than a day later without the deep dive into the case! Especially when core team member forwards the reporter to himself (some kind of recursion 🤪). And the question is, why are we rushing like this? In the end, Andrey was right in the sense that isn't a bug of third-party code, but the bug of October CMS itself, right?

As for the bug itself. The model classes have to work with the DB on writing as well as on reading. It's how things work in Laravel world. And what's the result of this bug report? This is not a solution, but a disguise. This is not the best way to make October the best CMS for the developers. Besides this is a bad sign to the enterprise businesses who want to use October CMS. 🙁

So, I propose to shake the hands and to be more attentive to both sides: the community and the core team. We do the one job: developing October CMS, developing plugins or even just reporting the bugs and suggest ideas. 🤝

We have a suggestion. Reopen this issue, please, and we'll fix the bug! I think this is the winning way for October CMS. @bennothommo, @LukeTowers, @daftspunk, how do you feel about this?

Looking forward to hearing from you! Have a nice day or whatever the time is now for you! 😉

[LukeTowers]

@kharanenka @lautsevich

It's definitely a bug because 'it works before' and 'is broken' after some of the latest CMS updates.

This was not mentioned. This should never have worked period, so please post the version that it used to work in so that we can verify that.

it's not appropriate to close the bug report less than a day later without the deep dive into the case

We close bug reports when we deem them solved to clear them from our list to deal with. Just because a report has been closed however doesn't mean that the conversation has to stop, if further discussion reveals that there is still more work to be done to solve the issue then it's easily reopened, closing an issue is purely an administrative action. If we had closed and locked the issue then you could feel offended 😉

The model classes have to work with the DB on writing as well as on reading.

In this particular case we do not want to encourage developers to use these classes when they could just as easily use the class intended for this purpose: ThemeData. If you can provide a reason why you want to use ThemeExport instead of ThemeData then I would be happy to discuss that; but we're not going to support a use case where it makes no sense to do so other than "it has to". Having that class be a model class is a workaround in the first place, so we do not want developers encouraged to use it when we could rewrite it to make it "proper" in the future.

[daftspunk]

The problem was a massive chunk of information was missing from this ticket (why are you doing that?). The best outcome here is to study XY further, know when it exists and know how to deal with it. People naturally speak from the position they are in now, not the position from which they started, so XY is inevitable and common. These types of conflicts are easily resolved by forcing someone to retrace their steps and the solution is always to ask why? Why are you doing that?

To clarify "XY Problem" is when someone determines a solution to a given problem and seeks help with their solution, instead of seeking help with their problem. In some cases, they may falsely assert that their solution is the best and support is in no position to determine because they are blind to the backstory. It results in a massive waste of energy and resources for the technical support member who has to basically play a guessing game to figure out what the person wants. Nobody is expected to support people blindly, nobody is expected to solve riddles. Cut to the chase and ask directly what is the answer to this riddle...

_Let's pause fixing your solution for a moment, what are you actually trying to achieve here?_

The requester should be more mindful of this when logging issues. Optimally the requester should provide this information from the start. The technical support should be direct and there is a difference between being rude and being direct. In general, to avoid wasting time for everyone, this is a shared responsibility for all.