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

Default arg and actual arg key mismatch in comment sanitizer #4624

Closed
schlessera opened this issue Apr 24, 2020 · 1 comment · Fixed by #6546
Closed

Default arg and actual arg key mismatch in comment sanitizer #4624

schlessera opened this issue Apr 24, 2020 · 1 comment · Fixed by #6546
Assignees
Labels
Bug Something isn't working Changelogged Whether the issue/PR has been added to release notes. P2 Low priority Sanitizers WS:Perf Work stream for Metrics, Performance and Optimizer
Milestone

Comments

@schlessera
Copy link
Collaborator

Bug Description

The default argument for enabling or disabling the comments live list in the comments sanitizer is set here:

https://github.com/ampproject/amp-wp/blob/1.4.4/includes/sanitizers/class-amp-comments-sanitizer.php#L23

The actual value of the argument after filtering is then supposed to be used here:

https://github.com/ampproject/amp-wp/blob/1.4.4/includes/sanitizers/class-amp-comments-sanitizer.php#L48

Note that the two use different keys, so no default value is actually set for the argument that is being used, and the condition just happens to resolve in the same way by coincidence:

'comment_live_list'  // <= key of the default argument being set
'comments_live_list' // <= key of the actual argument being used

Bugs like this are why I advocate using constants for pretty much any magic string we use throughout the code, as they are very hard to detect otherwise.

Expected Behaviour

The default argument should be set to the same key as the actual value being used.


Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

Implementation brief

QA testing instructions

Demo

Changelog entry

@schlessera schlessera added Bug Something isn't working Sanitizers labels Apr 24, 2020
@westonruter westonruter added the P2 Low priority label Apr 24, 2020
@kmyram kmyram added the WS:Perf Work stream for Metrics, Performance and Optimizer label Aug 5, 2020
@westonruter westonruter self-assigned this Sep 2, 2021
@westonruter westonruter added this to the v2.2 milestone Sep 2, 2021
westonruter added a commit that referenced this issue Sep 2, 2021
westonruter added a commit that referenced this issue Sep 2, 2021
westonruter added a commit that referenced this issue Sep 4, 2021
@westonruter westonruter removed their assignment Oct 13, 2021
@dhaval-parekh
Copy link
Collaborator

Passed

I have checked the current code in the develop branch and confirm that this is no longer the case and key of the default argument is updated and there is no other instance where a different key is being used. reference

@westonruter westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Dec 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working Changelogged Whether the issue/PR has been added to release notes. P2 Low priority Sanitizers WS:Perf Work stream for Metrics, Performance and Optimizer
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants