| Project: | Drupal core |
| Version: | 6.x-dev |
| Component: | base system |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs review |
Issue Summary
The documentation states:
Rather than using a textfield or weight field, this form depends entirely upon the order of form elements on the page to determine new weights.
Because there might be hundreds or thousands of taxonomy terms that need to be ordered, terms are weighted from 0 to the number of terms in the vocabulary, rather than the standard -10 to 10 scale. Numbers are sorted lowest to highest, but are not necessarily sequential. Numbers may be skipped when a term has children so that reordering is minimal when a child is added or removed from a term.
However, the weight field of term_data is implemented as a signed tinyint in the taxonomy_schema(), which would mean it can only range from -128 to 127, and I don't see any function in taxonomy.module to update the schema. Indeed, I tested it for a vocabulary with 150 terms or so, and ordering stops working after term weight 126 (all heavier terms are inserted as 127). I understand this could be corrected with contrib, but the core documentation is incorrect given the current functionality.
Comments
#1
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.
#2
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.
#3
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.
#4
Upon further reflection, this is probably a critical bug.
#5
suncatch asked me how big our terms table is, it's less than a million, a normal int will do.#6
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.
#7
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.
#8
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
#9
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?
#10
db_change_field() api help has:
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)
#11
This looks good to me, but let's see what the test bot says (currently in 'test request sent').
#12
How about putting in an explicit size rather than just removing 'size' => 'tiny' in all of those db columns?
#13
@Dries Test bot won't work until #693362: taxonomy_form_all() is dangerous is rolled back as well.
#14
#693362: taxonomy_form_all() is dangerous was rolled back.
#15
Looks like this is RTBC then? My comment in #12 was negated by #9 & #10.
#16
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.
#17
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.
#18
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.
#19
+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.
#20
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.
#21
Subscribing. Also encountering the weight limitation, on the terms of a taxonomy vocabulary.
#22
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.
#23
Here is a D6 backport of this patch, ready for testing. At least the taxonomy part of this patch is critical for many sites.
#24
Tested the patch in #23, seems to work for me ...
#25
Will test this soon. Ran into this today with profile.module (shudder!) fields...
#26
#23 worked well for me too. Thanks benshell et al.
#27
Thanks much, was beating my head against an Ubercart install with a large catalog (taxonomy). Updating DB schema solved it.
#28
Thanks, Ben! Prior to this patch, I kept running into issues and have had to increase the size of the int field manually.
#29
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)
#30
We RTBC here (based on #24-28)?
#31
Sorry didn't actually test. But others seem to have had success...
#32
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?
#33
#34
Subscribe
#35
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.
#36
Sorry, I accidentally changed the title of this topic! Hopefully this will change it back...
#37
#8: 612870-increase-weights.patch queued for re-testing.
#38
any progress?
#39
See #32 for the questions that still need to be addressed.