Support advisories from multiple repositories for the same package #11436
Conversation
| unset($packageConstraintMap[$nameFound]); | ||
| } | ||
|
|
||
| $advisories = array_merge($advisories, $result['advisories']); |
There was a problem hiding this comment.
Getting rid of array_merge() in a loop is also a performance improvement.
| } | ||
|
|
||
| $advisories = array_merge_recursive(...$advisories); |
There was a problem hiding this comment.
This is a bad idea to me, as advisories themselves are also arrays so a recursive merge could end up doing weird things. This should be array_merge(...$advisories) instead (also, be careful if $advisories is empty if no repository supports advisories, as both array_merge and array_merge_recursive require at least one argument)
There was a problem hiding this comment.
Thanks for the review.
- Used array_merge originally but it did not create the expected result. I would also avoid nested merging for sure.
- You are right about the second part, calling array_merge* without an argument is only possible since 7.4.0 and Composer requires 7.2.5.
There was a problem hiding this comment.
- but it did not create the expected result.
what do you mean with that ? AFAICT, you need to merge only the first level of the list (to do the equivalent of what $advisories = array_merge($advisories, $result['advisories']) was doing in the loop), and that's exactly what array_merge(...$advisories) will be doing.
There was a problem hiding this comment.
Additional info for the 1st:
The task
with array_merge()
with array_merge_recursive()
| } | ||
|
|
||
| $advisories = array_merge_recursive(...$advisories); |
There was a problem hiding this comment.
- but it did not create the expected result.
what do you mean with that ? AFAICT, you need to merge only the first level of the list (to do the equivalent of what $advisories = array_merge($advisories, $result['advisories']) was doing in the loop), and that's exactly what array_merge(...$advisories) will be doing.
| } | ||
|
|
||
| $advisories = array_merge($advisories, $result['advisories']); | ||
| $advisories[] = $repository->getSecurityAdvisories($packageConstraintMap, $allowPartialAdvisories)['advisories']; |
There was a problem hiding this comment.
This variable should probably be renamed to $repositoryAdvisories instead of $advisories as it is not a list of advisories (you get the list of advisories only after the merge). This would make the code easier to read IMO.
|
Ok let's give this a shot in 2.6 and see.. if it creates any problems we'll deal with them then. |
|
Thank you! |
Closes #11435.