[mdalikadar]

Description
Filters::discoverFilters() ran FileLocator::getClassname() on every Config/Filters.php it found, which reads and tokenizes the file, and then skipped the app and system ones because their class is Config\Filters or CodeIgniter\Config\Filters. In a default app those are the only two files, so every request parsed two files it never used.

The two paths are now skipped by path, and the class-name check is removed, so no discovered file is tokenized anymore. The paths are built with DIRECTORY_SEPARATOR, as APPPATH, SYSTEMPATH and the realpath() results from search() all use real paths.

On my machine new Filters() with discovery on goes from about 134 µs to about 60 µs (PHP 8.5 CLI, so indicative). Apps with extra module Config/Filters.php files save a further ~16 µs per file. It mostly helps apps without FileLocatorCached.

Edge cases:

• An app/Config/Filters.php that declares a class other than Config\Filters used to be included by discovery and is now skipped.
• Any other discovered file that declares Config\Filters or CodeIgniter\Config\Filters used to be skipped by the class check. It is now included, so it would fail as a redeclaration.

Both break the framework's conventions, and discovery has been deprecated since 4.4.2.

Fixes #10616

Checklist:

• Securely signed commits
• Component(s) with PHPDoc blocks, only if necessary or adds value (without duplication)
• Unit testing, with >80% coverage
• User guide updated
• Conforms to style guide

[neznaika0]

See next lines:

            // Don't include our main Filter config again...
            if ($className === FiltersConfig::class || $className === BaseFiltersConfig::class) {
                continue;
            }

This is almost done before you.

[mdalikadar]

Thanks for the review. The check you quoted does already exist, but it runs after $locator->getClassname($file), and that call is what reads and tokenizes the file. So today both default Config/Filters.php files are tokenized on every request, and only then skipped.

This PR moves the skip ahead of getClassname(), matching by path, so those two tokenizations no longer happen. The class-name check is kept unchanged for every other file.

testDiscoveryDoesNotTokenizeTheDefaultFiltersConfigs covers this: it records the files passed to getClassname() and asserts the app and system configs are not among them. I checked that it fails against the current 4.8 code (app/Config/Filters.php is passed to getClassname()) and passes with this change.

I can add a comment next to the existing check making the ordering clearer if that would help.

[neznaika0]

I suggest removing the class validation. It's the same thing. This will give you more speed

[mdalikadar]

Good point, thanks. Pushed.

The class check is redundant once the two default files are skipped by path: APPPATH and SYSTEMPATH are realpath()'d, as are the results of search(), so the path match can't miss. I removed the check, so discovery now only skips the app and system configs by path and includes everything else.

On my machine this saves about 16 µs per extra module Config/Filters.php (57 µs → 41 µs for three discovered files), with no change for a default app. The test now asserts that no discovered file is tokenized. I also updated the PR description for the one behavior change: any other file declaring Config\Filters is no longer skipped, so it would fail as a redeclaration instead of being ignored.

[neznaika0]

Let’s take a look at the history #3459.

This detection was made so that users wouldn’t create their own classes in */Config/Filters.php (in modules). Instead of a class, you could use as global var:

<?php

$filters->aliases[] = ...;
$filters->required['before'] = ...;
$filters->globals = ...;
...

There are no tests. I consider this 100% outdated and it should be removed. Creating a Registrar seems like the right way.

The funny thing is that checking for the presence of NS $className = $locator->getClassname($file); isn’t necessary if we insert a variable.

Sorry, I didn’t figure it out right away.

[mdalikadar]

Thanks for the history link, that explains why the check exists. I already removed the class-name check in 4c1c369 (no discovered file is tokenized now), so I'm not sure which part is still open. Two questions:

1. Is the remaining change you want to drop the path skip as well? app/Config/Filters.php and system/Config/Filters.php are class files that autoloading already loads, so including them again from discovery would be a redeclaration, which is why I skip them by path.
2. Should replacing discovery with a Registrar happen here, or in a separate PR? That looks like a bigger change than this perf fix.

Could you clarify what you mean by "if we insert a variable"?

[neznaika0]

Could you clarify what you mean by "if we insert a variable"?

This code is a technical debt. Instead of using a class, it relies on a variable $filters. If we want to change the filters as it is now in the method, then there’s no point in using a class, because it won’t be used.

The second bug is that if we configure $filters and add Registrar, the values will be overwritten. This is the second reason to remove the deprecation.

This PR can be reworked to close the task.
Wait for the team members; I have less influence on the decisions here.

[mdalikadar]

Thanks, that makes sense: a discovered file that only edits $filters gains nothing from a class, and the Registrar overwrite problem is a good reason on top of that.

I'm happy to rework this PR to remove discoverFilters() (and the filters discovery option, its tests and the support file) instead of speeding it up. That is a breaking removal for apps whose modules still ship a Config/Filters.php that edits $filters, so it would need a v4.8.0 changelog entry and an upgrading note pointing them to a Registrar. Deprecated since 4.4.2, so I believe it is within the policy for 4.8.

@michalsn @lonnieezell, do you want that, or should I keep this PR as the small path-skip optimization?

[michalsn]

Sorry, but there's no point in optimizing deprecated methods. This particular one may be removed in 4.8 or even before its release.

[mdalikadar]

Understood, thanks for the context. I'll close this, since there's no point optimizing a deprecated method. And thanks @neznaika0 for the history and the review.

If you'd like discovery removed in 4.8 and want a hand with it (code, tests, changelog and upgrading note pointing module authors to a Registrar), let me know and I'll open a separate PR for that.