removed some RFI and dynamic parameter warnings from PHPCS #222
Conversation
|
I've clearly done something wrong but I am not able to see the details of the CI checks. |
|
@davearch Thanks for working on this. We have the main CI tool we use (ProboCI) configured to work with dev branches on the main az-digital/az_quickstart repo but not forks (there are some hard-coded references to the main repo in the FWIW, it actually looks like ProboCI got farther along with building your PR than I would have expected so maybe we could look into to tweaking our .probo.yaml config file to better support PRs from forks. |
@davearch I don't see any immediate issues - I think the issue with CI checks might be because our CI script may make some incorrect assumptions about where the branch lives (I think it currently expects that the branch lives in the az_quickstart repo). We may need to make a few tweaks to allow for it to operate correctly with a fork. I'll check this out today. |
|
To get around the update delay on packagist, we'd put in a fix a while back where the repository itself is set up as a direct vcs repository rather than using packagist. I think we might potentially be able to leverage this to support forks, if probo has access to the forked location. |
|
@tadean It'd be cool if we could get that working but I'm afraid the info/variables provided by ProboCI might not give us all the context needed (at least not enough to make it very easy): |
|
That's true |
|
Honestly I'm surprised ProboCI worked at all with this PR based on the experience we had early on when we first started using ProboCI. We explored the idea of supporting PRs from forks and ran into some pretty big issues mainly having to do with our ProboCI subscription being tied to the main repo and ProboCI refusing to even do anything on repos without an associated subscription. It's possible that either ProboCI has changed the way things work somewhat since then or that the ProboCI integration with GitHub works different than the integration with BitBucket so I'd definitely be open to exploring this fork PR workflow again. |
|
@jwmoore1 Suggested that it may be possible to use special code comments to basically whitelist certain lines in our code for certain PHPCS security checks. We discussed this in a meeting last week and agreed that we'd prefer to pursue that route instead of changing many of the affected lines of code. |
|
Hey @joeparsons thanks for the update! @jwmoore1 is the man! Let me know if there is anything else I can do to support. |
|
We decided to merge #239 instead. |
Description
The changes I made removed most but not all PHPCS warnings. The only warnings that were left over were 'possible RFI detected' by the use of
drupal_get_path()in the az_barrio.theme file and the theme-settings file and 'filesystem function with dynamic parameter' on the use offile_get_contents()in the QuickstartConfigInstaller.php file. However I would argue that in keeping with the spirit of the security measures that PHPCS is helping us mitigate, these warnings should be acceptable. For one, the code sniffer tool is by its own admission a generator of 'false positives'. Please refer to their README as well as this interesting issue conversation. Secondly, seeing as how a Remote File Inclusion attack relies on user input I would think that the simpledrupal_get_path() . '/includes/common.php';would not be at a high risk. And withfile_get_contents(), I simply could not figure out a way around it but to be fair it also relies on basically static input. The Drupal codebase itself is littered with this function also.Related Issue
#174How Has This Been Tested?
I ran lando phpcs and lando phpunit.
Types of changes
Checklist: