Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ticket/10640] Change maximum subject length #571

Merged
merged 4 commits into from Jun 19, 2012

Conversation

Projects
None yet
8 participants
@imkingdavid

This comment has been minimized.

Copy link
Contributor

commented Feb 11, 2012

Diff looks fine

@michaelcullum

This comment has been minimized.

Copy link
Member Author

commented Feb 11, 2012

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.

@imkingdavid

This comment has been minimized.

Copy link
Contributor

commented Feb 11, 2012

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.

@michaelcullum

This comment has been minimized.

Copy link
Member Author

commented Feb 11, 2012

Fixed, commited and squashed.

@jellydoughnut

This comment has been minimized.

Copy link
Contributor

commented Feb 13, 2012

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 = '')

This comment has been minimized.

Copy link
@nickvergessen

nickvergessen Feb 21, 2012

Contributor

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....

This comment has been minimized.

Copy link
@imkingdavid

imkingdavid Feb 21, 2012

Contributor

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.

This comment has been minimized.

Copy link
@jellydoughnut

jellydoughnut Feb 22, 2012

Contributor

@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.

This comment has been minimized.

Copy link
@nickvergessen

nickvergessen Feb 22, 2012

Contributor

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.

This comment has been minimized.

Copy link
@jellydoughnut

jellydoughnut Feb 24, 2012

Contributor

@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.

This comment has been minimized.

Copy link
@p

p Mar 7, 2012

Contributor

Let's audit the call sites, surely there cannot be that many of them.

This comment has been minimized.

Copy link
@callumacrae

callumacrae Apr 28, 2012

Contributor

I don't see how this will harm anything.

@michaelcullum

This comment has been minimized.

Copy link
Member Author

commented May 2, 2012

Should be ready for merging now thanks to @nickvergessen

@vinny

This comment has been minimized.

Copy link
Member

commented Jun 2, 2012

You should change the template mcp_topic.html to include the new value(124) for splitting topic.

@imkingdavid

This comment has been minimized.

Copy link
Contributor

commented Jun 2, 2012

Rebase to remove the merge commit.

michaelcullum and others added some commits Feb 11, 2012

[ticket/10640] Do not change default value of truncate_string()
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
@michaelcullum

This comment has been minimized.

Copy link
Member Author

commented Jun 17, 2012

Done

@travisbot

This comment has been minimized.

Copy link

commented Jun 17, 2012

This pull request passes (merged 7a6a573 into e045a80).

@travisbot

This comment has been minimized.

Copy link

commented Jun 17, 2012

This pull request passes (merged dd6eeef0 into e045a80).

@travisbot

This comment has been minimized.

Copy link

commented Jun 17, 2012

This pull request passes (merged a41f86f into e045a80).

p added a commit to p/phpbb3 that referenced this pull request Jun 19, 2012

Merge PR phpbb#571 branch 'unknownbliss/ticket/10640' into develop
* 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

@p p merged commit a41f86f into phpbb:develop Jun 19, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.