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

Wraps file extension comparison components in strtolower() #5096

Merged

Conversation

DanielGordonIT
Copy link
Contributor

@DanielGordonIT DanielGordonIT commented Jul 3, 2024

Created to fix #5095
This avoids the issue where replacing file.PNG with newfile.png fails due to "PNG" not being equal to "png"

This avoids the issue where replacing file.PNG with newfile.png fails due to "PNG" not being equal to "png"
@ssddanbrown ssddanbrown added this to the BookStack v24.05.3 milestone Jul 3, 2024
@ssddanbrown
Copy link
Member

Thanks @DanielGordonIT for providing this!

If you're confident with such a codebase, it would also be good to have a test to cover this scenario.
We have some test guidance here.
Here's an existing test related to this scenario, so an added test would be similar:

public function test_image_file_update_does_not_allow_change_in_image_extension()
{
$page = $this->entities->page();
$this->asEditor();
$imgDetails = $this->files->uploadGalleryImageToPage($this, $page);
$relPath = $imgDetails['path'];
$newUpload = $this->files->uploadedImage('updated-image.jpg', 'compressed.png');
$imageId = $imgDetails['response']->id;
$this->call('PUT', "/images/{$imageId}/file", [], [], ['file' => $newUpload])
->assertJson([
"message" => "Image file replacements must be of the same type",
"status" => "error",
]);
$this->files->deleteAtRelativePath($relPath);
}

If you're not able to add this, that's fine, just let me know and I can write the test during review.

@DanielGordonIT
Copy link
Contributor Author

I will work on creating a test for this tonight! If I am not able to make the test (PHP is not a language I am very familiar with, not that it's stopped me from using the theme system), I will let you know.

@DanielGordonIT
Copy link
Contributor Author

DanielGordonIT commented Jul 5, 2024

@ssddanbrown I've added what I think would be a functional test case, but please do give it a review.

Wait no I did not the commit didn't work

@ssddanbrown
Copy link
Member

This all looks good @DanielGordonIT, Thank you very much again for providing this and for putting in the extra effort to write a test! Merging for next patch release.

@ssddanbrown ssddanbrown merged commit ddec809 into BookStackApp:development Jul 14, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Image replacement is case-sensitive when it should not be
2 participants