Skip to content
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.

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

Use appropriate for loop on preEventDeliveryQueue array instead of for…in #5762

Merged

Conversation

dongilbert
Copy link
Member

Q A
Bug fix? Y
New feature? N
Automated tests included? N
Related user documentation PR URL N/A
Related developer documentation PR URL https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/for...in
Issues addressed (#s or URLs) Pending
BC breaks? N
Deprecations? N

Description:

for...in loops do not work consistently across browsers when using them to iterate over an array. They are not guaranteed to always return in the same order and doing that style of loop on an array returns all properties of the array, not just the numeric indexed items.

This bug was introduced with this commit: a6150e8#diff-33b07c1d837daff498f70f652062659eR215

Steps to reproduce the bug:

  1. Create Dynamic Content and embed it in a page where you have loaded the mtc.js
  2. Load the page in the Safari or Chrome browsers, and see the console error #<Object> is not a function

Steps to test this PR:

  1. Apply PR
  2. Retest and see the error no longer appears.

@dongilbert dongilbert added bug Issues or PR's relating to bugs ready-to-test PR's that are ready to test code-review-needed PR's that require a code review before merging labels Mar 1, 2018
@mautibot mautibot added code-review-needed PR's that require a code review before merging and removed code-review-needed PR's that require a code review before merging labels Mar 1, 2018
@escopecz escopecz self-assigned this Mar 1, 2018
Copy link
Sponsor Member

@escopecz escopecz left a comment

Choose a reason for hiding this comment

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

I wasn't able to replicate the error. My browsers probably doesn't have that problem. It works fine with the update too. 👍

@mautibot mautibot removed the code-review-needed PR's that require a code review before merging label Mar 1, 2018
@javjim
Copy link

javjim commented Mar 2, 2018

wasnt able to reproduce bug it works properly, maybe someone can test and be able to reproduce bug

@dbhurley dbhurley added this to the 2.13.0 milestone Mar 4, 2018
@Maxell92
Copy link
Contributor

Cannot replicate an error too. Maybe any specific settings?

@Maxell92 Maxell92 added pending-feedback PR's and issues that are awaiting feedback from the author and removed ready-to-test PR's that are ready to test labels Mar 19, 2018
@Maxell92 Maxell92 self-assigned this Mar 21, 2018
@mleffler
Copy link
Contributor

@dongilbert Any comments?

@dongilbert
Copy link
Member Author

No comment

@mleffler
Copy link
Contributor

Looks good, discussed with David, merging.

@mleffler mleffler merged commit 0b1e20c into mautic:staging Mar 27, 2018
@mleffler
Copy link
Contributor

#5762

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issues or PR's relating to bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants