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

Fix escaping linter errors #2296

Merged
merged 30 commits into from Nov 15, 2018

Conversation

Projects
None yet
2 participants
@donnapep
Contributor

donnapep commented Nov 6, 2018

Fixes escaping violations reported by PHPCS.

Note that I used wp_kses_post for any apply_filters calls to minimize the potential of breaking any third parties hooking into these filters.

Testing

This PR has the potential to break pretty much everything on the backend and the frontend, so careful testing is required. We should try to get this merged as soon as possible to avoid rebasing future PRs, and so that we can work from this version and watch for issues.

@donnapep donnapep self-assigned this Nov 6, 2018

@donnapep donnapep added this to the 1.12.2 milestone Nov 6, 2018

@donnapep donnapep requested a review from alexsanford Nov 6, 2018

@donnapep donnapep force-pushed the fix/escaping branch 2 times, most recently from 0de8b38 to 902070a Nov 6, 2018

@alexsanford

Alright, I haven't made it through the whole thing yet, but I wanted to leave the notes that I have so far. I've walked through the diff and stopped at includes/class-sensei-learners-main.php. I'll continue from there in my next review.

I have a few comments. A bunch of them are lines that hand an array into wp_kses or wp_kses_post as the first argument. This seems to work fine because of implementation details of those functions, but the documentation states that the first parameter should be a string. I feel like handing in an array might be an unsupported pattern that we should avoid? Or maybe it's a common practice that just isn't reflected in the documentation, in which case feel free to disregard these comments 🙂

Show resolved Hide resolved includes/class-sensei-analysis-course-list-table.php
Show resolved Hide resolved includes/class-sensei-analysis-course-list-table.php Outdated
Show resolved Hide resolved includes/class-sensei-analysis-lesson-list-table.php Outdated
Show resolved Hide resolved includes/class-sensei-analysis-overview-list-table.php Outdated
Show resolved Hide resolved includes/class-sensei-analysis-overview-list-table.php Outdated
Show resolved Hide resolved includes/class-sensei-analysis-user-profile-list-table.php Outdated
Show resolved Hide resolved includes/class-sensei-grading-main.php Outdated
Show resolved Hide resolved includes/class-sensei-grading-main.php Outdated
Show resolved Hide resolved includes/class-sensei-grading.php
Show resolved Hide resolved includes/class-sensei-learners-admin-bulk-actions-view.php Outdated
@alexsanford

Not a blocker, but it might be good to create a function for applying wp_kses_post to all of the elements in an array, rather than having that code duplicated.

Gotta run for a bit, but I'm getting closer. Wanted to give you my current set of comments. I stopped at includes/class-sensei-list-table.php and will continue when I get back.

Show resolved Hide resolved includes/class-sensei-analysis-course-list-table.php Outdated
Show resolved Hide resolved includes/class-sensei-analysis-course-list-table.php Outdated
Show resolved Hide resolved includes/class-sensei-learners-main.php Outdated
Show resolved Hide resolved includes/class-sensei-learners-main.php Outdated
Show resolved Hide resolved includes/class-sensei-learners-main.php Outdated
Show resolved Hide resolved includes/class-sensei-learners-main.php Outdated
Show resolved Hide resolved includes/class-sensei-lesson.php
@alexsanford

Phew! Made it through! A few more comments below 🙂

Show resolved Hide resolved includes/class-sensei-list-table.php Outdated
Show resolved Hide resolved includes/class-sensei-quiz.php
Show resolved Hide resolved templates/single-quiz/question_type-gap-fill.php Outdated
Show resolved Hide resolved templates/single-quiz/question_type-gap-fill.php Outdated
@alexsanford

🎉

@donnapep donnapep force-pushed the fix/escaping branch from f63c36e to 6a62a87 Nov 15, 2018

@donnapep donnapep merged commit 1fb48b4 into master Nov 15, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@donnapep donnapep deleted the fix/escaping branch Nov 15, 2018

@donnapep donnapep changed the title from Fix PHPCS violations to Fix escaping linter errors Nov 29, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment