[#23189] - Update string.php to address issues in #23058
- Closed
- 29 Dec 2018
- Medium
- Build: staging
- # 23189
- Diff
- rogercreagh:patch-1
User tests: Successful: Unsuccessful:
Previous code had four problems:
- with $noSplit true if the break point is at the end of a word then that word is cut - it should be included as the whole word could be included in $length
- with $noSplit true if the break point is after the space after a word then the last word is cut making the resultant truncated string shorter than it need be as the whole word without the trailing space could be included within $length
- with $allowHtml true then if the string starts with a tag
<...and the break occurs in the middle of a tag then fragments of html are left in the truncated string - with $allowHtml true then if the string starts with a tag the ellipsis is place after the tag is closed - it should appear immediately before the tag closure.
NB There may be a debate as to whether the 3 chars of ellipsis should be included in length - my opinion is that they shouldn't, but if you wish to include them then $length should simply have 3 deducted before processing.
NB2 There may be a case for some tags actually having the ellipsis added after the tag is closed - I can't think of a case myself and I would argue that this should be covered in the complex version
NB3 The truncateComplex() function has similar problems which may need addressing.
Pull Request for Issue #23058 .
Summary of Changes
for issue 2 test to see if next char is a space and if it is include in the string by incrementing $length before determining the cut point (the extra space gets trimmed later anyway).
for issues 1 & 2 only apply ltrim() not trim() to create $tmp before processing.
for issue 3 move the code block to cut off any tag fragments from line 127 up to line 106 before we close any complete tags that are open
for issue 4 add the ellipsis after any tag fragments are stripped and before tags are closed and only add the final ellipsis at line 137 if $allowHtml is false
Testing Instructions
The proposed unit tests do not seem to fully capture these problems. You need to also test the following cases with $noSplit and $allowHtml true
test if maximum words are included with the break at char 7 and 8
<p>test ellipsis inside</p> not here with break at char 11 onwards
<p>test <a href="#">html fragment left hanging</a> clean html</p> at char 10 onwards
Expected result
test if at char 7
test if at char 8
<p>test...</p> at char 11
<p>test...</p> at char 10 to 20 and <p>test <a href="#">html...</a></p> at char 24
Actual result
test - only 4 chars, could be 7
test - only 4 chars, could be 8
<p>test</p>... - ellipsis outside the tag messes up formatting
<p>test <a</p> - html fragment left hanging
Documentation Changes Required
none
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Category | ⇒ | Libraries |
@mbabker ok. I've not done this before so I'll have to work out how to both use the unit tests and also how to add to them. Also how to see previous failures and so on. This sounds like a fairly major learning curve - can you point me at some help files or something? I understood from a previous comment that the unit tests for string.truncate() were only in draft or proposed form and not fully implemented. Is this not the case?
How would I find the assumptions which the proposed tests were based on?
Sorry for all the questions but I'm a little out of my depth here. I know I've fixed the issue I had on my sites, and it also seems there is no way of over-riding a libraries/cms/html file so that my changes don't get overwritten on core updates so I'd like to see them in the core.
Is there a way to override library files? Although even if there was it is still clearly a bug that truncate() can produce broken html, and arguably an undesired feature that it puts ellipsis in the 'wrong' place.
The proposed unit tests do not seem to fully capture these problems. You need to also test the following cases with $noSplit and $allowHtml true
You should add these cases to the unit tests as well then.
You also need to look over the unit test failures and address those since your changes introduces newer truncate logic that breaks the current set of assumptions.
The tests for this class can be found in https://github.com/joomla/joomla-cms/blob/staging/tests/unit/suites/libraries/cms/html/JHtmlStringTest.php
You can look at the build details for the Travis CI build to see the current failures (the result list is about a third of the way down the log, if you reach the list of deprecated things still in use you've gone too far).
It looks like https://docs.joomla.org/Running_Automated_Tests_for_the_Joomla_CMS is still reasonably up-to-date as it relates to getting set up locally and running the tests.
Is there a way to override library files? Although even if there was it is still clearly a bug that truncate() can produce broken html, and arguably an undesired feature that it puts ellipsis in the 'wrong' place.
For JHtml helper functions, as long as they aren't called directly (and they shouldn't be in core except for in the unit test suites, i.e. you should not find any use of JHtmlString::truncate() but it should always be JHtml::_('string.truncate')), you can register custom handlers for those functions. https://github.com/mbabker/bs3-demo/blob/master/plg_system_bootstrap3/bootstrap3.php is a basic plugin that demonstrates how this works.
In fact it was less than 30 mins work to package it up as a plugin to override the library function so that's what I've done.
Simply setting up to be able to run the unit tests is a major piece of work so I will leave this one hanging - there is a definite bug to be fixed relating to what happens if you run the existing truncate function with
<p>test <a href="#">link here</a> html</p> and $length=12 up to 22, $allowHtml=true and $noSplit=true
you get this output
<p>test <a</p>... leaving a fragment of the anchor tag which is wrong html
whereas if you run it with $length=10 or 11 you get
<p>test</p>...
In both cases the ellipsis is outside the closing </p> which I think is also wrong.
@rogercreagh Please advise status. Should this be closed?
@rogercreagh Please advise status. Should this be closed?
Sorry @Quy for the festive delay in replying. Yes it probably should be closed. The problems still exist but I am not competent to run the tests and submit the pull request so someone else needs to pick it up.
What I will do is post two (or possibly three) new issues.
a) the bug that generates incompetent html in some circumstances
b) the bug/undesired feature that causes illogical (in my opinion) placement of ellipsis in some circumstances (this might be actually broken into two related issues).
These can then sit as open issues until someone better than me picks them up and does the necessary to get them implemented or rejected.
This pull request needs to be closed as I can go no further with it.
Thanks for asking.
RogerCO
| Status | Pending | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2018-12-29 22:39:17 |
| Closed_By | ⇒ | rogercreagh | |
| Labels |
Added:
?
|
||
You should add these cases to the unit tests as well then.
You also need to look over the unit test failures and address those since your changes introduces newer truncate logic that breaks the current set of assumptions.