News theme template cleanup #78

matt-bernhardt · 2023-03-28T19:56:33Z

This PR focuses on the issues flagged in GitHub Actions from #77. The changes can be grouped into two buckets:

This is the second PR that is part of https://mitlibraries.atlassian.net/browse/LM-143

General changes

Bumps the News theme to version 0.2.0
Adds the What The File plugin, for additional help when developing templates.
Adds documentation for a smoke test technique that I used to help shape this PR. The line of code in the document is no longer in place, but I wanted to preserve it for future-us.
Copy-pasting in the images/ folder, which is loaded via CSS in some of these templates.

Template changes

Updating @package and @since values in the file metadata
Fixing a few templates whose names aren't first in the file comment block
Setting all templates to use the Mitlib\News namespace
Related to the namespace, adjusting calls to libraries like DateTime or WP_Query to be properly namespaced.
Updating a handful of function calls that need to be properly namespaced to reach code inherited from the Parent theme.
Adding escaping functions when prompted by PHPCS.
Removing a few cases where there were duplicate attributes being rendered (see inc/footer.php and elsewhere, when two title attributes were being rendered).
Refactoring a long string of if statements that resulted in variables which would need wp_kses to escape, when an alternative structure that assigned only attribute values could be escaped with esc_attr() - see inc/spotlights.php
In a few cases where the standard escaping functions aren't sufficient, I've opted to use phpcs:disable to turn off linting - rather than spending the time to thoroughly understand the range of possible markup and using something like wp_kses (which would be ideal, but a lot more work - particularly considering all this code is already in production for us).
Refactoring a few calls to PHP superglobals, to either avoid touching them at all or at least do so more safely (bib-header.php, ).
The additionalPosts*.php templates have code that reads in values for $offset and $limit from the querystring. These lines were getting flagged for poor code sanitization, which has been corrected. These could still be abstracted to a shared function, but that felt like a step too far for now. Ultimately I'd love to have only one of these templates, rather than a shared function between them.
Removing a few blocks of code that reference $pageRoot and $isRoot. A few of these still remain, though, which will be removed in the next PR.
Some page templates have code blocks removed which rely on these $isRoot values. I'm open to backing out of these changes if you'd prefer, and setting up a clearer PR that focuses only on this. As I write this, it looks like this is not quite as clean as I'd like it to be yet.
A few variables that don't conform to variable naming standards are updated, but only when the changes above have already resulted in most of their references being changed (see $category_id in additionalPosts-cat.php)

Notes

I've tried to reduce the amount of extraneous cleanup, but wasn't able to hold off in a few places. Examples include one place where or was replaced by || in archive.php, and some more intensive changes in author.php like removing a commented-out block of code that was still getting flagged. Shortly after this I realized the scale of what that PR would look like, and so changed course to be more focused.
In the next (and hopefully final) PR for this ticket, I expect some page templates to be removed. I strongly suspect that we'll be removing page-subscribe.php, test-template.php, and maybe page-last-year.php. Doing this will require consulting with the folks in UX to make sure that we're handing them a theme that supports all the needed features, and that's fundamentally not part of this "make CI happy" work.

Developer

Secrets

No secrets are affected

Documentation

Project documentation has been updated (but more may be needed)

Accessibility

ANDI or Wave has been run in accordance to
our guide and
all issues introduced by these changes have been resolved or opened as new
issues (link to those issues in the Pull Request details above)

Stakeholder approval

Stakeholder approval is not needed

Dependencies

YES dependencies are updated (new plugin What The File)

Code Reviewer

The commit message is clear and follows our guidelines
(not just this pull request message)
The changes have been verified
The documentation has been updated or is unnecessary
New dependencies are appropriate or there were no changes

This next piece of work will focus on content escaping and other issues flagged by our linting tools.

The sprites image ends up being loaded by inc/spotlights.php

- Namespace and file metadata - Adjust a call to DateTime outside the namespace - Ignore one call to mb_substr that is too tricky to escape right now - convert a few translated functions to their escaped equivalents - esc_url a few times

- Namespacing and file metadata - Fixing call to DateTime outside namespace - Content escaping (including removing a double title attribute) - Converting one instance of "or" to "||"

- Content escaping in two lines - Adding the right namespace - Fixing the call to a parent theme function that builds our pagination

