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 install error for some paths #989

Merged
merged 5 commits into from
Oct 19, 2023
Merged

Conversation

singular0
Copy link
Contributor

Instead of str_replace, which may, instead of truncating lead directory from the path, replace some random inner part of it, use substr to remove it.

@LukeTowers
Copy link
Member

@singular0 would you be willing to add a test case for this?

@singular0
Copy link
Contributor Author

singular0 commented Oct 14, 2023

@LukeTowers well, this will not fit into modules/system/tests/classes/FileManifestTest.php, as to test you have to initialize FileManifest with $root path that is substring of inner files paths (like "/test"). not sure how to proceed here. create separate TestCase for this in a separate file?

@LukeTowers
Copy link
Member

@singular0 you can put it in the same test, you would just initialize a new local instance of FileManifest instead of using the pre-initialized instance.

@singular0
Copy link
Contributor Author

@LukeTowers ok, so i was unable to create direct test of the #988 itself as github runner (as well as my local macos system integrity protection) does not allow to create directories in /. the best thing i could come up with is the test that proves changed FileManifest->getFilename() to work correctly.

@LukeTowers LukeTowers added Status: Completed maintenance PRs that fix bugs, are translation changes or make only minor changes labels Oct 19, 2023
@LukeTowers LukeTowers added this to the v1.2.4 milestone Oct 19, 2023
@LukeTowers LukeTowers changed the title Fix install error for some paths (#988) Fix install error for some paths Oct 19, 2023
@LukeTowers LukeTowers merged commit f69164a into wintercms:develop Oct 19, 2023
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance PRs that fix bugs, are translation changes or make only minor changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants