Updated: Comment #28
Problem/Motivation
The 'type' field in the search index tables (search_index, search_dataset) is currently limited to 16 bytes. Search plugins that use the search index are free to pass anything they want in for this value when they call search_index(), but it needs to be unique. 16 bytes is pretty short.
Proposed resolution
Increase the size of these fields to 64 bytes. This will allow modules providing search plugins to use their module name (max 50 characters in Drupal 8) plus an additional 16 characters of their choice, which should be sufficient to cover the typical choice of the plugin machine name ID [which is probably (MODULE)_search], as well as sufficient for modules that need to have different search index types for each individual search page that is created.
Remaining tasks
Create a patch that:
a) Changes the sizes of the 'type' database fields in search.install :: search_schema() for the search_index and search_dataset tables. The patch in #16 does this (doesn't apply, needs a reroll, due to one table no longer being there).
b) We do not need a hook_update_N() at this point, since there are no updates from 7 to 8 using update.php and 8.0 is not out yet, so that can be taken out of the patch in #16.
c) We also need to document this restriction of 64 bytes in the documentation of the $type parameter of the function search_index() in search.module.
User interface changes
None directly. Modules that need to have separate indexes for each search page and want to use the search page machine name as part of $type may need to restrict the length of the search page machine name so that they can have a $type something like $module_name . "_" . $page_machine_name and not exceed 64 bytes.
API changes
None that will affect working search plugins. Plugins will have more room in $type when they call search_index() going forward.
Original report by @dealancer
Type field is limited to 16 chars in search_dataset, search_index tables and search_node_links. See more for reference:
http://drupalcode.org/project/drupal.git/blob/refs/heads/7.x:/modules/se...
http://drupalcode.org/project/drupal.git/blob/refs/heads/7.x:/modules/se...
http://drupalcode.org/project/drupal.git/blob/refs/heads/7.x:/modules/se...
That not allow modules with a big names > (e.g. crm_core_activity) to provide search.
However module name in a system table is limited to 256 chars.
Comments
Comment #1
dealancer CreditAttribution: dealancer commentedRegarding to http://drupal.org/node/159605 drupal varchar is translated to VARCHAR in most databases. This means that we can easily update this value to 255 without affecting the database size.
Comment #2
dealancer CreditAttribution: dealancer commentedOk, so the issue needs work.
Comment #3
dealancer CreditAttribution: dealancer commentedHere is a patch that increases size of type fields in 'search_dataset', 'search_index' and 'search_node_links'.
Comment #4
podarokComment #5
andypost255 could cause a lot of troubles with indexes see #1280792-37: Trigger upgrade path: Node triggers removed when upgrading to 7-dev from 6.25
{node_type}.name is varchar(255) but it's not a part of primary key
so I recommend to limit size and test this under mysql myisam engine
Comment #6
dealancer CreditAttribution: dealancer commented@andypost, thanks for notice!
Ok, cause 255 could make index troubles, let's decrease it to 64 at least and re roll patch.
Here is an updated patch.
Comment #7
podarokComment #8
andypostPlease make a patch against 8.x first to conform Backport policy
Comment #9
podarokTrue Backport Policy
Comment #10
swentel CreditAttribution: swentel commentedAlso, you can make the update hook a lot easier to read and remove superfluous code for the description like this:
Comment #11
andypost@swentel it's not good to use schema this way. hook_update_n1() should use exact definition inside of the field because next time another hook_update_n2() could change schema again and hook_schema() should hold current state. So user trying to update module got error using current values because hook_update_n1() will try to use current schema not the one it's require when it was added.
Comment #12
dealancer CreditAttribution: dealancer commentedAgreed with @andypost, schema definition can be changed in future, that cause to break of current update hook when updating drupal from the old versions.
BTW, here is a patch for 8.x. It contains changes, however without hook_update_N, which we don't need, because there is no alpha release yet.
Comment #13
swentel CreditAttribution: swentel commented@andypost Hmm, I never looked at it that way. I've been using that technique a lot of times in contrib already, but that's probably since the update hooks always change another field instead of the same field in 2 different update hooks. I don't see a direct problem though, but I should test that before I make any more statements about that :)
Comment #14
dealancer CreditAttribution: dealancer commented@swentel, you are lucky! Please read http://drupal.org/node/150220 for more information why it is better not to use schema definition in hook_update_N.
Comment #15
Dave ReidYes, we do need update hooks, even in 8.x.
Comment #16
dealancer CreditAttribution: dealancer commentedUpdated patch, not where is an update hook.
Comment #17
dealancer CreditAttribution: dealancer commentedYeah, it needs review and testing.
Comment #18
dealancer CreditAttribution: dealancer commentedHey guys! Does anyone can make review?
Comment #19
dealancer CreditAttribution: dealancer commentedThe length of this field should be synchronized with a maximum module name length from the following module [#852454] which is currently equal to 40.
Comment #20
andypostProbably module names will be trimmed to 40 chars, so related #1852454: Restrict module and theme name lengths to 50 characters
I'd suggest to postpone this issue
Comment #21
xjmSee #1709960: declare a maximum length for entity and bundle machine names. These columns reference entity type and bundle names, not module names.
Comment #22
dealancer CreditAttribution: dealancer commentedSo entity names is not limited by 16 chars in Drupal, but we feel this limit when dealing with search module. Looks like we need to have some standards around it.
Comment #22.0
dealancer CreditAttribution: dealancer commentedClarifying issue
Comment #23
jhodgdon16: search_type_field_size-1425622-16.patch queued for re-testing.
Comment #25
jhodgdonHey, sorry this hasn't been reviewed.
Generically, the "type" database field stores whatever the caller of search_index() passes in, with the assumption that that user of the search index will use the same string when performing a search query on the search index as the "type" field condition.
So callers of search_index() can really pass in whatever they want.
In 7, that value was supposed to be the machine name of the module that provided search integration, such as "node", which coincidentally was also the name of the entity it was indexing, in that case.
In 8, a search plugin is free to define what it wants to store in the type field -- it is generally the search plugin ID, but it could also be a page ID if the plugin type needs to have a different search index for each of its configured search pages.
Making the field longer is a good idea in either case. Let's see if this patch still works -- it will probably need a reroll and probably a different update number at this point. And I'm not sure 32 is the right length... in 8, it should be the same length as the machine name limit for plugins. Which is... ???
Comment #26
andypostComment #27
jhodgdonI'm not exactly sure that parent issue needs to be a parent.
See #8 - the "type" put into the database here via search_index() does not have to be the plugin ID at all.
Comment #28
jhodgdonOK. So, that parent issue doesn't actually have any mandates that are specific to this issue. We just need to pick something sensible, so let's think about this:
- A module offering a search plugin might use the plugin ID or might use the page ID, but they definitely need to have something unique.
- So, for sure we should allow something as long as a plugin ID might be, which is not actually limited by the parent issue, so that doesn't help.
- Module machine names can be up to 50 characters, so we should allow for something like really_long_module_name_search -- for this, 64 seems like a reasonable limit, I think?
- If the plugin chooses to use the search page machine name... that is probably not a great idea actually, since it is not guaranteed that it doesn't clash with other $type choices (search page machine names are only restricted to not clash with other search page machine names)... And then the parent issue only requires that these machine names come out less than 166 characters due to config naming conventions.
So. Let's choose 64 characters as the limit here. Then:
- We need to change the sizes of the 'type' database fields in search.install :: search_schema() in all tables that have this field. The patch in #16 does this (doesn't apply, needs a reroll, due to one table no longer being there).
- We do not need a hook_update_N() at this point, since there are no updates from 7 to 8 using update.php and 8.0 is not out yet, so that can be taken out of the patch in #16.
- We also need to document this restriction in the documentation of the $type parameter of the function search_index() in search.module.
- There had been a "needs tests" tag but I don't know why we need to test this actually.
Those all seem like reasonably easy tasks for a novice contributor. Issue summary is updated...
Comment #29
visabhishek CreditAttribution: visabhishek commentedI have created a patch as per #28. Please review.
Comment #30
jhodgdonThanks! That's pretty much exactly what we need.
Can you fix up the documentation part a bit though?
a) It needs to be wrapped so the documentation lines are <= 80 characters.
b) It would be a bit more readable if it said "...of this item, which should be..." instead of "of this item and should be" in the middle.
c) At the end, "character" ==> "characters". Although actually, it needs to be less than or equal to 64 *bytes*, not characters (my mistake earlier, fixed issue summary).
Comment #31
visabhishek CreditAttribution: visabhishek commentedThe patch is updated now as per #30. Please review.
Comment #32
jhodgdonThanks! That looks fine.
Comment #34
visabhishek CreditAttribution: visabhishek commented31: drupal8.type-field-should-be-longer-in-search_dataset-search-index-tables.1425622-31.patch queued for re-testing.
Comment #35
sriharsha.uppuluri CreditAttribution: sriharsha.uppuluri commentedIt was a random test failure, rather than anything wrong with the patch.
Comment #36
catchJust updating the title since the patch does not match in the slightest.
Comment #37
webchickSeems sensible to me.
Committed and pushed to 8.x. Thanks!
Moving down to 7.x.
Comment #39
cs_shadow CreditAttribution: cs_shadow commentedPatch in #31 backported to 7.x.
Comment #40
RoSk0Drupal 7 backport and hook_updade_N() implementation, table 'search_node_links'(which is removed from D8) also included because it contains same 'type' field.
Comment #41
jhodgdonThe patch in #40 looks correct to me. Since we don't have automated tests for update functions, I think we could use a manual test here. To test:
a) Install Drupal 7.x without this patch.
b) Apply the patch.
c) Run update.php and verify that the update is reported as being done.
d) Take a look at the database and verify that the 3 tables are updated correctly.
Oh, actually, the documentation on the update function should say "Increase" not "Increased".
Comment #42
RoSk0Updated documentation.
Testing probably should do someone else?
Comment #43
jhodgdonThanks! New patch looks good. Yes, someone other than people who wrote the patch should do the testing, generally.
Comment #46
visabhishek CreditAttribution: visabhishek as a volunteer and at Azri Solutions commentedupdating status to Fixed.
Comment #47
andypostNot, this issue is for d7