Bug #59889
closed
stdWrap.addParams breaks on comments
80%
Description
In typo3\sysext\frontend\Classes\ContentObject\ContentObjectRenderer.php:addParams (line 4191 in Typo3 6.2.3, but the bug has been around since 4.5 at least), exploding the Tag works by exploding by "<". This breaks, if the first opening bracket belongs to a HTML comment.
Example:
HTML content of a content element:
<!-- begin FCE accordeon -->
<div class="accordeon">
[...]
</div>
applied stdWrap:
tt_content.stdWrap.addParams.id = cid
tt_content.stdWrap.addParams.id.dataWrap = |{field:uid}
expected output:
<!-- begin FCE accordeon -->
<div id="cid645" class="accordeon">
[...]
</div>
actual output:
<!-- id="cid645" begin FCE accordeon -->
<div class="accordeon">
[...]
</div>
Suggested Fix: Instead of just exploding randomly, match using
preg_match('/\<[A-Za-z].*?\>/', $content)
Files
Updated by Markus Klein over 10 years ago
- Status changed from New to Needs Feedback
Well, why not simply using the _offset setting for addParams?
http://docs.typo3.org/typo3cms/TyposcriptReference/Functions/Addparams/Index.html#addparams
Updated by Lukas Domnick over 10 years ago
I am not sure how this is supposed to be a bugfix for the described use case.
tt_content starts how ever it will... e.g. CSS Styled Content will start with a tag, whereas some extensions start their output with a comment, or even two (whyever - this is not predictable for a community-driven extension repository!) Thus, addParams would produce unpredictable, unreliable results.
So no, using _offset is unfortunately not a reasonable workaround.
Updated by Markus Klein over 10 years ago
What is your real use case?
You're adding params to some unknown string??
Updated by Lukas Domnick over 10 years ago
Somehow, yes.
In fact, I am intending to add an ID to the first tag on a set of page content elements, or - depending on the setting - on the first Tag of each content element.
A page content element may be either a plug-in's output, or a common RTE/Heading/... content element.
If no HTML tag is present within a content element, then no ID is needed that element.
If the first HTML tag is preceeded by a comment, then that comment should be treated as such, and not be torn apart.
Updated by Markus Klein over 10 years ago
Then you should maybe have a closer look at the TypoScript of css_styled_content. There you can add everything you like to a content element or whatever. Parsing the output again for adding an id is IMO the wrong approach.
Updated by Lukas Domnick over 10 years ago
While I have found a solution to my specific use case, this has nothing to do with the bug report.
Whether or not it is a sensible approach to USE the function provided by the core, it still should work allright. Using explode('<',$str) is IMHO not the best method of parsing HTML.
Updated by Markus Klein over 10 years ago
But it is a quick way of parsing HTML. And the question is, the right tool for the right job?
Are comments an edge case or not?
Updated by Lukas Domnick over 10 years ago
I don't think that HTML comments could be considered an edge case when talking about HTML output. E.g. I have found the bug a couple of weeks ago, because the widely used "extension builder"-Extension starts the output with a comment.
How many times faster are the two explodes when compared to one preg_match, and what speed difference is reasonable to justify the errateous behaviour?
Updated by Markus Klein over 10 years ago
- Category set to TypoScript
- Status changed from Needs Feedback to New
- Assignee set to Lukas Domnick
Alright, please push a patch to the review system. We'll see what others think. I'll not be blocking this.
Updated by Lukas Domnick over 10 years ago
- File isotest.php isotest.php added
- % Done changed from 0 to 80
implementation done and tested via an isolated test, see attached file.
A benchmark shows, this implementation is actually faster. At 10.000 iterations of the test case on my laptop pc, 16 Seconds vs. 22 Seconds.
Updated by Gerrit Code Review over 10 years ago
- Status changed from New to Under Review
Patch set 2 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/31570
Updated by Gerrit Code Review about 10 years ago
Patch set 3 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at http://review.typo3.org/31570
Updated by Gerrit Code Review almost 10 years ago
Patch set 4 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at http://review.typo3.org/31570
Updated by Gerrit Code Review almost 10 years ago
Patch set 5 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at http://review.typo3.org/31570
Updated by Christian Kuhn over 8 years ago
- Status changed from Under Review to Rejected
i'm sorry for the time that was put into the issue. but even after two years with a longish list of comments we're still no convinced the patch solves a major problem that can't be circumvented via _offset and other cases are rather a misuse of addParams. This patch seems to have problems of finding a general consensus, was abandoned and this issue is closed as rejected.
if you still think the issue is important to tackle and we should re-discuss the issue, please go ahead and open a new issue.