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

MNT Fix unit test #621

Conversation

emteknetnz
Copy link
Member

@emteknetnz emteknetnz commented Dec 16, 2024

Issue silverstripe/.github#349

Fixes https://github.com/silverstripe/silverstripe-subsites/actions/runs/12342588552/job/34442723479#step:12:109

1) SilverStripe\Subsites\Tests\GroupSubsitesTest::testAlternateTreeTitle
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'The A Team <i>(global group)</i>'
+'The A Team &lt;i&gt;(global group)&lt;/i&gt;'

@emteknetnz emteknetnz marked this pull request as ready for review December 16, 2024 01:59
Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we sure this behaviour is correct, and isn't indicative of an unintended regression? When changing a test to match a change in behaviour I'd expect to see some explanation of why the behavioural change is correct.

@emteknetnz
Copy link
Member Author

The unit test is simply calling Hierachry::getTreeTitle() which has recently been updated

    public function getTreeTitle(): string
    {
        $owner = $this->getOwner();
        $title = $owner->MenuTitle ?: $owner->Title;
        $owner->extend('updateTreeTitle', $title);
        return Convert::raw2xml($title ?? '');
    }

@GuySartorelli
Copy link
Member

Right, I get that, but should raw2xml be applied there? It looks like it wasn't in the past or this test wouldn't have failed. Does this cause peoples' legitimate HTML to not render correctly in some context? If so that's an unintended regression.

I believe at the time I tested groups in TreeDropdownField without subsites but it appears that GroupSubsites::updateTreeTitle() adds some intentional HTML which should either not be added if it doesn't display as expected in context that calls getTreeTitle() - or if it would display correctly, it should be permitted instead of being escaped (i.e. raw2xml called on the title, not on the updated result)

@emteknetnz emteknetnz force-pushed the pulls/4.0/broken-builds branch from 8b2f7a2 to 5b09d61 Compare December 16, 2024 22:27
@emteknetnz
Copy link
Member Author

Updated

$title = Convert::raw2xml($title);
$owner->extend('updateTreeTitle', $title);
return $title;
}
Copy link
Member

@GuySartorelli GuySartorelli Dec 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixes the specific unit test rather than the underlying regression.

If updateTreeTitle should permit HTML content, we should change where raw2xml() is called in the method that calls it (in Hierarchy) so that other updateTreeTitle extension hooks can behave as expected

@emteknetnz
Copy link
Member Author

emteknetnz commented Dec 17, 2024

Closed in favour of silverstripe/silverstripe-framework#11517

@emteknetnz emteknetnz closed this Dec 17, 2024
@GuySartorelli GuySartorelli deleted the pulls/4.0/broken-builds branch December 17, 2024 22:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants