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

Make "paths that should not be sorted" configurable #704

Closed
3 tasks done
TravisCarden opened this issue Mar 5, 2021 · 25 comments · Fixed by #1056
Closed
3 tasks done

Make "paths that should not be sorted" configurable #704

TravisCarden opened this issue Mar 5, 2021 · 25 comments · Fixed by #1056
Assignees

Comments

@TravisCarden
Copy link
Contributor

TravisCarden commented Mar 5, 2021

Some elements of composer.json are highly sensitive to order, such as extra.patches with https://github.com/cweagans/composer-patches, which applies patches in the order specified and can therefore fail if it changes. See another example #699. I need a way to exclude arbitrary paths from sorting. I see that the current exclusions are hardcoded in \Ergebnis\Json\Normalizer\Vendor\Composer\ConfigHashNormalizer::PROPERTY_PATHS_THAT_SHOULD_NOT_BE_SORTED. I would like this value to be configurable like the other options. I could take a stab at a PR. Alternatively, a few specific additions to the current hardcoded list would temporarily unblock my customers. I would gladly send a PR for that, too.

@localheinz
Copy link
Member

@TravisCarden

Which property paths are these?

@localheinz localheinz self-assigned this Mar 6, 2021
@TravisCarden
Copy link
Contributor Author

@localheinz Which property paths should be added to the hardcoded exclusions? Off the top of my head I know of the following order-dependent paths:

@rafatwork
Copy link

Also using https://github.com/cweagans/composer-patches and experiencing the same issue: patches are re-ordered, but some of them need to be applied in a specific order. I'm currently working around this issue by defining my patches in a separate file (explained @ https://github.com/cweagans/composer-patches#using-an-external-patch-file).

@FlorentTorregrosa
Copy link

Hello,

About composer-patches, a workaround is to use put number at the beginning of the description like in

            "drupal/core": {
                "1 - BrowserTestBase::setUp() must be compatible with PHPUnit\\Framework\\TestCase::setUp() - https://www.drupal.org/project/drupal/issues/3182103": "https://git.drupalcode.org/project/drupal/-/merge_requests/28.patch"
            }

Best regards,

@benr77
Copy link

benr77 commented Jan 28, 2022

I also am experiencing problems with this in the "scripts" section - I define various commands to be run via the install and update hooks (via the "auto-scripts" block shown below), but these commands need to be in a specific order, whereas when normalized they are placed in alphabetical order which is not what we want!

"scripts": {
    "post-install-cmd": [
      "@auto-scripts"
    ],
    "post-update-cmd": [
      "@auto-scripts"
    ],
    "auto-scripts": {
      "cache:clear": "symfony-cmd",
      "doctrine:migrations:migrate -v": "symfony-cmd",
      "bazinga:js-translation:dump assets --merge-domains --format=json": "symfony-cmd",
      "assets:install %PUBLIC_DIR%": "symfony-cmd"
    }
  }

When normalized changes the order and becomes:

"scripts": {
    "post-install-cmd": [
      "@auto-scripts"
    ],
    "post-update-cmd": [
      "@auto-scripts"
    ],
    "auto-scripts": {
      "assets:install %PUBLIC_DIR%": "symfony-cmd",
      "bazinga:js-translation:dump assets --merge-domains --format=json": "symfony-cmd",
      "cache:clear": "symfony-cmd",
      "doctrine:migrations:migrate -v": "symfony-cmd"
    }
  }

@localheinz
Copy link
Member

@benr77 Working on this!

@localheinz
Copy link
Member

localheinz commented Jan 31, 2022

@benr77

