I think 128 characters is not enough for real life use cases. May be 255 would be great?

CommentFileSizeAuthor
#6 commerce_autosku-maxlength_on_sku_pattern-1189654-6.patch909 bytestmsimont
#1 1189654-1-max_length_patter_field.patch1.39 KBtwistor

Comments

twistor’s picture

twistor commented
Status: Active » Needs review
StatusFileSize
new 1189654-1-max_length_patter_field.patch1.39 KB

I agree. The database needs an update as well.

rbayliss’s picture

rbayliss commented
Status: Needs review » Fixed

Yeah, makes sense to me. Even 255 could probably be extended if necessary, since tokens usually take up more space than the values they replace. This brings up another point, which is that we should be limiting the length of the final SKU to commerce_product's limit of 255 characters, but I will create a separate issue for that.

Committed. Thanks!

rbayliss’s picture

rbayliss commented

Just a note that I changed this hook_update_n as per http://drupal.org/node/150220.

Status: Fixed » Closed (fixed)

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

tmsimont’s picture

tmsimont commented
Status: Closed (fixed) » Active

Looks like that last patch wasn't committed properly. Only the database was updated. I am using the latest dev, and there is no '#maxlength' property set on $autosku['pattern'] in commerce_autosku_form_commerce_product_ui_product_type_form_alter (which is second part of patch in #1)

It looks like the default must be 128 (form API?) or something, because that's what the front end limits the input to...

tmsimont’s picture

tmsimont commented
Status: Active » Needs review
StatusFileSize
new commerce_autosku-maxlength_on_sku_pattern-1189654-6.patch909 bytes

patch

rszrama’s picture

rszrama commented
Status: Needs review » Reviewed & tested by the community

Just confirming this opened it up for me. I may even go further to recommend making the textfield itself wider, like to a #size of 128 or something.

Anonymous’s picture

Anonymous (not verified) commented
Issue summary: View changes

Patch from #6 works for me!

  • rbayliss committed 786f9f2 on 7.x-1.x 
    Issue #1189654 by tmsimont, twistor | RoSk0: Added Max length of #edit-...
rbayliss’s picture

rbayliss commented
Status: Reviewed & tested by the community » Fixed

Sorry I missed this. Committed the change and extended the textfield to 128 as Ryan suggested.

The last submitted patch, 1: 1189654-1-max_length_patter_field.patch, failed testing.

Status: Fixed » Needs work

The last submitted patch, 6: commerce_autosku-maxlength_on_sku_pattern-1189654-6.patch, failed testing.

rbayliss’s picture

rbayliss commented
Status: Needs work » Fixed

Crazy testbot. This passes all tests and has already been committed.

wqmeng’s picture

wqmeng commented

Hello,

Why add the max length here, that is some human add blocker.

Here I have a pattern
[commerce-product:field_tax_processor]-[commerce-product:field-tax-ram:name]-[commerce-product:field-hard-drive-1:name]-[commerce-product:field-hard-drive-2:name]-[commerce-product:field-hard-drive-3:name]-[commerce-product:field-hard-drive-4:name]-[commerce-product:field-hard-drive-5:name]-[commerce-product:field-hard-drive-6:name]-[commerce-product:field_os]-[commerce-product:field_billing_period]-[commerce-product:field-term-bandwidth:name] .

You see it is very long than 255 characters, but the real sku which generated will be very short than the pattern itself.

Something will be e5-2650-8g-1tb-1tb----... , mostly of them will be less than 128 characters.

Calculate the max-length here by pattern will be no mean as the real SKU will not has the same length of it's pattern.

I have to change the length in the database to increase the pattern to be 1024, and also change the code of this module but , any time that I will upgrade this module, have to repeat the work again.

Any think?

Thanks

wqmeng’s picture

wqmeng commented
Status: Fixed » Active