IPv6 tweaks

[petecooper]

Upgraders will have ip fields in txp_discuss and txp_discuss_ipban revised to VARCHAR(45).

On the grounds that new installs will process txpsql.php and then chew through the patches in textpatten/update (see http://forum.textpattern.com/viewtopic.php?pid=284631#p284631 for reference), ip in txp_log is taken care of by _to_4.5.7.php file. This update will take care of the other two places ip exists. There is (currently) no reference to ip in the _to_4.6.0.php, but if these changes belong in there instead, please advise and I'll raise a more appropriate PR.

This should resolve #442, which can be closed if this gets the thumbs up.

[Bloke]

It looks fine to me and since ip addresses are never likely to be greater than 45 chars anyway, shouldn't pose a problem when truncating data.

MySQL does have a safety feature built in which aborts field resizing if it would result in lost data - see stack overflow. Now, in 99.99999% of cases I'm sure the proposed truncate operation would work out just fine, but sod's law says that someone somewhere will have a value > 45 chars in one of those fields due to a bug or misconfiguration or plugin reusing the field. rvm_counter, for example, reuses the txp_discuss_nonce field for its own purposes, so we can't rule out some plugin reusing txp_discuss_ipban and taking advantage of the fact it's currently > 45 chars.

So, a couple of ways to proceed:

1 Use one of the stack overflow solutions to ensure that the data is a guaranteed maximum 45 chars prior to doing the alter.
2 Leave it and live with the wasted space.

From a consistency viewpoint I'm all for the first option. After all, it's a core field and if it's being abused then plugins cannot expect to rely on it not changing in future versions of Textpattern.

To prevent potential warnings on upgrade, would you be able to test the pull request in MySQL's most verbose / least permissive setup and see if it complains if you've manually inserted values > 45 chars in the field(s)? If so, choose one of the solutions to pre-truncate the fields and then alter. I'm usually wary of IGNORE. Perhaps in this case it's OK, although that does mean you'd have to construct the query by hand. So for the sake of running an update and then an alter it might be worth it.

EDIT: also, what's the performance hit on upgrade if you have hundreds of thousands of comments in the database and try to do safe_update() then safe_alter() on both tables? Wouldn't want the update script to time out...

[petecooper]

Thinking out loud, there's also an argument that IP version n might come along in a bunch of years and need more than 45 characters. And there's an argument that if you need 45 characters max, make it 45 characters and save the slack. Both valid, the first being unlikely.

My gut feeling on this is that since there's discussion going on and no definitive Textpattern needs to do xyz because reasons then I should probably just close this for now. I'll go make some soup and have a think.

[petecooper]

My soup was delicious. Closing.

[Bloke]

Glad the soup worked out.

It still feels wrong to have different widths for the same type of data, so at some point it'd be nice to bring the fields in line. Standardising on 45 seems logical as long as we can do it without the script falling over under various loads.

According to this 'ere Internet thing, the number of addressable sites under IPv6 is 340 undecillion, 282 decillion, 366 nonillion, 920 octillion, 938 septillion, 463 sextillion, 463 quintillion, 374 quadrillion, 607 trillion, 431 billion, 768 million, 211 thousand and 456, so we've probably got a little bit of breathing room before needing to up the field width from 45 chars!

[petecooper]

No great shakes, really. I figured since there was already a tweak in the 4.5.7 update patch that it'd be a trivial thing to merge, but no biggie.