This patch just does a tiny refactor so that the field creation normally done inline in hook_enable() can be done elsewhere (specifically by Commerce Repair

It doesn't really do much except move a couple lines of code around.

CommentFileSizeAuthor
commerce.refactor_hook_enable_field_creation.patch5.05 KBrfay

Comments

rfay’s picture

rfay
he,him
Primary language English
Location Palisade, CO, USA
commented
Status: Needs review » Needs work
rfay’s picture

rfay
he,him
Primary language English
Location Palisade, CO, USA
commented
Status: Needs work » Needs review

Forcing test

rszrama’s picture

rszrama commented
Issue tags: +1.1 blocker

Tagging.

rszrama’s picture

rszrama commented

Was looking to put this in this morning, but on review I noticed that you're changing the logic with respect to hook_modules_enabled(). In those hooks, our various field creation functions were just supposed to look for newly defined bundles in modules that were just enabled and add fields to them where necessary. In your patch, you're switching it so that any time the hook is invoked without specifying any modules, you look through every enabled module for newly defined bundles and re-attempt to add fields.

Functionally, your code does more now, but I'm not even sure what would result in hook_modules_enabled() being called without anything in the $modules array. Is there something you came across in testing that this fixes?

rfay’s picture

rfay
he,him
Primary language English
Location Palisade, CO, USA
commented

If I changed the logic it was a mistake. My intent was only to refractor.

rszrama’s picture

rszrama commented

No worries - I just thought maybe you found a use case where hook_modules_enabled() wasn't being provided a list of enabled modules. It's easy enough to undo, but I didn't wanna just change it in case Commerce Repair depended on that if (empty($modules)) check.

Suppose it'll work fine without it?

rfay’s picture

rfay
he,him
Primary language English
Location Palisade, CO, USA
commented

OK, so I was wrong. I was trying to make it so that all the "rebuild fields" functions worked the same. For this one, I did change the interface so you could call it with NULL (probably should have been array()) and it would check everything.

IMO this *didn't* change what happens in any normal situation (where the incoming array must by definition be populated). But it did make it so you could do basically what the other related functions do.

I *can* reroll commerce_repair to get the list of modules ahead of time and call with that; it makes the code there less consistent, but that's not as important if you object to how this works.

rszrama’s picture

rszrama commented
Status: Needs review » Fixed

Ok, committed as is with a little more comments. I also moved the other configuration functions to be next to these in the code.

Commit: http://drupalcode.org/project/commerce.git/commitdiff/799af4a

rfay’s picture

rfay
he,him
Primary language English
Location Palisade, CO, USA
commented

Thanks Ryan - I've updated the project page.

Automatically closed -- issue fixed for 2 weeks with no activity.