Skip to content

Conversation

@jonasschmidt95
Copy link
Contributor

Wrap file path in quotes to allow files with spaces

Copy link
Member

@Pierstoval Pierstoval left a comment

Choose a reason for hiding this comment

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

I made some suggestions for you. If you could remove the trim part, the gitignore, and replace your quotes concatenation with escapeshellarg() I'll merge it directly :)

.gitignore Outdated
ImageMagick*.tar.gz
/ImageMagick-*/

.idea
Copy link
Member

Choose a reason for hiding this comment

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

This should not be versioned. Instead, you can use a global ignore file.

Example of command to create such file:

git config --global core.excludesfile ~/.gitignore
touch ~/.gitignore
echo ".idea" >> .gitignore

This will create a global ignore file that you can use to remove all unnecessary files from Git, like .DS_STORE (for macs), or thumbs.db for Windows, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for that tip :)

src/Command.php Outdated
$path = \rtrim($path, '/');
}

$path = \trim($path, '"');
Copy link
Member

Choose a reason for hiding this comment

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

There is a flag argument to check whether to trim the value or not, reasons where inconsistent path checks. Adding trim() here makes the arg useless, while it is useful not to trim sometimes.

src/Command.php Outdated
}

$path = \trim($path, '"');
$path = '"' . $path . '"';
Copy link
Member

Choose a reason for hiding this comment

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

For shell commands, PHP already has a function for that, called escapeshellarg()

@Pierstoval Pierstoval merged commit bf7e04e into Orbitale:main Dec 9, 2024
@Pierstoval
Copy link
Member

Thanks @jonasschmidt95 👍

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