i18n adds index 'textgroup_location' which leads to an incompability with upcoming change #221020-44: locales_source.location is just varchar(255) and it is not truncated

A simple fix is to change index or is this index really needed? #221020-46: locales_source.location is just varchar(255) and it is not truncated

CommentFileSizeAuthor
#1 803380-i18nstrings-location-d6.patch2.12 KBandypost
i18n_locale.patch892 bytesandypost

Comments

andypost’s picture

andypost
he/him
Primary language Russian
commented
Category: support » bug
Status: Active » Needs review
StatusFileSize
new 803380-i18nstrings-location-d6.patch2.12 KB

Fixed all occurrences of location field. Also this issue blocks a really annoying bug in core.

Also there's a issue that needs more opinions - #532512: Plural string storage is broken, editing UI is missing

hass’s picture

hass commented

We are changing to text in #221020: locales_source.location is just varchar(255) and it is not truncated and do not limit to 255 chars.

andypost’s picture

andypost
he/him
Primary language Russian
commented

It should bring some performance improvements as I found in old issues at #532512-5: Plural string storage is broken, editing UI is missing

andypost’s picture

andypost
he/him
Primary language Russian
commented
Issue tags: +D7 upgrade path

Taggin because this index breaks upgrade path for d7

andypost’s picture

andypost
he/him
Primary language Russian
commented

@hass mysql cant make index on text|blob field so we need location(some value of chars)

jose reyero’s picture

jose reyero commented
Priority: Normal » Critical
Status: Needs review » Needs work

This looks good, though I'm not sure about how will it work with the actual upgrade path (will the index be just kept when updating from varchar to text in D7?) and also with other DBs... ?

About this patch, looks it will work for new installs of the module, but I'm missing the actual update function (?)

andypost’s picture

andypost
he/him
Primary language Russian
commented

Sure, I forget about hook_update_N()

But the main question - what for this index?

I think because {location} is TEXT so we could proceed with array('location', 30) as it already done for [source] field.

Any ideas?

int’s picture

int commented

#864020: Locale upgrade to D7 fails due to index added by i18nstrings module, we need this resolved.

Or we drop the hijacks index.
Or fix the index to D6 can be upgrade to D7..

I think it's better to remove the index, because we should not hijacks the core-schema.. Or we will have again problems with future versions..

int’s picture

int commented

I see now that we can remove the module, all the index will be droped, nice. I will drop using this module for now in D7

function i18nstrings_uninstall() {
 	$ret = array();
	
 	// Create database tables.
 	drupal_uninstall_schema('i18nstrings');
 	// @TODO locale table cleanup, think about it.
	// Should we drop all strings for groups other than 'default' ?
 	
	// Drop custom field.
 	db_drop_field($ret, 'locales_target', 'status');
 	// Drop custom index.
 	db_drop_index($ret, 'locales_source', 'textgroup_location');
 	// May be a left over variable
 	variable_del('i18nstrings_update_skip');
}
andypost’s picture

andypost
he/him
Primary language Russian
commented

@Jose Reyero, Could you explain a purpose of this index?

damien tournoud’s picture

damien tournoud commented

The correct fix here is for i18n to *STOP HIJACKING A CORE TABLE*. You can add a column to that table if you want, but repurposing the location column is just a dirty hack.

Stevel’s picture

Stevel commented

I agree that making changes to the database tables of another project is prone to errors popping up.

Also, foo_update_6xxx-functions should never be changed, as chances are they have already run. There should be a foo_update_7xxx function here.

andypost’s picture

andypost
he/him
Primary language Russian
commented

As I see most of queries use i18nstrings_context_query() which always adds textgroup and location fields to queries and probable source

I cant understand a reason to store all strings in 1 table. rebuild of all this indexes a very expensive operation so +1 to drop this index and measure performance

andypost’s picture

andypost
he/him
Primary language Russian
commented

Suppose much better to change locale_update_7000() and schema index source_context to source_context_textgroup in core. But anyway drop this in i18n

steinmb’s picture

steinmb commented

Hi
Any progress on this issue? I tested a D6 -> D7 upgrade on one small existing site and tripped over this bug. Workaround for me was to manually do:
ALTER TABLE `locales_source` DROP INDEX textgroup_location
though this worked it prob. got some usability issues :)

andypost’s picture

andypost
he/him
Primary language Russian
commented

I'm waiting review of module maintainer because this index could have big impact on performance

pfrenssen’s picture

pfrenssen
Location Sofia
commented

subscribe

jose reyero’s picture

jose reyero commented

Yes, this index is critical for performance.

For D7, maybe we can use the 'context' field instead of this one, though that will need specific import/export code for po files...

I think this patch here (#18) looks good and will fix the issue, #864020: Locale upgrade to D7 fails due to index added by i18nstrings module

crea’s picture

crea commented

Subscribing

joseph.olstad’s picture

joseph.olstad
Primary language French
commented
Issue summary: View changes
Status: Needs work » Closed (outdated)

fixed in 7.x