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

fix: handle exceptions when using /0 as a URL #2825

Merged
merged 1 commit into from
Nov 19, 2023

Conversation

wilr
Copy link
Member

@wilr wilr commented Jan 20, 2023

Fix for #2680

@michalkleiner
Copy link
Contributor

Thanks Will. Since this is fixing a bug, it can target 4.12.
As per @GuySartorelli's comment, the fix may not be as simple as this, so unit test(s) would be needed as well.

@shanholmes
Copy link

This is still an issue and has since carried over to the 5 branch of the CMS.

@GuySartorelli
Copy link
Member

Rebased, added tests, and made changes necessary for those tests to pass.

@GuySartorelli GuySartorelli changed the base branch from 4.12 to 4.13 November 17, 2023 03:05
tests/php/Model/SiteTreeTest.php Outdated Show resolved Hide resolved
tests/php/Model/SiteTreeTest.php Outdated Show resolved Hide resolved
Comment on lines 458 to +459
$link = trim(Director::makeRelative($link) ?? '', '/');
if (!$link) {
if ($link === false || $link === null || $link === '') {
Copy link
Member

Choose a reason for hiding this comment

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

Note that while trim() will only ever return a string, having the full check here avoids issues if we ever remove that trim.

Copy link
Member

@emteknetnz emteknetnz left a comment

Choose a reason for hiding this comment

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

Tested locally, works good

@emteknetnz emteknetnz merged commit 579986a into silverstripe:4.13 Nov 19, 2023
12 checks passed
@emteknetnz emteknetnz mentioned this pull request Dec 5, 2023
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.

5 participants