Hi,

i got this Error Message when i want to create a bulk of Codes:

Fatal error: Call to undefined function commerce_coupon_save() in /usr/www/users/xyz/site/sites/all/modules/commerce_coupon/modules/batch/includes/commerce_coupon_batch.form.inc on line 128

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pcambra’s picture

Status: Active » Needs review
FileSize
1.14 KB

We should provide a way to save the coupons that is more consistent with Commerce code.

Patch attached with a commerce_coupon_save routine that invokes the entity controller.

pcambra’s picture

Attaching another version of the patch

This one includes a more wide transformation on how we're handling saving and commerce coupon objects. I think that the right step is to convert all our custom objects in more "commerce standard" stuff, this is an initial step.

I'm willing to give out a hand as co-maintainer as I'm seeing that there are tons of stuff to fix in here.

dgastudio’s picture

after apply the first patch and create coupons, the coupons entities are created but doesnt contain the coupon code.

after remove and apply the second patch:

Notice: entity_extract_ids() [function.entity-extract-ids]: The script tried to execute a method or access a property of an incomplete object. Please ensure that the class definition "CommerceCoupon" of the object you are trying to operate on was loaded _before_ unserialize() gets called or provide a __autoload() function to load the class definition in entity_extract_ids() (line 7403 of /home/u4586/domains/makey.u4586.argon.vps-private.net/includes/common.inc).
Notice: entity_extract_ids() [function.entity-extract-ids]: The script tried to execute a method or access a property of an incomplete object. Please ensure that the class definition "CommerceCoupon" of the object you are trying to operate on was loaded _before_ unserialize() gets called or provide a __autoload() function to load the class definition in entity_extract_ids() (line 7408 of /home/u4586/domains/makey.u4586.argon.vps-private.net/includes/common.inc).
EntityMalformedException: Missing bundle property on entity of type commerce_coupon. in entity_extract_ids() (line 7409 of /home/u4586/domains/makey.u4586.argon.vps-private.net/includes/common.inc).

pcambra’s picture

Status: Needs review » Needs work

#3 probably you need to clear cache after applying the patch

Setting to needs work as I want to include a proper roadmap for making this module standard with commerce itself.

geek-merlin’s picture

pcambra, can you please elaborate the idea behind your refactorings?
"do it like other modules do" is not necessarily "do it the right way(tm)".

i think

$coupon=new CommerceCoupon;
$coupon->save();

is more state of the art than

$coupon = commerce_coupon_create();
commerce_coupon_save($commerce_coupon);

i think we really don't want that functions that may or may not contain logic that belongs into the class.
(if that's "the commerce way" i think we should file ann issue there ;-)

pcambra’s picture

Axel see, for example, the product controller class and the product module of commerce, here for the controller http://drupalcode.org/project/commerce.git/blob/refs/heads/7.x-1.x:/modu... and here for the usage http://drupalcode.org/project/commerce.git/blob/refs/heads/7.x-1.x:/modu...

We should do an effort to respect the coding standards for commerce contrib here, see also http://www.drupalcommerce.org/development/standards

pcambra’s picture

Title: Batch creation not working » Abstract and standardize commerce coupon to fix general coupon problems
Status: Needs work » Fixed

I've committed a patch that moves the coupon entity into the drupal commerce standards, I haven't moved it from its file which is a standards problem as well but we don't want users to do a registry rebuild (or do we?) just because this change.

This way we're benefiting of all the cool things such as transactions that drupalcommerceentitycontrollers support.

More changes in this patch:

  • Code style and variable naming fixes (minor)
  • Changes in commerce coupon batch so coupons are now correctly generated both from normal UI and batch
  • Fix for #1380650: Batch Generation - pre and postfix isn't used and probably more issues that I need to review.
  • API cleanup for be more consistent in deletion and creation of coupons.
  • Remove inconsistent file including in .info files (minor)

I'll suggest to move forward and keep the debate about some of the changes in #1413126: Commerce coupons roadmap, my goal for today was to make the "damn thing" work and it seems I could ;)

pcambra’s picture

bojanz’s picture

Note that having an entity class is the Entity API convention, matching Og, Message, and many Commerce contribs (including pretty much all code that I write).
The only reason why Commerce doesn't follow it is because entity classes appeared when Commerce was already written. We attempted to do the conversion before 1.0 but it was too big of a task. Definitely an approach that will be considered for Commerce 2.x
So while it's fine to convert to a controller if you prefer it that way, there should be no confusion about that being a preferred way. It's not and doesn't need to be.

In any case, thank you Pedro for taking on commerce_coupon, hoping to see it in a working state soon.

geek-merlin’s picture

Status: Fixed » Needs work

thank you nojanz for clarifying, that was a big riddle to me!

so setting this as "needs work" as a reminder.

pcambra’s picture

Priority: Critical » Normal

As the coupons are now working in the crud parts, let's mark this as normal priority and revisit later, maybe synching with commerce for this part.

bojanz’s picture

Well, isn't it now synced with Commerce? I'd mark this as fixed.

My point was that if you have a working module that uses approach X, there is no need to rewrite it for approach Y.
That doesn't apply to coupon of course, since it's not a working module and probably needs a full rewrite anyway.
Just something for those running into this link through search or whatever.

pcambra’s picture

I agree with bojanz in marking it fixed, but I also understand axel point, I'd say this is priority 0 for me at the moment as the coupon entity is now working properly in terms of CRUD and API.

Axel, feel free to mark this as fixed or leave it open, we'd probably need to revisit the entity definition when commerce aims 2.x

geek-merlin’s picture

prio 0 is fine for me as long as new or refactored code should be done like bojanz pointed out in #9.

leaving this open as marker and documentation.

pcambra’s picture

Status: Needs work » Closed (won't fix)

Marking this as closed as we're going to rely Coupons 2.x branch in Commerce Discount and Coupons won't define anymore entities whatsoever.