composer-normalize icon indicating copy to clipboard operation
composer-normalize copied to clipboard

Crows feet abuse (unnecessary escaping of slashes)

Open Bilge opened this issue 3 years ago • 7 comments

Steps required to reproduce the problem

  1. Run program (composer normal)

Expected Result

  • Unescaped forward slashes (/) remain unespecaed.

Actual Result

  • All forward slashes are escaped, e.g. "require": { "foo/bar": "^1" } -> "require": { "foo\/bar": "^1" }.

Bilge avatar Aug 31 '22 11:08 Bilge

I can confirm that the plugin seems to introduce these extra slashes. However, it'll only do so when they already exist. Running composer normalize on the following composer.json file makes no changes:

{
  "require": {
    "php": "^8.1",
    "composer/composer": "^2.4",
    "composer/semver": "^3.3"
  }
}

However, when I supply the following composer.json file, this plugin will add slashes to the other package:

{
  "require": {
    "php": "^8.1",
    "composer\/composer": "^2.4",
    "composer/semver": "^3.3"
  }
}

resulting in a "normalized" file with:

{
  "require": {
    "php": "^8.1",
    "composer\/composer": "^2.4",
    "composer\/semver": "^3.3"
  }
}

I would expect this plugin to remove the extra slashes rather than adding the "missing" slashes elsewhere.

fredden avatar Nov 08 '22 18:11 fredden

From what I can tell, this is behaviour is fully intentional in https://github.com/ergebnis/json-normalizer/blob/a7246139cb124a2eb41660d370fc0ac5bf5daac2/src/Format/JsonEncodeOptions.php#L43-L56

I've opened https://github.com/ergebnis/composer-normalize/pull/995 to apply more opinionated formatting. However, @localheinz may choose to close this (and the associated pull request) because the behaviour may be regarded as intentional.

fredden avatar Nov 08 '22 19:11 fredden

@fredden Thanks for the analysis and the pull. I had noticed that the problem does not always occur but had not managed to figure out why. I must say I am surprised to learn it performs some kind of heuristic and attempts to match existing patterns. I thought the whole point of this tool was to enforce an opinionated standard based on subjective preferences. And yet, as for as crows feet goes, there is little subjective about this at all; it should be clearly observed that to introduce such syntax makes a human-readable file format less readable and increases the file size unnecessarily. If it was ever fine to be opinionated about something, it should be this.

Bilge avatar Nov 08 '22 19:11 Bilge

@Bilge

Your argument is valid, and given @fredden's proposals in https://github.com/ergebnis/json-normalizer/pull/756, I think I will be make some changes here!

localheinz avatar Dec 26 '22 13:12 localheinz

#995 should fix this here without having to adjust the linked library.

fredden avatar Dec 26 '22 13:12 fredden

@localheinz was this intentionally closed as "won't fix" or did this get closed by mistake? The reported behaviour is still being witnessed in composer-normalize version 2.39.0. I've verified this using the details in https://github.com/ergebnis/composer-normalize/issues/965#issuecomment-1307626291. See also #1231 which might be a duplicate of this issue.

fredden avatar Dec 05 '23 12:12 fredden