search

WPTT / WPThemeReview

PHP_CodeSniffer rules (sniffs) to enforce WordPress theme review coding conventions
MIT License
208 stars 38 forks source link

Feature/templates/required #214

Open timelsass opened 5 years ago

timelsass commented 5 years ago

Adds sniffs for language_attributes() and body_class() usage.

language_attributes() requirement in: https://make.wordpress.org/themes/handbook/review/required/#code body_class() requirement in: https://make.wordpress.org/themes/handbook/review/required/#templates

I think we should probably change the organization of the handbook, and add language_attributes() to the Templates section to stay in line with the sniff, plus is a requirement of "templates".

I know it's not 100% perfect as there can be some weird syntax and ways around with the mixes of various output methods in PHP + html, but the test cases were mostly the top common scenarios I saw, and have caught a couple of issues that made it past reviewers.

Example: https://themes.trac.wordpress.org/browser/nirmala/1.3.0/page-templates/blank.php#L23

dingo-d commented 5 years ago

Hmmm, I haven't thought of Templates folder. I could place Reserved Template Name Sniff (PR #213) in there then...

jrfnl commented 5 years ago

@timelsass Welcome! I'll try and review your sniff over the next few days, but for now, I just want to leave some comments for you to think about.

I made a very limited start on a sniff for this way back (and got distracted and never got back to it), so I'm very happy to see this being addressed now :smile:

All the same, I did set up some unit tests at the time. I haven't checked yet whether your unit tests already cover these cases, nor whether the sniff would handle them correctly, but they may be helpful to you to fine-tune the sniff:

<!-- Correct header example. -->
<html <?php language_attributes(); ?>> <!-- OK. -->
<head>
<meta charset="<?php bloginfo( 'charset' ); ?>"><!-- OK. -->
<meta name="description" content="something" />
<meta http-equiv="Content-Type" content="text/html; charset=<?php bloginfo( 'charset' ); ?>" /><!-- OK. -->
<?php wp_head(); ?>
</head>
<body <?php body_class(); ?>><!-- OK. -->

<!-- Correct header example: multi-line calls and such. -->
<html 
    <?php language_attributes(); ?>> <!-- OK. -->
<head>
<meta
    charset="<?php
        bloginfo( 'charset' );
    ?>"><!-- OK. -->
<meta name="description" content="something" />
<meta
    http-equiv="Content-Type"
    content="text/html; charset=<?php bloginfo( 'charset' ); ?>"
/><!-- OK. -->
<?php wp_head(); ?>
</head>
<body

    <?php
        body_class();
    ?>
><!-- OK. -->

<!-- Correct header example: bit convoluted code, but the output would be correct. -->
<html <?= echo get_language_attributes( 'html' ); ?>><!-- OK. -->
<head>
<meta charset="<?= get_bloginfo( 'charset' ); ?>"><!-- OK. -->
<meta name="viewport" content="width=device-width, initial-scale=1">
<link rel="profile" href="http://gmpg.org/xfn/11">

<?php wp_head(); ?>
</head>

<body class="<?php echo implode( ' ', apply_filters( 'theme-body-class', get_body_class( 'additional-class' ), $pageID ) ); ?>"><!-- OK. -->

<!-- Incorrect header: missing function calls. -->
<html> <!-- Bad. -->
<head>
<?php wp_head(); ?>
</head>
<body><!-- Bad. -->

<!-- Incorrect header: attributes filled in. -->
<html lang="en-US"> <!-- Bad. -->
<head>
<meta charset="UTF-8"><!-- Bad. -->
<?php wp_head(); ?>
</head>
<body class="home paged"><!-- Bad. -->
timelsass commented 5 years ago

thanks for looking over this @dingo-d and @jrfnl, yeah I started working on meta with charset, but it was a slightly different since it's setting only the property and not the attribute + value(s) on the tag, so I was going to save that for a little later. While doing that though - I saw some common stuff that might be able to be abstracted out, but haven't given much thought as to the best way of doing that still.

Yes, I thought about the naming and wasn't sure what to do for it :smile: I just kinda went with what felt okay at the time, but I'm open to any suggestions. I know @dingo-d had talked with me about some naming for the readme file sniffs in WPTRT/theme-sniffer - and also mentioned moving his sniff into Templates, so maybe he can chime in on naming convention here if he's got some thoughts.

I was going based off of trying to keep some kind of convention close to: https://make.wordpress.org/themes/handbook/review/required/#templates - but couldn't figure out a good one. These were some of the ones that I was thinking about, but didn't feel like they really fit quite right for one reason or another:

The first couple sounded more like the tag in the header in context of WordPress theme templates, so I kinda ruled those out - but "TemplateTags" rolls off the tongue nicely. I ended up just using RequiredFunctions since that's what I used when I started, they were functions that are required, and seemed pretty generic that no one would say anything :rofl:. I'm not happy with the naming there though, and was hoping someone might have better ideas. Especially as Templates becomes more populated with the other functions this doesn't cover.

I'll run some tests against what you provided, and update accordingly. The ones in particular are the get_ functions, and I did think about those. I was hoping to get some thoughts on the get_ functions like the - echo get_language_attributes(); and echo implode( ' ', apply_filters( 'theme-body-class', get_body_class( 'additional-class' ), $pageID ) ); examples above.

My thought process was basically along the lines of "use the right functions where they should be used." In that case I believe those situations are still caught correctly. For example, I don't think doing echo get_language_attributes() is necessarily appropriate when a core method exists for echoing the language attributes using the language_attributes() function.

Likewise - I understand why a theme author might do something like what's done in the body classes example, but I don't think that it is code style that should be considered appropriate to pass the sniff. Adding a filter with the theme's prefix, then calling get_body_class and echoing out seems a bit redundant when body_class() already echos out, and offers a filter. I think the sniffs should encourage the usage of the appropriate methods over considering variations that aren't optimal as being acceptable. If an author wants to provide a filter on body_class that should go inside of a function they are using to filter it. The guidelines do specifically say they must use body_class as well, so that was kinda the main deciding factor for me when working it out.

^^ Those are just my own thoughts, but if everyone else thinks these should be acceptable, I can work on adding those in. There's really not any technical reason why those aren't acceptable ways of handling it from what I can tell since they still get filtered in the get_body_class(). Doing some scans on https://wpdirectory.net I saw themes using get_body_class(), but from all the ones I looked at it seemed they were all using that in functions within the theme and still calling body_class() when outputting in a template. I didn't look at all the results though, so I could be wrong and missed something.

On the flip side though, the benefit of adding them in would be that the sniff is expanded out a bit more to cover the functions that only set the values and not just for the attribute + value in the tag. This pretty much brings me back around to why I didn't include the meta charset in this particular sniff in the first place lol.

When adding the meta charset I would've completely forgotten about setting it through the content attr with content="text/html; charset=utf-8" as I'm used to that being the "old" syntax, so thank you for sharing this, because that was my next "thing" I wanted to work on, along with some of the other template specific/placement specific ones like wp_footer and the new wp_body_open.

dingo-d commented 5 years ago

I'm fine leaving this in Templates category. I even moved the reserved template name sniff to this category in the PR 🙂 :+1:

As for the name, I was also worried a bit about the naming, since it's open to additions, changes and ambiguity. Changing the handbook is not a problem, especially if we want to keep the consistency up a bit.

RequiredTemplateFunctions maybe for the name?

Could we then check for the rest of the required functions mentioned in the handbook?

wp_head() – (immediately before </head>).
body_class() – (inside <body> tag).
$content_width.
post_class().
wp_link_pages().
the_comments_navigation(), the_comments_pagination().
the_posts_pagination(), the_posts_navigation().
wp_footer() – (immediately before </body>).

Or would this be too much?

dingo-d commented 5 years ago

@timelsass did you have the time to look over the review? If you want, I could pull your changes and see if I can help with this sniff 🙂

dingo-d commented 5 years ago

I've changed the milestone to 0.2.x. Once you'll have more time I'd love for you to finish the sniff. If you need any help just let me or Juliette know and we'll help out any way we can 🙂

dingo-d commented 3 years ago

Hi, @timelsass can you just let us know if you wish to continue working on this sniff. If yes, can you rebase so that the checks on GH actions will run?