WordPress / plugin-check

A repository for the new Plugin Check plugin from the WordPress Performance and Plugins Team.
https://wordpress.org/plugins/plugin-check/
GNU General Public License v2.0
242 stars 49 forks source link

Localhost check does not show errors in multiple files #362

Closed ernilambar closed 2 months ago

ernilambar commented 9 months ago

I tested with keeping http://localhost:8888 in two PHP files. And in the check, error is shown for only file.

/includes/Checker/Checks/Localhost_Check.php

Line 48:

$file = self::file_preg_match( '#https?://(localhost|127.0.0.1)#', $php_files );

This seems to be happening because self::file_preg_match() only returns the first match.

swissspidy commented 8 months ago

This is not only an issue with the localhost check, no? Every check using self::file_preg_match() or self::file_str_contains() currently only return the first matched file. It would make more sense to address this holistically instead of only the localhost check.

On one hand, it might be desirable to return all found results, on the other hand it's reasonable to only return the first found result as otherwise it can be very repetitive (for example if you use a code obfuscation tool, Code_Obfuscation_Check would flag all your files). The downside is that once you fix the file, the error message pops up again for the next file, and so on.

Let's think about how best to address this. Some possibilities:

  1. Do nothing, leave as is
  2. Make file_preg_match() and file_str_contains() always return all files and then leave it up to the consumer to process all results or just the first one. Downside: very expensive, so doesn't really make sense.
  3. Introduce new methods like in #399, but look at which checks would really need this.
frantorres commented 3 months ago

I'd be up to have both as those might be useful for different checks. The files_preg_match done on #399 looks great and would be amazing to have maybe a files_preg_match_all ? that returns file + line + column