ProductVariationSkuConstraintValidator crashes if the SKU field is empty [#2891770]

If SKU is not used for products (left empty), then the following error is thrown when product is created/updated:

Trying to get property of non-object in Drupal\commerce_product\Plugin\Validation\Constraint\ProductVariationSkuConstraintValidator->validate() (line 17)

/**
   * {@inheritdoc}
   */
  public function validate($items, Constraint $constraint) {
    $sku = $items->first()->value;
    if (isset($sku) && $sku !== '') {
    // ...

The offender is line 17 ($sku = $items->first()->value;) which tries to get the value (->value) from an object which does not exist ($items->first() returns null on empty fields).

Validator should check if field has values before attempting to proceed with further validation.

Comment	File	Size	Author
#9	2891770-9.patch	883 bytes	bojanz
#9	8.x-2.x: PHP 5.5 & MySQL 5.5, D8.4 418 pass
#5	commerce-sku_constraint-2891770-5.patch	1.66 KB	baikho
#5	8.x-2.x: PHP 5.5 & MySQL 5.5, D8.4 418 pass

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

3 July 2017 at 08:45

maijs created an issue. See original summary.

Comment #2

bojanz CreditAttribution: bojanz at Centarro commented 3 July 2017 at 09:04

Title:	SKU constraint validator is trying to get property of non-object	» SKU constraint validator assumes that the field is always required
Priority:	Normal	» Minor

The SKU field is always required, so you encountered your bug after hacking the base field :)

Still, it's something we can improve.

Comment #3

maijs CreditAttribution: maijs at Wunder commented 3 July 2017 at 11:04

I did not touch the SKU field programmatically in any way. I merely hid it from display in form display settings.

Comment #4

bojanz CreditAttribution: bojanz at Centarro commented 3 July 2017 at 11:07

Ah, that had the same effect then. Core doesn't prevent you from hiding field widgets of required fields, it seems.

Comment #5

baikho

Dutch

CreditAttribution: baikho as a volunteer and at Access commented 31 October 2017 at 02:13

File	Size
commerce-sku_constraint-2891770-5.patch	1.66 KB
8.x-2.x: PHP 5.5 & MySQL 5.5, D8.4 418 pass

I'm having the same issue when hiding the SKU field in the form display of a variation. It also does not seem to validate when using IEF as widget for the Variations in a product without SKU field being hidden.

Added a patch which solves it for me, iterating through the $items in the ConstraintValidator

Comment #6

drugan CreditAttribution: drugan as a volunteer commented 31 October 2017 at 07:24

I don't like the idea of having variations without SKU. It imposes a need to always check if the SKU exists on a variation and decide what to do if it does not. Assign a fake SKU? Leave it NUll? No. Also, I think that this approach possibly may introduce a lot of bugs in the future.

If you don't care about SKU and would be happy if it is assigned silently on the background of a hidden field then you can try this:

#2755529: Product variant bulk creation
patch: https://github.com/drupalcommerce/commerce/compare/8.x-2.x...drugan:vari...

Comment #7

maijs CreditAttribution: maijs at Wunder commented 3 November 2017 at 12:02

@drugan, what is the main technical concern here? If shop can functionally operate without a SKU, where does the need to check for SKU value come from? The way I see it is that SKU is just a base field that comes along for convenience and there's nothing that depends on its value. The reason why this issue was created was to address the assumption in the ProductVariationSkuConstraintValidator class that SKU is present. Given reasonable cases where it's not used, it should not be concerned or merely fail gracefully.

Comment #8

bojanz CreditAttribution: bojanz at Centarro commented 3 November 2017 at 13:06

Status:

Active

» Needs review

Let's test this.

-    $sku = $items->first()->value;
-    if (isset($sku) && $sku !== '') {
-      $sku_exists = (bool) \Drupal::entityQuery('commerce_product_variation')
-        ->condition('sku', $sku)
-        ->condition('variation_id', (int) $items->getEntity()->id(), '<>')
-        ->range(0, 1)
-        ->count()
-        ->execute();
+    foreach ($items as $item) {
+      $sku = $item->value;
+      if (isset($sku) && $sku !== '') {

The existing code already skips the check when the field is an empty string. That also follows the Symfony validator best practices (where required fields are enforced by their own constraint). The actual problem is $items->first() crashing for empty fields.

I am not a fan of the iteration over items. A field can't have multiple SKU items.
I suggest we try adding something like this to the top of the method:

    if (!$item = $items->first()) {
      return;
    }

And then using $item->value below as usual.

Comment #9

bojanz CreditAttribution: bojanz at Centarro commented 3 November 2017 at 13:09

Title:	SKU constraint validator assumes that the field is always required	» ProductVariationSkuConstraintValidator crashes if the SKU field is empty
Priority:	Minor	» Normal