Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Incorrect .reverse() in validations #193

Closed
AloisSeckar opened this issue Jan 19, 2024 · 1 comment
Closed

Incorrect .reverse() in validations #193

AloisSeckar opened this issue Jan 19, 2024 · 1 comment
Labels
1.2 target Issues aimed for 1.2 release help wanted Extra attention is needed maintenance Code and project improvements

Comments

@AloisSeckar
Copy link
Owner

In wbsc-validation.ts inside checkHit there lies

inputs.reverse().forEach( ... )

The usage of .reverse() unintentionally changes the order of elements in the source array. This is a classic "pass-by-reference" bug that lived under the radar for a long time.

The problem is removing it now will cause other validations to start failing, as some of them already relly on swapped element order. Therefore, fixing this little flaw also means double-checking all validations (or all that occur after checkHit). Who volunteers?

@AloisSeckar AloisSeckar added help wanted Extra attention is needed maintenance Code and project improvements labels Jan 19, 2024
@AloisSeckar AloisSeckar added the 1.2 target Issues aimed for 1.2 release label Jun 3, 2024
AloisSeckar added a commit that referenced this issue Jul 15, 2024
@AloisSeckar
Copy link
Owner Author

fixed with f79d0b8

reverse() call was removed all validation and Backstop tests are still passing - this makes me confident to close this for now. If some edge cases emerge, we will deal with them later.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
1.2 target Issues aimed for 1.2 release help wanted Extra attention is needed maintenance Code and project improvements
Projects
None yet
Development

No branches or pull requests

1 participant