Problem/Motivation

Currently, when a "&" (and probably other special characters) are inside the page title, the ad disappears.

Steps to reproduce

Proposed resolution

Fix this. It is probably related to missing escaping. Needs to be checked throughout.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#6 ad-escape-special-characters-in-query-string-3577478-6.patch920 bytesthomas.frobieter

Issue fork ad-3577478

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

grevil created an issue. See original summary.

grevil’s picture

grevil commented

We need to use "https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global..." to encode special characters, so that stuff like & is not interpreted as the start of a query parameter.

But the funny thing is, most of the query values are not even used! I think, it was supposed be used through the bucket factory by setting "ad_context" to query->all() and then LocalTracker.php to extract page title etc. from the query.
BUT: The bucketfactory is not initiated with the whole query in ImpressionController. Only "$request_data['ads']" is being used, so "LocalTracker" can't extract anything from there.

I'll create a follow-up issue for that.

grevil’s picture

grevil commented
Status: Active » Needs review

Also I kinda hate, that the whole query is just passed to the "ads" query parameter and not split by what they actually are "uid", "page_title", etc..

Even the actual URL is passed as part of the json... that just sounds incorrect, but we are not here to do any major refactoring, so I'll just create a quick fix for now...

Note, that we don't need to do any manual decoding in the impression controller. Symfony already decodes the encoded characters (like "%26" for "&") for us.

grevil opened merge request !43

grevil’s picture

grevil commented

Created a follow-up issue for LocalTracker here: #3577496: Fix LocalTracker not actually tracking anything .

thomas.frobieter’s picture

thomas.frobieter
Primary language German
Location EU
commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new ad-escape-special-characters-in-query-string-3577478-6.patch920 bytes

LGTM!

Static patch attached.

grevil’s picture

grevil commented
Status: Reviewed & tested by the community » Fixed

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

@grevil please tag a new release with this fix

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.