- Removing extra empty lines - Removing a non-snake-case variable (rCat) - Simplifying a printf command to remove extra concatenation - Removing an entire block that was confusingly commented out, which we don't use nor need.

- File metadata - escape a PHP variable used in javascript To test this, look at the script tag on a category index page, and check the Category ID parameter.

- Namespace and file metadata - content escaping content-single tweak content-single.php

- file metadata - remove pageroot / isroot lines that aren't needed

- Namespace and file metadata - content escaping

- Namespace and file metadata

- Namespacing and file metadata - Fixing one call to DateTime - Escaping a number of lines as prompted by PHPCS

- Namespace and file metadata - content escaping - convert a reference to $_SERVER['SCRIPT_URI'] to a WP-specific approach that avoids server variables

- Namespace and file metadata - content escaping

- File metadata - disable PHPCS around a function that is particularly hard to escape

- Namespace and file metadata - Properly namespaced call to DateTime - content escaping

- Namespace and file metadata - removing an extra title attribute - content escaping

- Namespace and file metadata

- Namespace and file metadata - Content escaping - Removing extra whitespace

- Namespace and file metadata

- Namespace and file metadata - Content escaping - Replacing the long string of if statements with a more tightly-scoped switch statement that only sets the relevant value (the class value assigned to the div), which is easier to escape.

- Namespace and file metadata - Disable PHPCS check for the echo on line 104, which is a regex-modified select tag(s)? - Collapse a handful of small php blocks into one coherent set of lines, trying to make this more readable.

- Namespace and file metadata

- Content escaping on two lines - Ignoring rule on two other lines which echo a blob of markup. Would need wp_kses() to do properly

- Namespace and file metadata - Better positioned template name - Content escaping - Better input validation / sanitization for limit and offset

- Namespace, template name, and file metadata - Better approach to input validation

- Namespace, tempalte name, and file metadata - Better input validation - Renaming $categoryId to $category_id since this touches most of its lines already

- Namespace, template name, and file metadata - Better input validation for $offset and $limit

- Namespace, template name, and file metadata - Better input sanitization for $offset and $limit - Content escaping - Ignoring the standard for retrieving the query_string variable, although this makes me uneasy. The bug is already there, we just don't close it here.

- Namespace, template name, and file metadata - Better input validation for $offset and $limit

JPrevost

I've skimmed this and it seems fine. Stakeholder feedback as we do more thorough testing will be the main way we know if this causes any issues.

Bump theme to 0.2.0

708c9ea

This next piece of work will focus on content escaping and other issues flagged by our linting tools.

matt-bernhardt force-pushed the lm143 branch 5 times, most recently from b2e48d8 to 2dbc7ec Compare March 29, 2023 22:07

matt-bernhardt added 24 commits March 30, 2023 13:30

Copy-pastes images folder

7ab87b7

The sprites image ends up being loaded by inc/spotlights.php

Adds what-the-file plugin

5e6141a

Add how-to page on conducting a smoke test

d285691

Cleans archive-bibliotech.php

d6ea982

- Namespace and file metadata - Adjust a call to DateTime outside the namespace - Ignore one call to mb_substr that is too tricky to escape right now - convert a few translated functions to their escaped equivalents - esc_url a few times

Cleaning archive.php

48a2e6b

- Namespacing and file metadata - Fixing call to DateTime outside namespace - Content escaping (including removing a double title attribute) - Converting one instance of "or" to "||"

Clean author.php

f9345ed

- Content escaping in two lines - Adding the right namespace - Fixing the call to a parent theme function that builds our pagination

Additional cleaning of author.php

11c8a07

- Removing extra empty lines - Removing a non-snake-case variable (rCat) - Simplifying a printf command to remove extra concatenation - Removing an entire block that was confusingly commented out, which we don't use nor need.

Cleans category.php

4e0ceae

- File metadata - escape a PHP variable used in javascript To test this, look at the script tag on a category index page, and check the Category ID parameter.

Cleans content-single.php

171a0c3

- Namespace and file metadata - content escaping content-single tweak content-single.php

Cleans index.php

c416565

- file metadata - remove pageroot / isroot lines that aren't needed

Cleans search.php

ac1caf9

- Namespace and file metadata - content escaping

Clean searchform.php

a15cae6

Clean sidebar.php

ead82d4

- Namespace and file metadata

Cleaning single.php

35fb0ae

