Skip to content

Fix variable shadowing bug in utilCombinedTags sort #11932

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.

Sign up for GitHub

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

Merged
tyrasd merged 6 commits into from
Mar 4, 2026
+31 −2
Merged

Fix variable shadowing bug in utilCombinedTags sort #11932

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9ed0cad
Fix variable shadowing bug in utilCombinedTags sort
JaiswalShivang Feb 26, 2026
4e803d6
Update modules/util/util.js
JaiswalShivang Mar 3, 2026
23fa853
Add tests for utilCombinedTags function
JaiswalShivang Mar 3, 2026
9102906
Refactor utilCombinedTags tests for clarity and structure
JaiswalShivang Mar 3, 2026
8182044
Merge branch 'develop' into fix/utilCombinedTags-variable-shadowing
tyrasd Mar 4, 2026
3446bc0
add to changelog
tyrasd Mar 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ _Breaking developer changes, which may affect downstream projects or sites that
* Make wording of fix for _overlapping features_ validator less ambiguous ([#9888], thanks [@k-yle])
* Skip `disconnected_way` validation for Golf Paths ([#11863], thanks [@Kayd-06])
#### :bug: Bugfixes
* Fix the sorting of tag values when multiple objects' tags are combined in `utilCombinedTags` ([#11932], thanks [@JaiswalShivang])
#### :earth_asia: Localization
* Support territory-level phone hints ([#10904], thanks [@Vectorial1024])
#### :hourglass: Performance
Expand All @@ -67,6 +68,7 @@ _Breaking developer changes, which may affect downstream projects or sites that
[#11778]: https://github.com/openstreetmap/iD/pull/11778
[#11865]: https://github.com/openstreetmap/iD/pull/11865
[#11876]: https://github.com/openstreetmap/iD/pull/11876
[#11932]: https://github.com/openstreetmap/iD/pull/11932
[#11944]: https://github.com/openstreetmap/iD/pull/11944
[#11952]: https://github.com/openstreetmap/iD/pull/11952
[@Shrinks99]: https://github.com/Shrinks99
Expand Down
3 changes: 1 addition & 2 deletions modules/util/util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -398,12 +398,11 @@ export function utilCombinedTags(entityIDs, graph) {
});
});

for (var key in tags) {
for (const key in tags) {
if (!Array.isArray(tags[key])) continue;

// sort values by frequency then alphabetically
tags[key] = tags[key].sort(function(val1, val2) {
var key = key; // capture
Copy link
Copy Markdown
Member

@tyrasd tyrasd Mar 3, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm intrigued what this was supposed to do in the first place. @quincylvania it looks like this was added in 1b331bb#diff-212effbbd75b34b6d4cfba40fc7787b8b1fed506653fe5636342478ed1f12d0dR303, do you remember the reasoning behind the odd line here. //edit: was it perhaps to try to pacify a linter rule like https://eslint.org/docs/latest/rules/no-loop-func?

var count2 = tagCounts[key + '=' + val2];
var count1 = tagCounts[key + '=' + val1];
if (count2 !== count1) {
Expand Down
28 changes: 28 additions & 0 deletions test/spec/util/util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,34 @@ describe('iD.util', function() {
});
});

describe('utilCombinedTags', function() {
it('sorts tag values by frequency then alphabetically', function() {
var n1 = iD.osmNode({ id: 'n-1', tags: { surface: 'paved' } });
var n2 = iD.osmNode({ id: 'n-2', tags: { surface: 'paved' } });
var n3 = iD.osmNode({ id: 'n-3', tags: { surface: 'paved' } });
var n4 = iD.osmNode({ id: 'n-4', tags: { surface: 'asphalt' } });
var n5 = iD.osmNode({ id: 'n-5', tags: { surface: 'gravel' } });
var graph = iD.coreGraph([n1, n2, n3, n4, n5]);
var result = iD.utilCombinedTags(['n-1', 'n-2', 'n-3', 'n-4', 'n-5'], graph);

expect(result.surface).to.be.an('array');
expect(result.surface[0]).to.eql('paved');
expect(result.surface[1]).to.eql('asphalt');
expect(result.surface[2]).to.eql('gravel');
});

it('returns raw value when all entities share the same tag value', function() {
var n1 = iD.osmNode({ id: 'n-1', tags: { highway: 'residential' } });
var n2 = iD.osmNode({ id: 'n-2', tags: { highway: 'residential' } });
var graph = iD.coreGraph([n1, n2]);
var result = iD.utilCombinedTags(['n-1', 'n-2'], graph);

expect(result.highway).to.eql('residential');
});
});



it('utilTagText', function() {
expect(iD.utilTagText({})).to.eql('');
expect(iD.utilTagText({tags:{foo:'bar'}})).to.eql('foo=bar');
Expand Down