mybb / merge-system

The MyBB Merge System allows for easy merging of an existing forum (be it MyBB or another forum software) into a MyBB 1.8.x forum.
Other
32 stars 33 forks source link

Avoid trimming data when we can't guess the column length #260

Closed enduvar closed 3 years ago

enduvar commented 3 years ago

The merge system can't guess the column length properly when importing to a postgres db, as a result the last three characters were being replaced by ...:

my_substr($insert_array[$key], 0, $column_length[$key]-3)."..."; // $column_length[$key] is 0

We had this problem while importing a ipb4 forum (mysql) to mybb (postgres). This commit should make it more obvious and prevent trimming data when the column length is not known.

yuliu commented 3 years ago

Looks like the get_length_info() function only works well with MySQL, where column lengths will be retrieved in the Type column through $db->show_fields_from() but the PostgreSQL version puts relevant information in the Extra column. https://github.com/mybb/merge-system/blob/d453ec2b922a088cfa41a81753c37b7f038ecb77/resources/functions.php#L1033-L1048.

So IMO the problem resides in the get_length_info() function. Or should we make the DB_Base::show_fields_from() function for different DB engines return results consistently?

Anyway I'm not sure about how PostgreSQL works here, @kawaii @euantorano could you please shed some light on it?

enduvar commented 3 years ago

For VARCHAR(length) postgres returns character varying in the Extra column. The column length is saved in the character_maximum_length column which is ignored by the function.

The problem is indeed with get_length_info show_fields_from but that's part of mybb, this commit is to mitigate possible issues in the merge system since this is currently trimming every column, even those which are within the maximum length. It doesn't fix the underlying issue. That should be fixed in mybb IMO.

EDIT: I meant show_fields_from

yuliu commented 3 years ago

I do think so but get_length_info() is a merge system function 😄

enduvar commented 3 years ago

Apologies, I meant show_fields_from, that's the one ignoring the character_maximum_length column. This query specifically https://github.com/mybb/mybb/blob/5fe63c81a0a6373d8f7bfd5c78311ee9977cf72d/inc/db_pgsql.php#L1139

yuliu commented 3 years ago

It's OK and yes, we'll probably have both the merge system and MyBB fixed.

euantorano commented 3 years ago

My preferred fix would be to fix the underlying code that messes up getting the length rather than applying a bandaid on top of it.

If the same problem is in the core as well as the merge, please create an issue there too and we'll get it fixed in the next release. Database library issues normally take a fairly high priority.