I am releasing a temporary fix (see #875) as ergebnis/composer-normalize:2.23.1.

Let's keep this issue open until I have solved the problem!

I apologize for any problems I have caused, and I thank you for opening these issues! Without your help, I would never even know about how different projects use composer.json, and I am learning a lot!

@benr77
Copy link

benr77 commented Jan 31, 2022

Thanks for your efforts - it really is appreciated a lot!

One point to consider - I see in the PR you are referring to auto-scripts but this is just what I have called the group - I think you can call it anything (this is just a Symfony convention IIRC).

In the scripts block at least, it is OK to sort the first level of children, but anything past that should not be sorted.

Cheers!

@localheinz
Copy link
Member

Thank you, @benr77!

@FlorentTorregrosa
Copy link

Hi,

@localheinz don't blame yourself, this tool is amazing! Thanks for providing it in open source with free usage possible!

@ergebnis-bot
Copy link
Member

Since this issue has not had any activity within the last 180 days, I have marked it as stale.

I will close it if no further activity occurs within the next 14 days.

@benr77
Copy link

benr77 commented Aug 12, 2022

I believe this issue is still unresolved, at least in the correct manner. Can we not close it please.

@Bilge
Copy link

Bilge commented Nov 8, 2022

I also cannot use this until scripts order normalization can be disabled.

@localheinz
Copy link
Member

@Bilge

Can you share an example composer.json where you are experiencing issues with the scripts section, please?

  • orginal
  • normalized
  • expected normalized

Thank you!

@localheinz
Copy link
Member

@benr77

Fixing scripts.auto-scripts with ergebnis/json-normalizer#776.

@localheinz
Copy link
Member

@TravisCarden

Fixing extra.installer-paths with ergebnis/json-normalizer#777.

@Bilge
Copy link

Bilge commented Dec 12, 2022

@localheinz I just don't want the scripts section "normalized" (alphabetized) at all. I would like to except the scripts section altogether. Same for scripts-descriptions.

@localheinz
Copy link
Member

localheinz commented Dec 12, 2022

@TravisCarden

Regarding extra.patches, can the direct children of extra.patches safely be sorted, or is that also an issue?

Original composer.json

{
  "extra": {
    "patches": {
      "drupal/foo": {
        "foo": "https://www.drupal.org/files/issues/2022-01-01/bar.patch",
        "bar": "https://www.drupal.org/files/issues/2020-12-13/foo.patch"
      },
      "drupal/bar": {
        "bar": "https://www.drupal.org/files/issues/2022-01-01/bar.patch",
        "baz": "https://www.drupal.org/files/issues/2019-01-01/baz.patch"
      }
    }
  }
}

Normalized composer.json

{
  "extra": {
    "patches": {
      "drupal/bar": {
        "bar": "https://www.drupal.org/files/issues/2022-01-01/bar.patch",
        "baz": "https://www.drupal.org/files/issues/2019-01-01/baz.patch"
      },
      "drupal/foo": {
        "foo": "https://www.drupal.org/files/issues/2022-01-01/bar.patch",
        "bar": "https://www.drupal.org/files/issues/2020-12-13/foo.patch"
      }
    }
  }
}

@localheinz
Copy link
Member

@FlorentTorregrosa @rafatwork @TravisCarden

Fixing extra.patches with ergebnis/json-normalizer#780.

Does that make sense?

@fredden
Copy link

fredden commented Dec 12, 2022

Yes, it's safe to sort children of extra.patches, but not their children. That's to say that grandchildren of extra.patches should retain their order.

@kAlvaro
Copy link

kAlvaro commented Dec 12, 2022

extra is arbitrary data that can be "virtually anything". I don't think a generic tool can make assumptions about its structure. At most, sort direct subkeys.

Regarding scripts, my own use case is like:

    "scripts": {
        "start": [
            "Composer\\Config::disableProcessTimeout",
            "php -S localhost:8080 -t public public/index.php"
        ],
        "phpstan": "phpstan analyse .",
        "phpcs": "phpcs",
        "phpcbf": "phpcbf",
        "rector-lint": "vendor/bin/rector process --dry-run",
        "rector-fix": "vendor/bin/rector process"
    },

Scripts are listed in logical blocks (build actions first, linters last). Linters are shown in order of importance/preference for the project. Each linter displays lint command first, fix command second. Order conveys information, I'm not fond of having it discarded blindly.

On the other side, composer list itself couldn't care less and alphabetises scripts nonetheless.

Would some generic opt-in/opt-out setting be a solution? Can it be flexible enough without adding too much complexity?

{
  "extra": {
    "composer-normalize": {
      "skip-keys": ["scripts", "scripts-descriptions", "extra.patches"]
    }
  }
}

@localheinz
Copy link
Member

localheinz commented Dec 12, 2022

@kAlvaro

From my point of view, JSON objects are objects or maps - if the order of properties or keys matter in an object or a map, then something is broken by design.

I am not judging, but I think it is a bad idea.

ergebnis/json-normalizer only sorts properties of objects, it never sorts lists.

@FlorentTorregrosa
Copy link

Hi,

@localheinz, about #704 (comment), I confirm, direct children of extra.patches can be sorted (and I like to sort it) but not their own children.

So ok with you comment result.

@kAlvaro
Copy link

kAlvaro commented Dec 13, 2022

JSON objects are objects or maps - if the order of properties or keys matter in an object or a map, then something is broken by design

You have a point here. Original json.org page says:

An object is an unordered set of name/value pairs".

ECMA-404 says:

The JSON syntax does not impose any restrictions on the strings used as names, does not require that name strings be unique, and does not assign any significance to the ordering of name/value pairs. These are all semantic considerations that may be defined by JSON processors or in specifications defining specific uses of JSON for data interchange

And I don't think Composer has anything to say about order.

I'd still appreciate a generic setting to customise behaviour for custom keys, but it's out of scope.

@TravisCarden
Copy link
Contributor Author

@localheinz++ 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment