Add line item field data when adding to cart

Patch in #5 works to add custom field data. OrderItemMatcher via CartManager is also checking against the custom field value as expected to not combine order items with different custom field values. However, there is no validation occurring on custom fields. E.g. if a custom order item field has a data type mismatch or is required yet not included in the payload there is no exception thrown and order item is added to the order with no value in the field.

Is there a way to get the order item type's fields and add them to the static::validate method? Or, at the very least we may want to add passed properties from $meta into the validate method. Currently, the properties to validate are hard-coded as "quantity" and "purchased_entity".
static::validate($order_item, ['quantity', 'purchased_entity']);

Yes, we need to add validation, which makes me wonder if passing "fields" isn't a better option since it's more explicit than assuming everything passed under meta is a field... Though quantity is already a field, so it'd be a bit inconsistent to have the quantity field set via meta and the rest via "fields".

Agreed. So, in that case can we just get the field definitions from the created entity? Get all the field names then pass to validate()? In this case both the 'quantity' and 'purchased_entity' field names are part of the base fields and any custom fields will also be included. Shouldn't this validate all fields on the entity before adding them to the cart?

        $field_definitions = $order_item->getFieldDefinitions();
        foreach ( $field_definitions as $field_definition ) {
          $field_names[] = $field_definition->getName();
        }
        static::validate($order_item, $field_names);

The only issue becomes multiple order items. We would want to save all of the temporarily created order items and validate them first before calling addOrderItem. Otherwise, some items may get added to the cart before a failed validation of another item.

Adding order item field validation via field definitions from the entity. Then, waiting until all order items are validated before adding to cart.

@nicer: I don't think your patch is correct? Why would you be validating fields that aren't passed in the payload?
Just pass meta?

Why would you be validating fields that aren't passed in the payload?

I chose this direction because if the payload does not include a required, custom field then that field will not be validated. Instead, the order item will be added to the cart with a null required field. Thinking decoupled, the payload is controlled by another application and there is nothing that says the payload must include all of the required fields. This would solve that.

We should just pass the meta array? Or just the fields present under meta that match field names?

As stated above, I feel we should make sure to validate required, custom fields that are not included in the payload. I understand by passing all of the field definition names we are passing fields that do not need to be validated—especially several base fields (e.g. uid, created, etc). If we just want to focus on the custom fields we could scrub the base fields (I believe there is an isBaseField method). If desired, left with only custom fields we could scrub those that are not required. We'd then need to merge with 'quantity' and 'purchased_entity' for validation.

Furthermore, the 'meta' array may contain non-fields, as you hinted, such as 'combine'. Even if we were to add a separate 'fields' array we would still want to validate required, custom fields missing from the payload. I feel getting the field definitions is still that solution. This, then, leads me to two reasons for not including a separate fields array:

