get_files() grabs files with 'extension' anywhere in the name

[coofercat]

I renamed a PHP file in an extension I'm working on from test.php to test.php.disabled (thinking that it would stop it from being loaded). It turns out that the get_files() method in Pico still finds the file.

In the code, I found this (line 335):

if(!$ext || strstr($file, $ext)) $array_items[] = preg_replace("/\/\//si", "/", $file);

...I think it should be more like this:

if(!$ext || preg_match("/\.$ext$/", $file)) $array_items[] = preg_replace("/\/\//si", "/", $file);

In the latter case, it finds the .php files, but not the .php.disabled files. What's the consensus on this? Shall I produce a patch/pull request?

[mistergraphx]

if you rename : _test.php the plugin is not loaded, a plugin is loaded only when the class name and the file name are the same. in your case it's always false.

[coofercat]

Another use case (with a vanilla download of Pico): Create /content/new.md and put some content in it. Check on the site, and sure enough, there'll be a title in the navigation with a link to /new.

Now go and rename it to new.md.disabled. Check the site again, the link will still be there, but to /new.disabled, which will 404 if you click it.

Next, apply my little adjustment and try this again - now the file is (correctly) ignored and not present in the navigation.

As for plugin PHPs - I can't get your suggestion to work. If I create test.php and put some junk in it so that it crashes out the request if it's included. If I name it test.php, _test.php, _test.php.disabled, ilike.phpprogramming or pretty much anything else it's still loaded.

Or... am I doing something crazy?

[mistergraphx]

No you're not going crazy ;-)

the only difference is that you test if $ext is the last match, so this work in your use case.
if you simply rename using only .disable or .back without hte $ext it'll do the same job.

strstr() function just find if the $ext is found in the string $file : in your case it's always true. just change the .ext ;-) using .back or .disable.

replacing the strstr() by a preg_match() is using more memory and time, this function is used frequently in pico, i'm not shure it's really useful.
may using strpos() 'll be a memory gain in this case.

[coofercat]

It's starting to sound like we agree on the problem...? I don't think it should be necessary to remove the 'extension' from the middle of a filename to have it ignored though. Some text editors just add a ".bak" or similar to the end of the filename when they save out drafts, and those shouldn't be included in file scans.

I'm happy to differ about the implementation of a fix though (my change was quick, rather than high quality). Using strpos() looks like you'd end up doing something like:

if(!$ext || strpos($file, ".$ext") == (strlen($file) - strlen(".$ext"))) $array_items[] = preg_replace("/\/\//si", "/", $file);

Alternatively, I guess you could also do something like:

if(!$ext || substr(strrchr($file_name,'.'),1) == $ext) $array_items[] = preg_replace("/\/\//si", "/", $file);

Some looking around suggests that actually, the solution should be more like this (not tested it yet though):

if(!$ext || pathinfo($file, PATHINFO_EXTENSION) == $ext) $array_items[] = preg_replace("/\/\//si", "/", $file);

It would take some benchmarking to figure out which is the quickest and lowest memory, but I'd suggest the pathinfo() solution is the most "PHP", and so probably the fastest - all the others (my preg_match() included) are essentially recoding a built-in function, which probably isn't such a great idea.

[mistergraphx]

It's starting to sound like we agree on the problem ..

Yes, ;-)

Adding unit testing to Pico, could certainly be a great addition for discussing about benchmark and optimisation.

The path_info is certainly better, you're right.

i'm just wondering if adding the Symphony Finder component to do all the file system task, could not be a better solution, even to be used by our plugins.

[coofercat]

I just had a look at Symfony Finder - it looks pretty nice. Whether to use it or not, and indeed to implement some unit/performance testing looks more like a feature request rather than this little bug report though (as much as I agree with both sentiments).

When I get a chance I'll produce a PR for this specific issue, although I understand that it'll probably sit in the pull queue for quite a long time :-(

[PhrozenByte]

Fixed with #252