Support from Acquia helps fund testing for Drupal Acquia logo

Comments

longwave created an issue. See original summary.

longwave’s picture

Title: Convert line items to plugins » Convert line item types to plugins
longwave’s picture

Status: Active » Needs review
FileSize
37.51 KB

First pass at this. Not sure yet that this is the right approach, this is just a direct conversion of hooks to plugins. The plugin interface and annotations need work at least, let's see what testbot thinks.

Status: Needs review » Needs work

The last submitted patch, 3: line-item-plugins.patch, failed testing.

longwave’s picture

Status: Needs work » Needs review
FileSize
37.54 KB

Status: Needs review » Needs work

The last submitted patch, 5: line-item-plugins.patch, failed testing.

TR’s picture

Some thoughts:

Should products also be line items so we can handle all order items in the same manner?
Is it necessary to have different entities for cart items and line items?

TR’s picture

As I work on #2672628: Convert TaxRate entities to use plugins I'm realizing that my changes to uc_tax.module will conflict with these changes in a big way. How do you want to proceed? Personally I would like to see the line items finished first, because I find that the current line item structure is a major hindrance to my work on uc_tax.module - there are several kinds of undocumented arrays that are being passed around and converted to/from line item arrays. It's a mess, and the best way to address it would be to make line item objects first.

longwave’s picture

Status: Needs work » Needs review
FileSize
37.74 KB

I am not sure products and line items should be handled the same way; there are a lot of differences about the way we treat products vs other line items, and generally we have either none or one of each line item type on an order.

I think we do need to go ahead and make line item objects. They should probably be entities, and perhaps the non-stored line items can just be dynamically created stub entities that are never saved? Not entirely sure what the best way to go is here, though. Maybe we should just start with a small step and turn them into value objects instead of the undocumented arrays they are at the moment.

Status: Needs review » Needs work

The last submitted patch, 9: line-item-plugins.patch, failed testing.

longwave’s picture

Status: Needs work » Needs review
FileSize
38.35 KB
TR’s picture

That looks pretty good, and I favor the gradual approach. So long as the patch doesn't break anything I think it's a step in the right direction.

For non-stored, they can just be normal classes instead of entities - if the classes implement the same interface as the corresponding stored entity then the two can be manipulated the same way. If there's implementation that it should be shared between the two it can be put into a trait.

  • longwave committed 8879cd2 on 8.x-4.x
    Issue #2626132 by longwave: Convert line item types to plugins
    
longwave’s picture

Status: Needs review » Active

Committed, back to active to finish this off though.