SMF icon indicating copy to clipboard operation
SMF copied to clipboard

"No newline at end of file" policy

Open m4z opened this issue 1 year ago • 3 comments

While I'm not strictly a developer, I'd like to discuss this policy.

As of now, our README doesn't even mention it, which raises the barrier to contribute, as it has caused rework for new and old contributors—myself included—in the past, and (AFAIK) still makes it impossible to use the GitHub web editor (because that adds a newline, a behavior which can't be changed). If the policy is documented elsewhere, I didn't find it.

I'm not a PHP expert, but this SO discussion makes me think we use the policy based on a misinterpretation of the unclearly phrased and outdated PSR-2, which has since been superseded by PSR-12 and received better phrasing. (See also this very helpful comment elsewhere, which links to a discussion on that specific—phrasing, not policy—change.)

So I'd like us to either:

  • document the current no-newline policy for new contributors
  • drop the policy (my preference)

m4z avatar Jul 23 '23 17:07 m4z

The reason that we have the policy is because we include closing PHP tags in all our files. A newline after the closing PHP tag will break output.

So why do we include closing PHP tags? Apparently because the package manager needs them to be there in order to work properly. I've never looked into why that is or if it can be changed. But at any rate, that's why we currently have the policy against trailing newlines.

As for making that policy explicit, that's a good idea. It should probably be added to the README.md file.

In the long term, it would probably be worthwhile to look into changing the package manager so that it doesn't need closing PHP tags to be present. But that sort of change will need to wait until 2.2 at the earliest.

Sesquipedalian avatar Jul 23 '23 20:07 Sesquipedalian

It was more of a problem back in the day because:

  1. Boardmod format used <search>?></search> which could be anywhere (also why we break up ?> in the code. This was only a problem when replace was used and the code ended with a extra line ending.
  2. The xml format for searching End of File looked for ?> and if anything occurred after it (except returns), it would fail and just append after.

The first issue has become less of a issue since boardmod is not actively used (it could be a candidate for removal in a future release). The second can still occur. We do our best to ignore extra white space, but it still happens.

Mods installs would occasionally break forums or add output to the top because of the extra whitespace caused it to failt to install properly.

I believe removing ?> gets more difficult for undo operations because mods where expected to be uninstalled in the reverse order they where installed. I think we can argue that replacing it with nothing is acceptable for when a EOF operation is requested to be undone.

I honestly would like to get rid of the closing PHP statement. It isn't needed and always ensures we don't break out of PHP unexpectedly

jdarwood007 avatar Jul 23 '23 20:07 jdarwood007

Well, if that's all it is, it shouldn't be too difficult to adjust the package manager so that we can drop the closing PHP tags. A regex to find the end of a file is simple. It would still need to be something for 2.2, though.

Sesquipedalian avatar Jul 24 '23 01:07 Sesquipedalian