field lengths differ across many tables

[steros]

Description

I found numerous field lengths to be different across many tables. Thus strings get cut off.
In my case the "telephone" was cut off as someone set it to be just 20 characters in the quote_address table despite it being 255 characters wide in the sales_order_address table.
There are many other fields differing.

Someone seriously needs to check this! Unfortunately I do not have enough knowledge about all the fields.

In my case our customers want to enter not one phone number but sometimes their landline and mobile phone number into the telephone field. The field is not validated against any format.

Fixed Issues (if relevant)

1. field lengths differ across many tables #10869: field lengths differ across many tables

Manual testing scenarios

1. do not validate the telephone number against any format
2. place a new order
3. enter a telephone number wider than 20 characters

[orlangur]

Well, telephone field is supposed to store phone number, you know. If you need to store something else, another field shall be created.

despite it being 255 characters wide in the sales_order_address table

This seems really wrong but I'm not sure we can change this to some reasonable value, like 20.

do not validate the telephone number against any format

Not a valid scenario.

Someone seriously needs to check this!

Please describe concrete issues with concrete observations which does not seem to be valid.

[steros]

@orlangur well sure a phone is max 20 chars at least according to https://en.wikipedia.org/wiki/Telephone_numbering_plan
But I don't see any validator ensuring that, is there any? (I only found US and UK format validators and one checking it to be required)
This together with the sales_order_address being 255 chars wide makes it seem that it should be valid to enter more chars than 20.

Also there are other fields that differ like shipping_method, firstname, lastname, prefix, suffix and so on. Some of those are even updated in the UpgradeSchema script.
And 20 chars for surname seems...I mean two long double names and that's it, don't you think?
There is a german politician called: https://en.wikipedia.org/wiki/Sabine_Leutheusser-Schnarrenberger -> 27 characters.

[orlangur]

@steros trying to understand the reason of 20 characters length I came across cb3914b#diff-78dad85ac24baac9f8f56748e920549e commit.

Looks like column size was mixed up with column position, isn't it?

[steros]

Thanks for finding that commit!

That could be an option. Those numbers seem very arbitrary. Why would "country_id" be changed to 40 chars? As far as I can see "country_id" seems to be something along ISO 639 or similar. So that could be 2 to 4 characters wide possibly.

@mlogvin Can you comment on that?

[orlangur]

Note the commit message MAGETWO-36214: Reorder install logic. Numbers like 10, 20, 30 are usually used to define field position (such tradition comes from Basic programming language I believe where lines were numbered).

[orlangur]

Ok, I managed to reach Michael on this, suspicious commit was performed in scope of NDB compatibility efforts. It is not clear whether column sizes were picked up to make it work or mixed up with column positions.

Also, I noticed https://github.com/magento/magento2/pull/10011/files which is about firstname/middlename/lastname truncation.

@steros please go all over cb3914b and fix all numbers not fixed yet which seem unreasonable (for example, I don't think country_id should be expanded).

Note that changes should be made similar to PR I mentioned, please rewrite this branch with force push having single commit so that only app/code/Magento/Quote/Setup/UpgradeSchema.php file is touched.

[steros]

I'm not sure if I have enough knowledge to go over all fields and decide the correct field length.
But if I can find the time I'll look at it.

[orlangur]

@steros just check previous values in cb3914b commit and decide whether they are valid (probably country_id is the only one which should not be reverted). Also, check the current resulting schema, first/middle/lastname were already expandeed, maybe some others too.

[orlangur]

@steros feel free to just fix the telephone field which you initially reported if you does not feel like to perform such analysis.

[orlangur]

@steros do you need any assistance with rewriting PR as an upgrade script?

[steros]

@orlangur I would only need an advice what version to target. Cause as I see it the fix actually targets all versions but you need to set a specific one don't you?

Other than that I'm fine but I can't work on it before November. So if anyone wants to pick it up to fix it earlier, I'm fine with it.

[orlangur]

@steros thanks for quick response!

Let's close this PR for now then. I reopened initial bug report with instruction how the fix should be done and added to Community Dashboard so that it can be picked up by anyone.

develop branch will be removed soon, the 2.2-develop should be used as main target branch now. If no one fixes it before you, please ping me back and I'll reopen PR for you.

[steros]

If no one fixes it until November I'll pick it up, thanks 👍