Moving product classes to CMI shouldn't be too complicated. Attributes is a different story. That will probably require a major refactoring, and will affect contrib modules like uc_attribute_views.

EDIT

Attribute handling is very messy in UC. Just to lay out the complexity of attribute handling, this is the list of tables that contain some sort of info about them. As you can see, this looks messy, and will most likely need to be redone in different way s as CMI:

• uc_attribute holds the attribute definitions
• uc_class_attributes holds attribute info specific to each product class and relates to the attribute ID defined in uc_attribute. It can override things like attribute label and display
• uc_attribute_options holds options for each attribute
• uc_class_attrubute_options basically holds options for product class, (references the option ID defined in uc_attribute_options) and can override item from the option defined in uc_attribute_options such as price and weight, etc (this is where things start to get really messy)
• uc_product_attributes copies some of the attribute info into each product instance (node) and allows for some overriding

Looks like class attributes can be handled in a different issue, if we want to do that too. So I can submit a PR for just the classes table.

(For attributes, I'm thinking only converting uc_class_attributes and uc_class_attributes_options if that's possible, since the other tables rely on nid.)

(For attributes, I'm thinking only converting uc_class_attributes and uc_class_attributes_options if that's possible, since the other tables rely on nid.)

uc_attribute and uc_attribute_options do not rely on NID. They are definitions, and will probably need to be converted too, since uc_class_attributes and uc_class_attributes_options refer to them through the attribute ID and the option ID.

I'm thinking that we can encapsulate some of these without the need to create separate json files. For example, the CMI containing the attribute definition for a specific attribute can contain options definitions. Currently, with the database model, options are never available or referred to outside the attribute that contains them.

A general consideration when moving db tables to CMI is whether there are db queries that JOIN to the table being replaced. A quick trawl through my moderately complex UC site (so covering some UC contrib modules and custom modules) searching over the attribute-related tables listed above, turns up the following:

• Several uc_attribute.module functions JOIN to uc_attribute_options
• uc_attribute_condition_ordered_product_options_list has a JOIN to uc_attribute_options
• uc_clearance has a lookup from uc_attribute_options but this could easily be replaced by config code.
• uc_price_per_role contrib module has a fairly complex db query in uc_price_per_role_option_prices_form that selects from uc_attribute_options that JOINs to uc_product_options

So some things to consider with attributes, but if we restrict this issue to the classes, it should be easier to merge.

I noticed that classes can also be defined in code so we definitely should be able to get it into CMI.

I've got a PR for converting the product classes to CMI. This is the more straightforward part.

I made a follow-up for attributes: #519.

Thanks, @herbdool. There are lots of test failures.

Technically, also, this should not be stored as uc_product.settings.json as this is not a product general setting. I'd suggest we split each new class into its own json file, the same way content types are stored. In this case this could be done as in uc_product_class.[PCID].json. This makes importing/exporting more granular (otherwise you'd be importing/exporting all classes and product settings at once, which may not be desirable).

Also, other contrib modules that will be affected by this change:

• uc_who_bought_what (@laryn)
• uc_recurring (@alanmels and @hosef)

@argiepiano to me it seems like too much to have separate files for each product class, when all Ubercart is doing is duplicating stuff that's already in node.type.CLASSNAME: the title, and description. Which seems odd to do that anyway. The class form could just pull the info from that config and save to it (which it's mostly doing already).

I was comparing it to webform which only has a checkbox on the content type and saves it to webform.settings like: "webform_node_mywebform": true. Maybe that can be improved too but I think we can simplify ubercart a bit.

And fix tests.

I've got tests passing now. I've simplified what it stores, which is now just the content type machine name.

Bigger question is what to do about the modules you've mentioned @argiepiano:

• https://github.com/backdrop-contrib/uc_who_bought_what
• https://github.com/backdrop-contrib/uc_recurring (no releases yet)

If we're going to introduce breaking changes we may want to make sure we make all the changes we want and then make a new 4.x branch. That's too bad, but I guess that's what we'd have to do.

I'm thinking now that we could make this backwards-compatible by keeping the table and syncing it when the config gets updated. I'd need to keep some of old code and also add a hook_config_update(). It's messy but at least will make it more robust for importing config.

I think that's sensible.

However, since this was prompted by the need to import/export classes and attributes, and given that changing attributes to CMI is likely to be a whole lot of work, I wonder if this should (1) be a major release, (2) merging this PR should happen only after ALL this functionality (classes AND attributes) has PRs to convert everything to CMI. In which case, there is no need to keep the table (as this will be a new major release).

If a major release is the way to go, I'd also suggest creating API functions that don't exist that the moment, as in product_class_load() etc. instead of having to deal directly with config.

EDIT: oops, I see you already did this ;-)

Actually uc_product_class_load() already existed but should have been used everywhere. I created functions to create and sync the config.

If you are amenable to a major release then sure, we can make sure we put in the breaking changes into it. I guess it should be a new branch if you still want to maintain the older version for awhile?

I think @bugfolder will need to decide on this. I co-maintain/help with the modules, but he's the "boss" as he uses this much more than me.

he's the "boss"...

I did not realize this. I usually rely on @argiepiano for judging what makes technical sense, and am happy to do so here, but I'd sure like to do a bunch of testing before any release.