CSS Class field is limited to 50 chars

I'd be a bit more direct to change this to a bug. The strings are cut off because the database field only has 50 characters.
There is no check on the form however to warn the user that his string is too long. Or not even een MAXLENGTH on the form.

I'd say:Maxlength on the form (255) and change the databasefield to 255 chars.

We should increase all three columns to 255 and set the classes input field to maxlength.

* Change block_class_schema()
* Add block_class_update_6001() to change existing schema
* Change block_class_form_alter() to increase input field to maxlength

This is great, please commit ASAP!

+1

I've committed this patch to 6.x-1.x-dev. Please test. :)

This will also need to be ported to the 7.x-1.x branch to allow easy Drupal 6.x->7.x upgrading. I've created an issue here:

#1020790: Port schema update to 7.x-1.x branch

Actually this change causes an error because a well known MySQL bug:

user warning: Specified key was too long; max key length is 1000 bytes query: CREATE TABLE block_class ( `module` VARCHAR(255) NOT NULL, `delta` VARCHAR(255) NOT NULL, `css_class` VARCHAR(255) NOT NULL, PRIMARY KEY (module, delta) ) /*!40100 DEFAULT CHARACTER SET utf8 */ in D:\web\2011\euintel\www\includes\database.inc on line 551.

The problem is that 3 bytes are reserved for every character in UTF-8 tables and the compound key blows the maximum size of 1000 bytes set for keys.

Although it's possible to use class names with 255 chars but IMO it's unlikely and pointless. Furthermore we should adhere to the core Block module's database scheme and set the length 64 for the module field and 32 for delta.

The patch misses the maxlength on the fields. But I agree with your point that 64 should be enough, as long as the interface reflects this.

Allowing 255 chars for the class name is not pointless. It's maybe a little excessive, but the 50 char limit is much too small, and I found myself reaching it multiple times which is very frustrating. Of course no one is going to create one class name that long, but you run into problems when wanting to add 6 or 7 or even 8 class names to one block.

sgabe, as you obviously have more MySQL expertise than myself, can you explain how changing the char length of the css_class column fixes your bug? From what I can tell, the key is built using the module and delta columns only.

@dboulet: what fixes the bug is changing (reducing) the length of the module and delta fields. About the class length, that was just a comment. Let's continue in #1020846: 7.x-1.x fails to install database schema ("Specified key was too long"), however I don't understand why did you change the length for these fields when this issue was supposed to be about only the class field.

Thanks for the explanation sgabe. To answer your question, I had only included those changes in the patch because of the comments in #2 from the project maintainer. Turns out it was a bad idea. :s

Thanks for bringing it to our attention.

I'm going to mark this as fixed—any bugs related to this issue can be dealt with in other issues.

This is not fixed at all. I'll roll a patch soon.

The patch at #3 had 2 problems (I mention that because it has been committed and is in the dev branch right now):

First, there's a typo in the db_change_field() for the css_class field, it sets "size" instead of "length". As "size" is not a valid keyword, it will cause update error like this:

Failed: ALTER TABLE {block_class} CHANGE `css_class` `css_class` NOT NULL

Second, if the module and delta were changed to 255 it will break primary key as #8 mentioned.

To me, I do need the css_class char limit increase to 255, not because I'm going to use a crazy long class name, but because I want to give multiple class to the block, which can be done by having them space separated. And when you have multiple classes, it's very easy to exceed 50 chars.

So, my patch, which is against current dev version, will update the css_class correctly to 255 and keep module at 64, delta at 32.

I've tested it on my project and it actually works. Please review and commit it if possible.

The patch in #16 changes the update function, but not the schema information. Perhaps the code from this patch should instead be added to this issue: #1020846: 7.x-1.x fails to install database schema ("Specified key was too long").

right, let's mark this a dup of #1020846: 7.x-1.x fails to install database schema ("Specified key was too long") and I'll roll a new patch there.

if you don't mind, I'm marking this as "fixed" once again—technically the original issue has been fixed.

Dboulet, this is not "fixed". That would happen if the patch in #16 were applied to 6.x-dev. As it is now, the dev branch is still improperly changing the size of the key in addition to changing the size of the css_class field.

Proper install patch committed.

Thank you berenddeboer! It is so good to finaly see this in the repository.