- Namespacing and file metadata - Fixing one call to DateTime - Escaping a number of lines as prompted by PHPCS

Cleans inc/bib-header.php

6892814

- Namespace and file metadata - content escaping - convert a reference to $_SERVER['SCRIPT_URI'] to a WP-specific approach that avoids server variables

cleans inc/catFooter.php

240ac54

- Namespace and file metadata - content escaping

Cleans in/entry.php

157d306

- File metadata - disable PHPCS around a function that is particularly hard to escape

Cleans inc/events.php

6e2e15a

- Namespace and file metadata - Properly namespaced call to DateTime - content escaping

Cleans inc/footer.php

8b90737

- Namespace and file metadata - removing an extra title attribute - content escaping

Cleans inc/header-opengraph.php

9c20252

- Namespace and file metadata

Cleans inc/image.php

18bbaac

- Namespace and file metadata

cleans inc/imageEvent.php

646377a

- Namespace and file metadata

Cleans inc/more-posts.php

9ac8d15

- Namespace and file metadata

Cleans inc/related.php

53796b9

- Namespace and file metadata - Content escaping - Removing extra whitespace

matt-bernhardt added 17 commits March 30, 2023 13:35

Cleans inc/social.php

cb18271

- Namespace and file metadata

Cleans inc/spotlights.php

f847981

- Namespace and file metadata - Content escaping - Replacing the long string of if statements with a more tightly-scoped switch statement that only sets the relevant value (the class value assigned to the div), which is easier to escape.

Cleans inc/sub-header.php

0d54053

- Namespace and file metadata - Disable PHPCS check for the echo on line 104, which is a regex-modified select tag(s)? - Collapse a handful of small php blocks into one coherent set of lines, trying to make this more readable.

Cleans inc/sub-headerSingle.php

3387146

- Namespace and file metadata

Cleans inc/subheader-exception.php

d6b355f

- Namespace and file metadata

Cleans inc/subtitle.php

6913c3c

- Namespace and file metadata

Cleans inc/subtitleEvents.php

5d39f28

- Namespace and file metadata

Cleans inc/title.php

3d2237d

- Namespace and file metadata

Cleans lib/cards.php

c3377f9

- Content escaping on two lines - Ignoring rule on two other lines which echo a blob of markup. Would need wp_kses() to do properly

Clean templates/additionalPosts-archives.php

47fd64f

- Namespace and file metadata - Better positioned template name - Content escaping - Better input validation / sanitization for limit and offset

Cleans templates/additionalPosts-biblio.php

a02e2b8

- Namespace, template name, and file metadata - Better approach to input validation

Cleans templates/additionalPosts-cat.php

69ecb6c

- Namespace, tempalte name, and file metadata - Better input validation - Renaming $categoryId to $category_id since this touches most of its lines already

Cleans templates/additionalPosts-events.php

fa9387b

- Namespace, template name, and file metadata - Better input validation for $offset and $limit

Cleans templates/additionalPosts-news.php

5d1b8e8

- Namespace, template name, and file metadata - Better input validation for $offset and $limit

Cleans templates/additionalPosts.php

2e40d49

- Namespace, template name, and file metadata - Better input validation for $offset and $limit

Non-pagination templates

0caf5cc

matt-bernhardt force-pushed the lm143 branch from 7493e67 to 0caf5cc Compare March 30, 2023 17:37

matt-bernhardt requested a review from JPrevost March 30, 2023 18:56

JPrevost approved these changes Mar 31, 2023

View reviewed changes

matt-bernhardt merged commit 2da2d92 into master Mar 31, 2023

matt-bernhardt deleted the lm143 branch March 31, 2023 15:37

matt-bernhardt mentioned this pull request Mar 31, 2023

Implement news theme templates #71

Closed

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

News theme template cleanup #78

News theme template cleanup #78

Uh oh!

matt-bernhardt commented Mar 28, 2023 •

edited by JPrevost

Loading

Uh oh!

JPrevost left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

News theme template cleanup #78

News theme template cleanup #78

Uh oh!

Conversation

matt-bernhardt commented Mar 28, 2023 • edited by JPrevost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Developer

Secrets

Documentation

Accessibility

Stakeholder approval

Dependencies

Code Reviewer

Uh oh!

JPrevost left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

matt-bernhardt commented Mar 28, 2023 •

edited by JPrevost

Loading