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

Changes to canonical url being preferred for site.page #9185

Closed
patmmccann opened this issue Nov 1, 2022 · 9 comments
Closed

Changes to canonical url being preferred for site.page #9185

patmmccann opened this issue Nov 1, 2022 · 9 comments
Assignees

Comments

@patmmccann
Copy link
Collaborator

As discussed in #9152

  • canonical looses its query string params
  • Some pubs have the wrong canonical on their page for some reason

cc: @jsadwith @nllerandi

Proposal, ignore canonical being in pub config, append query string by default

@dgirardi
Copy link
Collaborator

dgirardi commented Nov 9, 2022

Would it make sense to have a config toggle that gives preference to the page's location over canonicalUrl?

pbjs.setConfig({
   useCanonicalURL: false
})

@dgirardi
Copy link
Collaborator

dgirardi commented Nov 9, 2022

Consensus is that the referer logic should be updated to refererInfo.page = getConfig('pageUrl') || refererInfo.location || refererInfo.canonicalUrl.

@nllerandi3lift
Copy link
Contributor

Consensus is that the referer logic should be updated to refererInfo.page = getConfig('pageUrl') || refererInfo.location || refererInfo.canonicalUrl.

Was it refererInfo.location or refererInfo.topmostLocation as the second step?

@dgirardi
Copy link
Collaborator

dgirardi commented Nov 9, 2022

@nllerandi3lift, consensus is that canonical should be used only if prebid is unable to determine the top frame location. That translates to refererInfo.location as the second step. Yes, I am regretting my name choices on those.

@dgirardi
Copy link
Collaborator

dgirardi commented Nov 9, 2022

For a bit of history:

It's unclear if the first was an attempt to solve the same problem with the expectation that adapters would pick up canonicalUrl. @mmoschovas, do you remember the context?

@nllerandi3lift
Copy link
Contributor

cc @patrickloughrey @bwoolcott as I'll be on leave for some weeks any day now.
fyi @patmmccann

@mmoschovas
Copy link
Contributor

@dgirardi Sorry, I dont really remember the context here. @bretg may remember the reason for this change

@bretg
Copy link
Collaborator

bretg commented Nov 15, 2022

Sorry, wasn't really involved in this effort. FWIW, I think there are too many URL choices.

@patmmccann patmmccann moved this from Needs Req to PR submitted in Prebid.js Tactical Issues table Nov 28, 2022
@patmmccann
Copy link
Collaborator Author

fixed in #9241

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

No branches or pull requests

6 participants