1. Now, 'quantity' and 'purchased_entity' are set by meta even though they are fields. If the rest were set by fields that would become inconsistent (as stated in comment #7).
2. If we go through the process of getting the field definition names from the entity why do we need a separate fields array? We've already defined what the fields are without separating them.

I'm not suggesting to pass the fields under "meta", just to be consistent with the quantity. But we shouldn't blindly validate all fields. We should only validate fields passed under meta.

So we can get the field definitions and build an array of field names for the fields that are passed under meta.

Sorry for the confusion. Sounds like you want to do something this?

        // Get the field definitions for the order item. Then, build an 
        // array of field names that exist in the meta array.
        $field_definitions = $order_item->getFieldDefinitions();
        foreach ( $field_definitions as $field_definition ) {
          $field_definition_name = $field_definition->getName();
          if ( array_key_exists($field_definition_name,$meta) ) {
            $field_names[] = $field_definition_name;
          }
        }
        static::validate($order_item, $field_names);

This would identify what are field names in "meta" and would only validate the fields that were passed. Am I correctly understanding?

Patch allows order item field data to be passed in 'meta' and determines what properties are field names by cross referencing against item's field definitions. Validates only passed field names.

With the patch from #14, the purchased entity is no longer validated, which is incorrect.

Are the quantity and purchased entity fields defined in the order item already? If so, they would be in the field definitions we are looping through.

But they aren't present in the "meta" array, and these need to be validated.

Instead of hard-coding the quantity and purchased_entity fields, can we edit #16 to add isRequired() during the field definitions loop. Quantity is not required anyway and will be validated if included—otherwise it defaults to 1. And, purchased_entity IS required for an OrderItem entity.

The benefit of adding isRequired() is a developer who adds a custom required field to their order item bundle will have it included during validation in case it is missing from the payload.

Suggested changes:

        $field_definitions = $order_item->getFieldDefinitions();
-        $field_names = ['quantity', 'purchased_entity'];
-        foreach ($field_definitions as $name => $field_definition) {
-          if (array_key_exists($name, $meta) && !in_array($name, $field_names, TRUE)) {
+        foreach ($field_definitions as $name => $field_definition) {
+          if ( $field_definition->isRequired() || (array_key_exists($name, $meta) && !in_array($name, $field_names, TRUE)) ) {
            $field_names[] = $name;
          }
        }
        static::validate($order_item, $field_names);

The resulting $field_names array now becomes:
["type","purchased_entity","unit_price","created","changed","field_custom_name"] (+ "quantity" if included in $meta)

I like this idea a lot because a required custom field that is missing from the payload will now fail validation. Otherwise the order item could be created with a null value in a required field.

@nicer concerning #20, I feel like this is a great idea!

We would once again validate fields that might not be included in $meta, BUT in this case it would greatly benefit us, as we will see if we missed a required field!

+1 from my side, I'll add it to this issue fork branch.

Alright, I did some renaming and adjusted the code similar to how it was suggested by @nicer.

Please review! Works great on my side :)

Static patch attached for the time being.

Ok, I think we still have 2 problems left we need to solve:

Problem 1:
Currently, if we provide a field, that doesn't exist on the order item type, the field value simply gets skipped, and the order item will be created without the field value. I think one would expect to get a 422 error instead, as the provided field / field value doesn't exist on the entity and can therefore not be used.

Problem 2:
Now that we expose all order item fields, people can easily abuse it by overriding the unit price of the order item like so:

{
  "data": [
    {
      "type": "commerce_product_variation--frame",
      "id": "c8fb0673-9519-4e8b-a9d5-7a42e8518065",
      "meta": {
        "combine": true,
        "quantity": 1,
        "overridden_unit_price" : true,
        "unit_price" : {
            "number" : 400.00,
            "currency_code" : "EUR"
        }
      }
    }
  ]
}

Meaning we need further validation on the person doing the post request, whether he is illegible to override sensible fields.

Ok, I fixed Problem 1. First I thought about introducing the original payload structure, with a dedicated "fields" key for the payload.

This way we would have a clean separation between field data and all other meta properties (currently only "combine"), but this would cause a BC and nobody likes those, so I reverted it again.

Fixed Problem 2, now the user is not able to set any based fields apart from "quantity". This should be sufficient for now. But we should think about cases, where a developer would like the user to be able to override specific base field values as well.

I talked about this internally with @Anybody → and we thought about one conclusion, where we would validate the meta field values through the "add_to_cart" form mode (or a custom form mode specific to this module).

This way, an admin could easily enable / disable fields he wants to be available / unavailable through the cart add API call.

@jsacksick, what do you think about this? Should we implement it here or as a follow-up issue? Or not at all and keep the current logic as is?

New static patch for the time being.

Don't think hardcording the "field_" prefix is a good idea. The "field_" prefix can be easily removed and in fact, in many of the projects I'm working on, we just skip it.
Also, not a big fan of enforcing required fields to be submitted, I see this as a breaking change... And +1 on preventing setting the unit price. I started by a simple patch in #16, and this is now going into a complete different direction.

I think we should validate only the fields that are passed, validating a field that isn't passed could be blocking / limiting and frustrating :). And we should prevent setting the unit price, other than that... Passing fields via meta remains a workaround as usuaully the right way to do this is via attributes, this was needed on a project I was working on last year, but this approach is also problematic for more complex fields (and reference fields for example, as this would allow for setting any reference without real access checks / validations...

To prevent setting base fields, we used to have the following code in OrderProfile::propertyDefinitions().

$excluded_fields = array_merge(
      array_values($entity_type->getKeys()),
      array_values($entity_type->getRevisionMetadataKeys()),
      ['is_default', 'data', 'created', 'changed'],
    );
    $fields = array_filter($fields, function ($item) {
      return !($item instanceof BaseFieldDefinition);
    });

#30 LGTM! Thanks for the snippet!
I guess for such a public API we can assume the price should never be adjusted in public?

The only use-case I can image is for donations. But we could solve that in a follow-up, if someone needs it. Eventually by

• hook
• setting
• permission

PS: Best would be to ensure the given values are validated like in the regular UI "Add to cart form". I'm unsure how far we're away from that currently with static::validate() and if we could run them through the regular fields validation.

Do we furthermore need to check, if the user is allowed to access the fields for the given values? (#access)
@Grevil and me discussed if it wouldn't be best to have a commerce_api form display mode wo could use to decide which fields are allowed?

Alright, this should do the trick!

Don't think hardcording the "field_" prefix is a good idea

We are now using a similar approach to your snippet and if a base field is being set, we throw a 422 error.

Also, not a big fan of enforcing required fields to be submitted

Removed that. We are now only validating fields set through "meta".

All done. Please review!

Tests are failing, and we might need new tests as well.

The tests should check that the validation is working, that the unit price field cannot be set, and that other custom fields can be set.

The given meta fields 'arity' are not valid entity fields.

I don't think "arity" is a valid meta key? There seems to be no code using "arity" and it isn't documented in https://www.drupal.org/docs/8/modules/commerce-api/cart-and-checkout/add... → .

I'll adjust the test accordingly.

Arity is needed, I think this is necessary when you don't want to combine order items... this is the "index"/"delta".

Ah ok, thanks for clearing this up!

Alright, all done! Sorry for the noise, I haven't worked with DrupalCI for a while now and my local test environment didn't show the last phpcs error.

All desired tests were added and all tests are green. Please review!

FYI: Just found out, that the "combine" meta key doesn't properly work and "arity" is indeed not used anymore. Both also applies to current 8.x-1.x and doesn't have anything to do with this fork's changes.

I'll provide a seperate issue for that.

Thanks @Grevil! Nice work!!

This LGTM, I just left some comments for the comments and code-readability. As we're both non-native speakers, I think it would make sense to let @jsacksick have a look first and perhaps improve the naming and comments. Our phrasing will never be perfect... :(

What's good for him is great for us :)

RTBC code-wise from my side.

Checked very quickly the logic, the first thing that stands out to me is the fact that we're passing the $meta array as is to createFromPurchasableEntity(), prior to any filtering, is that expected?

@jsacksick, yes I kept that from the original patch. It shouldn't matter, as the order item is only created, but not saved nor added to the cart. And if there are any invalid fields / unallowed fields we'll throw a 422 error before adding it to the cart.
This way we can easily get the field definitions from the entity without injecting any additional services.

If you don't like it, we can go back to injecting the field manager again, like in https://git.drupalcode.org/project/commerce_api/-/merge_requests/21/diff...

Then, we can filter out invalid fields before actually creating the order item. You decide! :)

Providing static patch for the time being.

Merged, thanks!

Great, thank you @jsacksick! Nice to have this new, important feature in action :)

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