CMS sidebar tree view - nested pages broken

[Zauberfisch]

With the release of 3.5.2 the page tree in the sidebar is broken for nested items, if SSViewer.source_file_comments is true. The problem is that each list item has 2 </li> closing tags.

3.5.1:

3.5.2:

3.5.2 after re-expanding "Home":

This happens because in Hierarchy.php#L148 we check for a closing </li> at the end, but with SSViewer.source_file_comments, the string actually contains </li><!-- end template /vagrant/www/framework/admin/templates/Includes/LeftAndMain_TreeNode.ss -->.
Therefore the existing </li> is not removed and the output becomes:

<ul>
    <li>Page 1</li></li>
    <li>Page 2</li> 
        <ul>
            <li> SubPage</li>
        </ul>
    </li>
    <li>Page 3</li></li>
</ul>

[Zauberfisch]

I've discussed 3 possible approaches to solve this issue with @dhensby on skype:

1. force the comments off when the TreeNode is rendered
   This would probably work fine. But stuff will break if developers overwrite the template and add things after the closing tag.
2. get really clever with looking for the last closing </li>
3. strip all </li> closing tags as they are optional
   I personally like this option best. This would be a simple str_replace, and I can't think of any side effects that could come from this at the moment, but obviously should be tested.

[dhensby]

4. Return to old template with missing </li>

I think for this we should probably just do that as it's the least risky

[Zauberfisch]

I actually happen to believe 3. is less risky. Because with 4. a developer might still overwrite the template and add </li> there.

[dhensby]

I've had a quick look at removing </li> approach and it causes test failures (which can be rectified by changing the expected output to match) but I think that's a good bellwether for if it should be determined as an API change.

That leaves us two choices, really, strip comments or just roll back to the nasty way we were. TBH, I think the nasty way we were is how we should have left it for a patch fix.

[Zauberfisch]

considering that it was just a patch, I guess I agree.
But now that it's already in, and released, I would rather see comments being disabled for that part to avoid the comments rather than reverting.

[patricknelson]

Just wanted to throw this out there as a reminder. Technically there's no need for closing </li> tags unless you are putting other tags in there adjacent to the <li>'s and that doesn't seem to be the case. Maybe that'll simplify things a bit? 😎

p.s. I had to hunt this down since the links in the release announcement don't work (not sure if this is normal).

[dhensby]

@patricknelson yep, that's @Zauberfisch's point 3

[Zauberfisch]

while I would have chosen a different approach than the fix in #6605 , I am happy to report that I am no longer able to reproduce the issue. I see an RC for a new release has already been tagged, - thanks @dhensby - therefore I am closing this issue.

[dhensby]

@Zauberfisch feel free to open an alternative approach against 3 or master. We had to get a fix in that we knew would work under tight time constraints