bigbite / phpcs-config

Big Bite's PHP Coding Standards.
MIT License
2 stars 2 forks source link

index.php files being ignored #17

Closed g-elwell closed 2 years ago

g-elwell commented 2 years ago

I believe the following rule is designed to prevent WordPress' index.php file from being sniffed, but it also applies to any other files named index.php within your project: https://github.com/bigbite/phpcs-config/blob/c18b0f4df07bdef3ae622286ab1820d1b14a0911/BigBite/ruleset.xml#L28

This has been particularly noticable for me as I've been following a subdirectory pattern for creating custom gutenberg blocks:

my-block
 - block.json
 - index.php

I'm not sure if there are other factors to consider, but thought I'd point this out as it might be worth considering adjusting/removing that specific rule if the intention is only to ignore the main WordPress index.php file.

jaymcp commented 2 years ago

In the context of WordPress, index.php is usually (barring the main WP entry point) reserved for a silence message, to prevent misconfigured servers from revealing their directory indices:

<?php
// silence is golden
jaymcp commented 2 years ago

Having said that, I would personally prefer file exclusions to be the responsibility of the project, not the ruleset. As someone who's already using this package on a live project, what are your thoughts?

g-elwell commented 2 years ago

Your comment on the silence message makes sense... I took inspiration for the above subdirectory pattern from Gutenberg, but see that WordPress core takes a different structural approach that avoids the use of files named index.php.

I think there's potentially a separate issue there around whether index.php usage should be warned against? At the moment you can name a file index.php without realising you're going against the convention, and it'll just silently not be sniffed or formatted.

At the same time, I don't think that the silence message files will break any rules, so including them in PHPCS may not cause an issue?

I'm all for having as little config in the project as possible to be honest, it's so easy to get up and running using this package and most of the built-in file exclusions are unlikely to cause any problems.

jaymcp commented 2 years ago

whether index.php usage should be warned against?

it would be straightforward enough to create a Sniff for that, but i'm not sure whether we should. although it is convention to use index.php as a safety mechanism, we probably should care a lot less about it now than we used to have to. the hosts we use disable directory traversal, and the likelihood of us running a site on a host that has that vulnerability (because directory traversal is a vulnerability, imo) is slim.

I don't think that the silence message files will break any rules

i've just run a scan against wordpress/index.php using our standard… it breaks no rules, so wouldn't cause any issues if we removed this specific exclusion from the ruleset. it very much used to, but that was a long time ago. any silence message files that broke the rules would be in userland, so should probably be reported on anyway, even if it's basically meaningless in this case.

I'm all for having as little config in the project as possible

i can totally understand that. it is nice having a project-level phpcs file that's just a reference to a ruleset.

the main challenge is that we can't assume the location of /index.php, because it's dependent on how the project is structured in relation to ABSPATH (this is potentially more of a concern when backporting than it is for futureproofing). however, since WP's root index.php file doesn't break any rules in our ruleset, the straightforward solution might be to just remove the exclusion in the ruleset.

jaymcp commented 2 years ago

index.php exclusion removed in v1.1.1. thanks for chatting through this with me!

g-elwell commented 2 years ago

index.php exclusion removed in v1.1.1. thanks for chatting through this with me!

No problem... thanks for taking the time to look into it!