wp-cli / wp-config-transformer

Programmatically edit a wp-config.php file
MIT License
81 stars 25 forks source link

Replace DOS line endings with LF #49

Closed xknown closed 10 months ago

xknown commented 10 months ago

DOS line endings are not currently stripped from the wp-config files, which results in extra lines being added to the modified file.

Fixes https://github.com/wp-cli/config-command/issues/160 Related: https://github.com/wp-cli/wp-cli/issues/5859

schlessera commented 10 months ago

@xknown This is not something that can be solved with a simple str_replace(), I'm afraid.

What the code does with your changes seems worse than before.

Imagine the scenario where you have two regular blank lines in the standard notation for servers:

\n\r\n\r

The logic with this PR will first replace the middle part of that sequence, breaking the two individual blank lines. Then, as it completes, the final result will be this:

\n\n\n\n

So, for the more common configurations, it will end up turning two blank lines into four blank lines.

To solve this properly, we would need to switch away from str_replace() and use a regular expression that considers entire groups in one go and detects the patterns.

xknown commented 10 months ago

@xknown This is not something that can be solved with a simple str_replace(), I'm afraid.

What the code does with your changes seems worse than before.

Imagine the scenario where you have two regular blank lines in the standard notation for servers:

\n\r\n\r

The logic with this PR will first replace the middle part of that sequence, breaking the two individual blank lines. Then, as it completes, the final result will be this:

\n\n\n\n

That's not really the case though, because the str_replace will first replace\n\r (or \r\n) by a single \n. However, I agree that using a regular expression might probably be better to only replace those characters where it makes most sense.

schlessera commented 10 months ago

@xknown Indeed, you're right, my bad. I tested this in 3v4l.org and it seems to reliably work with all constellations.