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

Crows feet abuse (unnecessary escaping of slashes) #965

Open
Bilge opened this issue Aug 31, 2022 · 7 comments · Fixed by #1056
Open

Crows feet abuse (unnecessary escaping of slashes) #965

Bilge opened this issue Aug 31, 2022 · 7 comments · Fixed by #1056
Assignees
Labels

Comments

@Bilge
Copy link

Bilge commented Aug 31, 2022

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 Bilge changed the title Crows feet abuse (unnecessary escaping of slahes) Crows feet abuse (unnecessary escaping of slashes) Aug 31, 2022
@localheinz localheinz self-assigned this Sep 27, 2022
@localheinz localheinz added the bug label Sep 27, 2022
@fredden
Copy link

fredden commented Nov 8, 2022

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
Copy link

fredden commented Nov 8, 2022

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 #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.

@Bilge
Copy link
Author

Bilge commented Nov 8, 2022

@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.

@localheinz
Copy link
Member

localheinz commented Dec 26, 2022

@Bilge

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

@fredden
Copy link

fredden commented Dec 26, 2022

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

@fredden
Copy link

fredden commented Dec 5, 2023

@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 #965 (comment). See also #1231 which might be a duplicate of this issue.

@localheinz localheinz reopened this Dec 5, 2023
@adam-vessey
Copy link

adam-vessey commented Oct 18, 2024

Seemed to be triggered here in my bit of testing by the composer.json having some post install hooks containing a script that has some escaped backslashes preceding a forward slash (as in, a \\/ kind of thing).

Should https://github.com/ergebnis/json-normalizer/blob/4c62e2a63bf18758d98efff5fad0a203389ca5e9/src/Format/JsonEncodeOptions.php#L52-L54 be counting the number of backslashes before the slash to ensure there is an odd number of them, to ensure that there is indeed an escaped forward slash sequence (\/)? Something like a preg_match('#(?<!\\)(?:\\\\)*\\/#', [...])?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants