Resolve PHPStan issues about tablepress

[TobiasBg]

Hi @szepeviktor,

cool, thanks a lot for all these suggestions! :-)

That class_alias() thing really cost me some nerves when I first added that. I had checked so many things, but must have missed that @var TablePress_*_Controller line... I have now made that more precise in fbe75c5 so that the class_alias() is no longer needed :-)
(That $controller class property could have other types in previous versions of TablePress, but since TablePress 2.0, it's only TablePress_Frontend_Controller.)

Your other suggestions are also very intriguing, but they will need careful review and a lot of testing.

For that class-string idea with load_class(), that @template would probably need to be made more complex, as load_class() does not just load TablePress_Controllers.

And I totally agree that autoloading would be very cool to have in the future, but I'm not sure if that can be changed that easily, without breaking backward-compatibility. For example, the load_controller(), load_class(), load_file(), etc. methods all have WordPress filter hooks that are used in e.g. the premium version, TablePress Extensions, and other plugins. So, this would need careful investigation.

Could you maybe expand on that load_admin_page_* suggestion a little bit? I'm not sure that I fully understand where you want to change what here. While that long switch/case is indeed a bit ugly, its advantage is that the $data can then be filtered (which is also done in various plugins).

Thanks a lot for your great contributions here!

Best wishes,
Tobias

[TobiasBg]

Ah, now I see what you mean. This approach will only work if TablePress is displayed in a top-level menu entry (which is not always the case, it can be moved e.g. via the setting on the "Plugin Options" screen). Also, there are views that don't correspond to an admin menu entry, which would need separate handling.
But I understand the general idea now, and yes, that all looks very reasonable! I'll investigate that more closely! :-)

[szepeviktor]

PHP generics comes to the rescue 🚨
And there is a special type in PHPStan: class-string

/**
 * @template TClass of TablePress_Controller
 * @phpstan-param class-string<TClass> $class_name
 * @phpstan-return TClass
 */
public static function load_class(

[szepeviktor]

Please consider using spl_autoload_register instead of the load_class method.
Then you could instantiate a class anywhere in the code.

$view = new TablePress_Export_View();
$view->do_anything();

an example

I would use Composer's "classmap" autoloader.

{
    "autoload": {
        "classmap": ["classes/", "controllers/", "models/",  "views/"]
    }
}

composer dump-autoload --optimize

Later on you could leave legacy technologies behind and keep all classes in PSR-4 format in src/ and tame libraries with https://github.com/BrianHenryIE/strauss

[szepeviktor]

I would have as many load_admin_page_* methods as there are views.
Then you wouldn't need that array in init_view_actions and the long switch/case in load_admin_page.

[szepeviktor]

Could you maybe expand on that load_admin_page_* suggestion a little bit?

I would have a load_admin_page_list etc. method for each $action. One can argues that it will cause code duplications but explicit is better than implicit, and repeated things can be abstracted to new methods.
Each one would have an apply_filters() call.
That is all.

[TobiasBg]

Ah, then then call these dynamically (e.g. with call_user_func()) inside the existing load_admin_page()?
(That function would still be needed, as there's just one add_action( "load-{$page_hook}", array( $this, 'load_admin_page' ) ); where the load_admin_page_*() functions could hook into.)
Yes, this could indeed be a nice way to get rid of that switch in favor of individual functions, and their output could still be passed through the tablepress_view_data filter hook.

[szepeviktor]

there's just one add_action( "load-{$page_hook}", array( $this, 'load_admin_page' ) );

Have many of this also! With explicit values. It will be so nice.

[szepeviktor]

It will be hard. WordPress has no routing/views/template engine.

(many agencies fill these holes before they start a new project)

[szepeviktor]

will only work if TablePress is displayed in a top-level menu entry

You could insert a layer that takes this dynamic list of menu entries and calls concrete methods.
And add a comment (from WordPress core)

// Pretend you didn't see this

[TobiasBg]

😂 Yes, that part of the WordPress code is a nightmare ;-)

Having a layered approach will indeed be the most reasonable approach here.

All this with controllers/views was my idea of a "template engine" as you mention, but well, it's like 12 years old or so, so not "modern" or anything like that anymore :-)

[szepeviktor]

Even a dumb mustache-like template engine would make WordPress much-much better.
Support for template variables/if/for is enough.
(I am a Timber contributor)

ps. PHP started as a template engine, in 7.0 it changed direction to be a Java-like programming language