If you are adding a product with a slash in it's SKU you are going to bump into a really cute issue if you are using the autocomplete in the product reference fields. Every time you are going to try to add an extra value in you multivalued product reference field the previous values are removed and replaced with the newly selected product.
I don't think that we should allow slashes in the SKUs.

CommentFileSizeAuthor
#1 1962144-commerce_product_sku_slashes-1.patch484 bytesartusamak
#1 1962144-commerce_product_sku_slashes-1-2.patch1.49 KBartusamak

Comments

artusamak’s picture

artusamak
he/him
Primary language French
Location Bzh
commented
Status: Active » Needs review
StatusFileSize
new 1962144-commerce_product_sku_slashes-1-2.patch1.49 KB
new 1962144-commerce_product_sku_slashes-1.patch484 bytes

Here is a patch fixing that in the spirit of the current implementation (patch 1).

One thought though, we are displaying this help text for the SKU field: "Supply a unique identifier for this product using letters, numbers, hyphens, and underscores. Commas may not be used." it reminds me a lot the machine name field type. We could have used a dedicated element type for that i guess?
I believe that instead of a set of checks of chars to exclude we should go for an explicit list of what we are allowing (patch 2).

amateescu’s picture

amateescu commented

While the second patch is clearly the better choice (even better if it was changing the check to use Drupal's #machine_name), that would be an API change and who knows what other funky characters might be right now in existing user's sites. So I'll vote for patch 1 :)

rszrama’s picture

rszrama commented

Yeah, we've gone over various restrictions for SKUs before and decided to leave them in their current state. However, the problem with patch 1 is there may already be sites using slashes in their SKUs who weren't using this widget or weren't referencing multiple products. Perhaps there is some other fix that doesn't involve restricting slashes?

amateescu’s picture

amateescu commented

Sure, we can fix the widget instead.

rszrama’s picture

rszrama commented
Title: SKUs should not allow slashes » Product autocomplete does not work for SKUs with forward slashes
Status: Needs review » Fixed

The problem here is that we copied some "dumb" code from core. It basically didn't define any arguments for the page callback, simply leaving it up to the menu router to supply bits of the URL... thus, the $string argument was only ever getting the first bit of the SKU preceding the /. I changed this to explode the path arguments, splice out the first 5, and then rejoin the final bits as the new $string, and it worked just fine with a local site using / in SKUs.

Commit: http://drupalcode.org/project/commerce.git/commitdiff/5cc24ff

thim’s picture

thim commented

This limit of split up based on the args causes issues with multi country/language setups.

Example: http://example.com/BE/en/commerce_product/autocomplete/commerce_product/...

Where BE represents the country
en represent the language

The rest is a normal autocomplete.

How do I resolve this?

I tried to use make the autocomplete path absolute but this does break the widget '#autocomplete_path' => '/commerce_product/autocomplete/commerce_product/line_item_product_selector/product',

thim’s picture

thim commented

I filled a seperate issue for the problem.
http://drupal.org/node/1976426

The current issue is closed so not need to reopen.

Status: Fixed » Closed (fixed)

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