jenkinsci / simple-theme-plugin

A simple theme plugin for Jenkins
https://plugins.jenkins.io/simple-theme-plugin
MIT License
56 stars 46 forks source link

Fix for loop bug which breaks favicon replacement #171

Closed mdouek closed 10 months ago

mdouek commented 10 months ago

When iterating over the list , if a link is removed the list gets one element shorter which was causing things to be skipped over. Switched to only bumping i if nothing is removed. This was breaking the favicon replacement as "alternate icon" was getting skipped over. Tested by building own plugin and testing own environment.

Testing done

### Submitter checklist
- [x] Make sure you are opening from a **topic/feature/bugfix branch** (right side) and not your main branch!
- [x] Ensure that the pull request title represents the desired changelog entry
- [x] Please describe what you did
- [x] Link to relevant issues in GitHub or Jira
- [x] Link to relevant pull requests, esp. upstream and downstream changes
- [x] Ensure you have provided tests - that demonstrates feature works or fixes the issue
mdouek commented 10 months ago

Note: the commit message is wrong should be "only bump i when link isnt removed"

TobiX commented 10 months ago

Hmm. Interesting. Is the view returned by getElementsByTagName dynamic in all browsers? So it does shrink when a matching item gets removed? Either way, it might make things cleared to keep the loop as-is, but copy the result of getElementsByTagName to a static array before the loop to avoid the dynamic nature of this array in the first place... (In fact, MDN suggests something similar: https://developer.mozilla.org/en-US/docs/Web/API/HTMLCollection)

mdouek commented 10 months ago

@TobiX I took your advice and just created a static array, makes things easier. sorry for all the commits. I can create a new branch with the one commit and a new PR to keep git cleaner.

TobiX commented 10 months ago

All fine, I can simply squash all changes.