sebastianbergmann/comparator:5.0.2 Introduces (for Drupal) breaking changes

Putting this on NR to get some confirmation if this is the way to move forward.

The real MR needs to not-bump versions, but doing this here temporarily is making detecting test-failures much easier.

We have a Drupal\Component\Utility\NestedArray already. Wouldn’t it make sense to extend it to add a deepSort() method? So we could also add some testing for it.

Converting an object to an array maybe does not need an helper, better explicitly be done when needed. It’s a one line anyway.

Thanks @mondrake, I agree mostly with your review, but that would be in the case this issue would be about actual deep sorting an array.

It isn't, it's about a one-thing-to-fix-one-thing: Massaging data so it will be accepted by assertEqualsCanonicalizing() the same way it was before this update.

Updated the naming (which didn't explain that at all, mea culpa) and sprinkled comments on some cleaned-up code.

In my mind, this would benefit from being a wrapper somehow around assertEqualsCanonicalizing() to save core, and especially contrib from watching test-failures first to see where data-massaging is needed.
Not sure if this is possible, nor even desirable for core.

Anyway, putting this on NR to get more of your (and others) feedback.

Trying an alternative approach in MR!9330 - just making sure that Cache::mergeTags() and ::mergeContexts() return actual lists i.e. after removing dupes, the resulting array is sequenced 0...n.

Spokje → closed merge request !9240

Thanks @mondrake, this is exactly what @longwave intended in the original Slack-discussion.
I misunderstood it and tried (and failed) to use array_values in the failing tests, instead of the method that causes the test failure.

All this needs is a CR so contrib knows what to do if it hits this test-failure in its existing tests and this is RTBC.

@Spokje I am not sure about a CR here. The behavior change happened in PHPUnit, and I do not think we should echo their change, otherwise we should do that for any change in PHPUnit. The fix is an implementation detail, no one should really care about that. Happy to hear different opinions, though.

@mondrake:

longwave
Aug 16th at 11:13 AM
yeah maybe we just fix core uses, document the solution with a change record, and move on
11:14
to me this is a BC break in sebastianbergmann/comparator but I assume Sebastian will see it as a bug and not a feature

@longwave in Slack (https://drupal.slack.com/archives/C079NQPQUEN/p1723799591056949?thread_t...)

Ah OK. Added draft CR https://www.drupal.org/node/3470434 → .

I took the bold step to rewrite the CR a little, to help contrib find it more easily and advice on how this was fixed in core.

If you agree with it (feel free to rewrite it if not), I'm willing to RTBC this.

Looks good. Setting to RTBC as confirmation of the intent already expressed in #28

Thanks, I think explicitly returning a list here is more correct behaviour and it's cleaner than having to modify multiple tests (and perhaps some in contrib or custom code).

Committed to 11.x. As a minor behaviour change I don't think it's worth backporting to a patch release or even down to 10.4.x given we only need it in order to pass tests.

Also published the CR.

mondrake → closed merge request !9330

Committed to 11.x. As a minor behaviour change I don't think it's worth backporting to a patch release or even down to 10.4.x given we only need it in order to pass tests.

As a module maintainer, just wanted to chime in to say that I would appreciate this being applied to as many Drupal versions as possible (10.3+ would be best), so I can get rid of custom overrides for making my module’s CI pass as soon as possible. Currently, I’d have to wait until the MAX_PHP_VERSION test uses Drupal 11.1 (I think?) before I can remove the custom override.