File name with reserved slug in subfolder

[ernilambar]

Follow up of https://github.com/WPTRT/WPThemeReview/issues/212 (Check if page templates are using reserved prefix)

This check generates error for file in subfolder also, like template-parts/page-header.php. When such file is within the subfolder other than root, there wont be any effect in the hierarchy. It would be great if can apply this check in root folder only.

[dingo-d]

This is odd, as there is a list of allowed folders in the ruleset

https://github.com/WPTRT/WPThemeReview/blob/8398440d097cbe9b02fc901e1bc1e42bb3d02266/WPThemeReview/ruleset.xml#L127-L140

So this should work out of the box if I'm not mistaken 🤔

Unless this needs to be re-set in the custom ruleset 🤷‍♂

Any info on this @jrfnl ?

[ernilambar]

Oh, I did not know we had such exclusion. template-parts/page-header.php was just an example. Actually this was reported in TwentyTwenty theme.

[ernilambar]

https://github.com/WordPress/twentytwenty/issues/111

[jrfnl]

So this should work out of the box if I'm not mistaken Unless this needs to be re-set in the custom ruleset

It should work out of the box.

Based on the remark by @joyously in https://github.com/WordPress/twentytwenty/issues/111#issuecomment-529242658, I wonder if all templates files in subdirectories should be excluded ?

Note: this may not be feasible to do as PHPCS does not natively know what the root directory of a project is (and the sniff(s) may not be run from the root).

[jrfnl]

Looks like the subdirectory in TwentyTwenty is called parts, i.e. not one of the whitelisted subdirectories. Adding something along the lines of the below to the custom ruleset should solve this:

 <rule ref="WPThemeReview.CoreFunctionality.PrefixAllGlobals"> 
    <properties> 
        <property name="allowed_folders" type="array" extend="true"> 
            <element value="parts"/> 
        </property> 
    </properties> 
</rule> 

[dingo-d]

I'll mention this to the twenty twenty team. Maybe they can just follow the standard practices and use template-parts instead.

[dingo-d]

I just noticed that it's not PrefixAllGlobals that is triggered, but ReservedTemplatePrefix sniff.

  1 | ERROR | [ ] Template files should not be slug specific. The
    |       |     file name used for this template will be
    |       |     interpreted by WP as page-header and only applied
    |       |     when a page with the slug "header" is loaded.

Should we add the same property to this sniff as well?

[jrfnl]

@dingo-d Sounds like a good idea, but beware of what I said in https://github.com/WPTRT/WPThemeReview/issues/233#issuecomment-529919790

[ernilambar]

I guess this can be closed.

[dingo-d]

Oh, I'd leave this open, because this is an issue with the sniff, we just need to be smart around how to attack this problem (given the comment that Juliette mentioned).

[dingo-d]

@jrfnl Could we use <exclude-pattern> with this sniff, so that if the file path is in one of the proposed patterns the sniff would be ignored (so that we don't care where the root of the project is)? Or should we just use some helper inside the sniff to check if the path of contains template-parts, templates, partials or page-templates?