Color swatches work with disparate product IDs

[clockworkgeek]

There is a bug in ConfigurableSwatches module where product IDs are sorted numerically then compared alphabetically.
For example, "2" is less than "10" numerically, but "2" is greater than "10" because it is also greater than "1" alphabetically.
This led to cases where a child product with a shorter ID than it's siblings would break swatch switching for those parent products only.
The bug was identified in https://magento.com/tech-resources/bug-tracking/issue/index/id/1305/ @ 2/15/2016

This fix removes the buggy ConfigurableMediaImages.arrayIntersect method in favour of Prototype's Array#intersect

[Flyingmana]

Is the removal of the function part of an official fix?
I would prefer to keep it, and mark it as deprecated with a pointer of what should be used instead.

We then can remove it, when we one day have something like a major release

[edannenberg]

For full BC we could also fix the buggy function and leave everything else as is:

    arrayIntersect: function(a, b) {
        return a.intersect(b);
    },

[clockworkgeek]

That is the most sensible path, I'll do it now.

[Flyingmana]

one thing to note, the [].intersect which is used here comes from prototype and is not a native Javascript functionality.

[edannenberg]

@clockworkgeek, Personally I would drop the first commit completely, less stuff one has to process with future merges, etc.

@Flyingmana, I'm not up-to-date on that front, is running Magento 1 without Prototype a thing now?

[clockworkgeek]

@edannenberg the maintainers of a project have the option of squash merging. If you think about it this is necessary because contributors cannot un-commit published branches, and a new branch would mean a new pull request. Squashing must therefore be done upstream.

[edannenberg]

What I actually meant to say is that I would still use the old function instead of using .disect directly. If by some miracle Magento decides to fix arrayIntersect upstream in some other way we wouldn't need to remember to undo those extra changes outside of arrayIntersect.

PS: You can always squash/rebase and then push --force-with-lease to your PR branch and this PR will update accordingly.

[clockworkgeek]

I did as you suggest. I've always resisted altering public histories so push --force-with-lease was a novel experience. Project leaders might have different tastes about the inclusion of some details but I would prefer to offer a choice. It's all personal opinion anyway and those things always differ. For this small issue it hardly matters.

[edannenberg]

I did as you suggest.

Don't mind me, I'm just the local git nazi, not a maintainer. :P I have zero say in this.

I've always resisted altering public histories so push --force-with-lease was a novel experience.

It's fairly common practice when using a feature branch workflow, as it ensures a clean history in the main branch(es) while still allowing for review, qa, etc. I agree that for smaller projects it is a matter of taste/opinion/OCD level. As projects get larger though a clean git history is an investment in the future, much like good documentation and tests. It will pay off.

[luigifab]

Tested like this:

In my web console: function arrayIntersect(a, b) { ... the deleted code ... }
Then:

17:55:46,369 arrayIntersect([1,2,3], [1,2]);
17:55:46,434
Array [ 1, 2 ]
17:55:56,089 [1,2,3].intersect([1,2]);
17:55:56,157
Array [ 1, 2 ]

17:56:23,948 arrayIntersect([1,2,3,4,5,6,7,8,9,10,11,12], [1,2,10]);
17:56:24,020
Array(3) [ 1, 2, 10 ]
17:56:40,802 [1,2,3,4,5,6,7,8,9,10,11,12].intersect([1,2,10]);
17:56:40,862
Array(3) [ 1, 2, 10 ]

17:57:17,372 arrayIntersect(["1","2","3","4","5","6","7","8","9","10","11","12"], ["1","2","10"]);
17:57:17,452
Array [ "1", "2" ]
17:57:28,605 ["1","2","3","4","5","6","7","8","9","10","11","12"].intersect(["1","2","10"]);
17:57:28,683
Array(3) [ "1", "2", "10" ]

Not sure if this is the problem explained, but it seems to work better.

[addison74]

This one seems to be merged in a wrong branch 1.9.3.x.

BTW, I checked all branches in OM repository and I suggest to maintainers to delete most of them. All are with hundreds or thousands commits behind OM-19.14. Anyone who is forking the repository with get this outdated content. We should have only two branches 19 and 20, or renamed based on community votes.