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

Pubmatic Adapter dont modify shared bid data #3112

Closed
wants to merge 1 commit into from
Closed

Pubmatic Adapter dont modify shared bid data #3112

wants to merge 1 commit into from

Conversation

ReinoutStevens
Copy link
Contributor

@ReinoutStevens ReinoutStevens commented Sep 20, 2018

Type of change

  • Bugfix
  • Feature
  • New bidder adapter
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Does this change affect user-facing APIs or examples documented on http://prebid.org?
  • Other

Description of change

Ensure pubmatic adapter does not modify a shared sizes variable

Other information

#3111

Dont mess up size object
@ReinoutStevens
Copy link
Contributor Author

Note that this still leaves a suspicious looking line https://github.com/prebid/Prebid.js/blob/master/modules/pubmaticBidAdapter.js#L120

@ReinoutStevens ReinoutStevens changed the title Resolve Issue 3111 Pubmatic Adapter dont modify shared bid data Sep 20, 2018
@mkendall07
Copy link
Member

@ReinoutStevens
Is this causing an issue somewhere?

@ReinoutStevens
Copy link
Contributor Author

ReinoutStevens commented Sep 24, 2018

@mkendall07 The reported issue associated with this PR has been causing some problems. Atm we have reverted our code and no longer make use of Pubmatic's multi-size support, and specify multiple bids per size again (as it used to be the case).

@mkendall07
Copy link
Member

@ReinoutStevens
ah sorry I missed the linked issue. I think Prebid team should fix this at the core level. Will look into solution as well.

@ReinoutStevens
Copy link
Contributor Author

@mkendall07 no worries, thanks for taking a look at this.
I agree that an adapter modifying some passed objects should have no influence. I would expect a copy to be passed to each adapter, and not a shared object.
This PR is only a quick fix for a bigger problem, and I'm not sure of the impact of this change on pubmatic's adapter.

@jsnellbaker jsnellbaker requested review from jsnellbaker and removed request for mike-chowla September 25, 2018 15:45
@jsnellbaker
Copy link
Collaborator

Closing this PR in favor of the fix put together by the Pubmatic team. #3122

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants