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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dealancer’s picture

Regarding 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.

dealancer’s picture

Status: Active » Needs work

Ok, so the issue needs work.

dealancer’s picture

Title: Type field is limited to 16 chars in search_datase and search_index tables » Type field is limited to 16 chars in search_datase, search_index and search_node_links tables
Status: Needs work » Needs review
FileSize
1.91 KB

Here is a patch that increases size of type fields in 'search_dataset', 'search_index' and 'search_node_links'.

podarok’s picture

Title: Type field is limited to 16 chars in search_datase, search_index and search_node_links tables » Type field is limited to 16 chars in search_database, search_index and search_node_links tables
andypost’s picture

Status: Needs review » Needs work

255 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

dealancer’s picture

@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.

podarok’s picture

Status: Needs work » Needs review
andypost’s picture

Version: 7.x-dev » 8.x-dev
Status: Needs review » Needs work
Issue tags: +Needs tests, +Needs backport to D7

Please make a patch against 8.x first to conform Backport policy

podarok’s picture

swentel’s picture

Also, you can make the update hook a lot easier to read and remove superfluous code for the description like this:

  $schema = search_schema();
  db_change_field('search_dataset', 'type', 'type', $schema['search_dataset']['fields']['type']);
  db_change_field('search_index', 'type', 'type', $schema['search_index']['fields']['type']);
  db_change_field('search_node_links', 'type', 'type', $schema['search_node_links']['fields']['type']); 
andypost’s picture

@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.

dealancer’s picture

Status: Needs work » Needs review
FileSize
1.05 KB

Agreed 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.

swentel’s picture

@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 :)

dealancer’s picture

@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.

Dave Reid’s picture

Yes, we do need update hooks, even in 8.x.

dealancer’s picture

Status: Needs review » Needs work
FileSize
1.88 KB

Updated patch, not where is an update hook.

dealancer’s picture

Status: Needs work » Needs review

Yeah, it needs review and testing.

dealancer’s picture

Hey guys! Does anyone can make review?

dealancer’s picture

Status: Needs review » Needs work

The length of this field should be synchronized with a maximum module name length from the following module [#852454] which is currently equal to 40.

andypost’s picture

Status: Needs work » Needs review

Probably 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

xjm’s picture

See #1709960: declare a maximum length for entity and bundle machine names. These columns reference entity type and bundle names, not module names.

dealancer’s picture

So 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.

dealancer’s picture

Issue summary: View changes

Clarifying issue

jhodgdon’s picture

Status: Needs review » Needs work

The last submitted patch, 16: search_type_field_size-1425622-16.patch, failed testing.

jhodgdon’s picture

Hey, 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... ???

andypost’s picture

jhodgdon’s picture

I'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.

jhodgdon’s picture

Title: Type field is limited to 16 chars in search_database, search_index and search_node_links tables » 'type' field should be longer in search_dataset, search_index tables
Issue summary: View changes
Issue tags: -Needs tests +Novice

OK. 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...

visabhishek’s picture

I have created a patch as per #28. Please review.

jhodgdon’s picture

Issue summary: View changes
Status: Needs review » Needs work

Thanks! That's pretty much exactly what we need.

Can you fix up the documentation part a bit though?

+ *   The plugin ID or other machine-readable type of this item and should be less than 64 character.

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).

visabhishek’s picture

The patch is updated now as per #30. Please review.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Thanks! That looks fine.

Status: Reviewed & tested by the community » Needs work
visabhishek’s picture

sriharsha.uppuluri’s picture

Status: Needs review » Reviewed & tested by the community

It was a random test failure, rather than anything wrong with the patch.

catch’s picture

Title: 'type' field should be longer in search_dataset, search_index tables » Increase length of 'type' field in search_dataset, search_index tables from 16 to 64

Just updating the title since the patch does not match in the slightest.

webchick’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Seems sensible to me.

Committed and pushed to 8.x. Thanks!

Moving down to 7.x.

  • Commit 78fb347 on 8.x by webchick:
    Issue #1425622 by visabhishek, dealancer: Increase length of 'type'...
cs_shadow’s picture

Status: Patch (to be ported) » Needs review
FileSize
732 bytes

Patch in #31 backported to 7.x.

RoSk0’s picture

Drupal 7 backport and hook_updade_N() implementation, table 'search_node_links'(which is removed from D8) also included because it contains same 'type' field.

jhodgdon’s picture

Status: Needs review » Needs work
Issue tags: +Needs manual testing

The 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".

RoSk0’s picture

jhodgdon’s picture

Status: Needs work » Needs review

Thanks! New patch looks good. Yes, someone other than people who wrote the patch should do the testing, generally.

  • webchick committed 78fb347 on 8.3.x
    Issue #1425622 by visabhishek, dealancer: Increase length of 'type'...

  • webchick committed 78fb347 on 8.3.x
    Issue #1425622 by visabhishek, dealancer: Increase length of 'type'...
visabhishek’s picture

Status: Needs review » Fixed

updating status to Fixed.

andypost’s picture

Status: Fixed » Needs review

Not, this issue is for d7

  • webchick committed 78fb347 on 8.4.x
    Issue #1425622 by visabhishek, dealancer: Increase length of 'type'...

  • webchick committed 78fb347 on 8.4.x
    Issue #1425622 by visabhishek, dealancer: Increase length of 'type'...