[ticket/10640] Change maximum subject length #571
Conversation
|
Diff looks fine |
|
To be honest the size of the box looks fine to me. Maybe go up to 50/60 but any more would look weird. I'll apply the changes to the Quick Reply tonight. |
|
Good point, I had forgotten that quick reply even had a title field (I don't use QR much, tbh). So yeah, that change needs to be made as well and then this should be mostly ready for merging. |
|
Fixed, commited and squashed. |
|
Tests fine with long titles (the concern I mentioned @ A51). Looks good. |
| @@ -1107,7 +1107,7 @@ function extension_allowed($forum_id, $extension, &$extensions) | |||
| * NOTE: This parameter can cause undesired behavior (returning strings longer than $max_store_length) and is deprecated. | |||
| * @param string $append String to be appended | |||
| */ | |||
| function truncate_string($string, $max_length = 60, $max_store_length = 255, $allow_reply = false, $append = '') | |||
There was a problem hiding this comment.
This should not be done!
It breaks all other uses of truncate_string() where the value is not specified.
Instead you should look up the places and set max_length to 120 for the subject....
There was a problem hiding this comment.
Fair point. We should instead be modifying the function call to change the max length argument input instead of the default parameter here. If the function is receiving no input for that paramter, we should add it and remove this change to the signature line.
There was a problem hiding this comment.
@nickvergessen I don't believe anything could actually break from changing it here. truncate_string's only purpose is to ensure that a string's htmlspecialchars() representation does not exceed the DB's storage space, and that function is not affected by this change. Therefore the only harm possible is a broken layout if the text wraps.
There was a problem hiding this comment.
jellydoughnut, when they change the allowed length it will break/change all mods that use this function with default value. Also the change that was made here, is just a lazy work around. as I said, the function calls should be changed and not the default value.
There was a problem hiding this comment.
@nickvergessen Change, yes. Break, no. Lazy, probably. I don't have a strong opinion on this, but if it were my decision I would merge as it is.
There was a problem hiding this comment.
Let's audit the call sites, surely there cannot be that many of them.
There was a problem hiding this comment.
I don't see how this will harm anything.
|
Should be ready for merging now thanks to @nickvergessen |
|
You should change the template |
|
Rebase to remove the merge commit. |
PHPBB3-10640
The default value should be kept, so we do not change the behaviour for MODs and Extensions that use the function with its default values. PHPBB3-10640
PHPBB3-10640
|
Done |
* unknownbliss/ticket/10640: [ticket/10640] Change subject length in mcp in subsilver [ticket/10640] Change subject length in MCP [ticket/10640] Do not change default value of truncate_string() [ticket/10640] Change maximum subject length
http://tracker.phpbb.com/browse/PHPBB3-10640
http://area51.phpbb.com/phpBB/viewtopic.php?f=108&t=42547
PHPBB3-10640