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

Prebid core: fix adUnits for auction at the time requestBids is called #8637

Merged
merged 1 commit into from
Jul 25, 2022

Conversation

dgirardi
Copy link
Collaborator

@dgirardi dgirardi commented Jul 1, 2022

Type of change

  • Bugfix

Description of change

If a publisher calls pbjs.addAdUnits or pbjs.removeAdUnit after calling requestBids, those calls might have an effect on ongoing auctions - depending on whether any module needs to defer the start of the auction (for example, if consentManagement needs to look up consent data).

This changes requestBids to always use the adUnits that were defined at the moment it's called.

Other information

Closes #8628

@dgirardi dgirardi changed the title Prebid core: fix adUnits for auctoin at the time requestBids is called Prebid core: fix adUnits for auction at the time requestBids is called Jul 1, 2022
@ChrisHuie ChrisHuie requested a review from mmoschovas July 6, 2022 10:25
@jsnellbaker
Copy link
Collaborator

Tagging @snapwich to possibly review the new code in the hook.js, to see if it would have any strange side-effects with the internals of the hook package.

Copy link
Collaborator

@snapwich snapwich left a comment

Choose a reason for hiding this comment

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

from a fun-hooks perspective i don't think there should be any issue copying the methods from one function to another. i don't quite understand why that is necessary here, couldn't you just clone the adunits in requestBids to begin with without the use of this delegate?

either way i think this should work.

@dgirardi
Copy link
Collaborator Author

@snapwich there are various "before" hooks on requestBids, with many of them also falling back to the global adUnits, so I think to do what you say I'd need to add another before hook with higher priority than all of them. The main body of requestBids runs too late.

The high-priority hook was my first attempt, but I found it too hard to test, because a lot of "unit" tests already modify the global hook configuration.

Copy link
Contributor

@mmoschovas mmoschovas left a comment

Choose a reason for hiding this comment

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

lgtm

@patmmccann patmmccann merged commit 252d9d4 into prebid:master Jul 25, 2022
ccorbo pushed a commit to ccorbo/Prebid.js that referenced this pull request Jul 27, 2022
JacobKlein26 pushed a commit to nextmillenniummedia/Prebid.js that referenced this pull request Feb 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Question - Prebid v6 to v7
5 participants