Weight fields should be int, not tinyint

For reference, #471082: Taxonomy term weights range only 0-127 when dragged and http://drupal.org/node/117274#comment-1815970 are about this weighting behavior.

Wow, good catch! This is a bug in the code, in my opinion, not the documentation (although if the code is not going to be fixed for some reason, the documentation is clearly wrong compared to the documentation).

The bug being that if you have more than 125 taxonomy terms in a given vocabulary, taxonomy_overview_terms_submit() will assign weights that exceed the tinyint size of the weight field in the database. See
http://api.drupal.org/api/function/taxonomy_overview_terms_submit/7
http://api.drupal.org/api/function/taxonomy_schema/7

Actually, you could run into trouble with even fewer than 126 terms, because (if the current doc header is correct about that) the numbering might not be sequential.

It's a bug in Drupal 7, but probably should be backported to the next release of Drupal 6 as well? It seems serious to me. The fix is easy: change the taxonomy schema so that the weight field is not a "tiny" integer.

Other possible places in the Drupal 7 code that might have this problem are from fields (aside from "flag" or boolean fields) in the Drupal database that are defined to be type="int", size="tiny". Here is what I found in the Drupal 7 code base:

- block module/block table/weight field
- contact module/contact table/weight field
- filter module/filter table/weight field
- filter module/filter_format table/weight field
- poll module/poll_choice table/weight field
- profile module/profile_field table/weight field
- taxonomy module/taxonomy_term_data table/weight field
- taxonomy module/taxonomy_vocabulary table/weight field
- upload module/upload table/weight field

These mostly apply in Drupal 6 as well, with minor changes (such as table names that have changed).

I haven't yet examined the form submit code for these modules to see whether they have the same problem as taxonomy_overview_terms_submit() either, just noting that these are tables with tinyint weight fields, which seems like at least a potential problem.

Upon further reflection, this is probably a critical bug.

suncatch asked me how big our terms table is, it's less than a million, a normal int will do.

Well then, probably all of the tables listed in #3 should be changed so their weight fields are int/medium or int/normal rather than int/tiny. In both Drupal 6 and 7.

Hmmm... http://drupal.org/node/159605 says that medium int is 3 bytes in MySQL, 4 bytes in PostgreSQL.... That's enough to go to 8 million, so that should be OK (see http://dev.mysql.com/doc/refman/5.0/en/numeric-types.html). "int/normal" is 4 bytes on both platforms.

Agreed, and this should be done across the board. Since it's a schema change would be nice to get it in soonish. This is only critical for taxonomy and possibly profile since the rest are much less likely to hit the restriction, but since contrib modules will likely copy and paste schema from places it's better to be consistent here.

First stab at a patch to increase the weight columns to 'normal' for the tables listed in #3.

Uploaded so the testbot can test.

- Does 'normal' have to specified? I think this is the default if you don't specify a size (ie menu_links table)

@ #drupalsouth

I think we can just exclude 'size' => 'normal' since that's the default? And we don't need to drop and define all the indexes again?

db_change_field() api help has:

IMPORTANT NOTE: To maintain database portability, you have to explicitly recreate all indices and primary keys that are using the changed field.

I am just re-creating the indexes where the weight field was included.

Patch updated to remove the size => 'normal'. Pretty certain that is the default, on mysql 5.0 'normal' is int(11) and leaving it out is int(11)

This looks good to me, but let's see what the test bot says (currently in 'test request sent').

How about putting in an explicit size rather than just removing 'size' => 'tiny' in all of those db columns?

@Dries Test bot won't work until #693362: taxonomy_form_all() is dangerous is rolled back as well.

#693362: taxonomy_form_all() is dangerous was rolled back.

Looks like this is RTBC then? My comment in #12 was negated by #9 & #10.

I didn't notice any menu module hunks in here, but checked and those weight fields are already set to ints. I have a feeling this fix will solve a couple of other issues in the queue now.

Committed to HEAD. Marking for backport, depending on how Gábor feels about this.

I wish to encourage Gabor to backport this fix.

We are building out a site in 6.15 and encountered this issue with blocks yesterday. I found http://drupal.org/node/565220#comment-2161254 yesterday and found this issue today while checking for duplicates before posting a new issue.

I also encountered this today on a D6 site with a large number of blocks. They could be re-ordered with tabledrag, but saving the form had no effect.

+1 for the backport

I just run into this problem with taxonomy on a large vocabulary. If you have a large vocabulary of more than 127 items you are forced by this limitation to sort alphabetically or at least from the 127th item onwards. In my case Countries grouped by continent. I was unable to have a No Region item last, until I manually changed the column.

Gabor doesn't normally backport fixes. Someone in the community writes the patch, someone else in the community reviews and tests the patch sufficiently to mark it "reviewed and tested by the community", and then Gabor commits it if he agrees it is a good patch. That is the process.

So, if you are interested in getting this fix into Drupal 6, the next step is for someone to scan through the hook_schema() functions in Drupal 6 core, find everywhere that weight fields are tinyint instead of int, and make a patch to change them. In Drupal 6, this will also require update functions, besides changing the schema definition, similar to what was done in Drupal 7 patch in #10 above.

Anyway, it isn't the exact patch from #10, but it should be somewhat similar.

Subscribing. Also encountering the weight limitation, on the terms of a taxonomy vocabulary.

I'm having the same problem in drupal 6.x specifically with blocks but I see that the patch in #10 covers a lot of other tables which would be useful if it was rolled back.

Here is a D6 backport of this patch, ready for testing. At least the taxonomy part of this patch is critical for many sites.

Tested the patch in #23, seems to work for me ...

Will test this soon. Ran into this today with profile.module (shudder!) fields...

#23 worked well for me too. Thanks benshell et al.

Thanks much, was beating my head against an Ubercart install with a large catalog (taxonomy). Updating DB schema solved it.

Thanks, Ben! Prior to this patch, I kept running into issues and have had to increase the size of the int field manually.

Marked this as duplicate: #329740: Blocks cannot be rearranged if there are more than 128 in a given region (they get automatically arranged in alphabetical order)

We RTBC here (based on #24-28)?

Sorry didn't actually test. But others seem to have had success...

Uhm, why do we need to have separate code blocks here for MySQL and PostgreSQL? Also, how should we coordinate fixes for this in the D7 update path? I assume the update will not be a problem except unneeded code runs. Anybody verified that?

Subscribe

If there's any noobies out there, be sure to run Update.php after applying the patch. I about beat myself over the head until I remembered that I had not done that yet. Once I did, it worked like a charm!!! Thanks to benshell for backporting that patch for D6.

Sorry, I accidentally changed the title of this topic! Hopefully this will change it back...

#8: 612870-increase-weights.patch queued for re-testing.

any progress?

See #32 for the questions that still need to be addressed.

Subscribed!

@forbesgraham: You do not need to post "subscribe" comments any more. There is a green "Follow" button in the upper right corner of issues.

here is a thought for the first part of your question: I am not all that familiar with postgresql, but it seems that it handles alter column differently than mysql, so drupal 6 has an api function to deal with that kind of stuff. of course drupal 7 implements the database layer pretty differently than 6 and doesn't have that api call anymore.

can this change for drupal 6 be moved upstream?

See #16. This is already fixed in D7.

Patch updated to apply against latest D6 git code.

In reply to the concerns mentioned in #32:

Separate lines of code for postgress versus mysql: I guess these are not needed. Drupal 6 does have change_db_field() as well, so the D6 patch could be very similar to the D7 patch. However, the patch as posted also works as already confirmed before by others and now once more by myself (site with more than 255 blocks). So the discussion is not about whether it works or not, but about more subjective terms like cleanliness, maintainability, understandability. Regarding these, I would like to mention that it doesn't really matter in this case.
- This code won't be maintained. Errors in hook_update() can often only be solved by writing yet another hook_update,not by changing the code itself.
- This code was readable and understandable for me.

Moreover, considering D6 is already far in the maintenance phase of its life cylce, chances that this code needs to be refactored are minimal. And we all know that if I would put this issue back by rewriting the patch, it will not reach RTBC for the next year.

So, the choice is: accept this possibly slightly inferior but tested, reviewed and working patch, or accept that this issue will "never" reach RTBC and that from time to time Drupal users will loose many hours of debugging and searching before they stumble upon this issue.

D7 update: I tested these steps:
- clean 6.26 install
- applying the patch from #45
- run update.php
- install 7.14
- run update.php from 7.14
No problems at all. Database is as it should be.

Furthermore: I reviewed the hook_update()s that will be run between the updates in this patch and the "corresponding" hook_update()s in the D7 .install files: none of these resets, touches or assumes anything about the size of the weight field. So no problems here either.

this patch (#45) works.

But to have to redo the patch with every update is just asking for trouble. It is also getting annoying having to redo the taxonomy for the catalog (getting big for an ecommerce site)

Any chance this could be made permanent or as a module?

Thank you! Patch from #45 works!

Any hope? 3 years old bug...

Removing myself from the author field to unfollow the issue. --xjm

Hi, any hope of getting this commited to D6?