processwire / processwire-requests

ProcessWire feature requests.
39 stars 0 forks source link

Should $modules->isInstalled return the module instead of TRUE? #518

Closed BernhardBaumrock closed 5 months ago

BernhardBaumrock commented 5 months ago

Short description of the enhancement

I'm often doing this:

if ($modules->isInstalled("FooModule")) {
  $foo = $modules->get("FooModule");
  $foo->doSomething();
}

And I'm wondering if we could change the API (or there is already a simpler API for it) to return the module instead of true:

$foo = $modules->isInstalled("FooModule");
if($foo) $foo->doSomething();

It's a small thing, I know :)

I've had a look into the code and see that you can do this:

$foo = $modules->getModule("FooModule", ['noInstall' => true]);

Personally I'd never use this as I'm sure I'd not remember it by heart and it's hard to type.

But I'd remember this for example:

$foo = $modules->get("FooModule", false);

Where the second param stands for install yes/no.

I think I'd prefer $foo = $modules->isInstalled("FooModule") but I'm not sure if changing this might have unwanted side-effects?

teppokoivula commented 5 months ago

This would be a breaking change: a lot of us are using strict comparisons, so this would break every bit of code relying on current return value.

Changing return values of public APIs should only happen when a new major release comes out (and preferably not even then, if it can be avoided).

I must admit that I don't like the idea anyway, feels that weird that asking the system if a module "isInstalled" would respond with a module; I can see how that might be useful in some cases for those familiar with this behaviour, but it's still rather unexpected :)

or there is already a simpler API for it

$foo = $modules->get('FooModule', ['noInstall' => true]); if ($foo) $foo->doSomething();

BernhardBaumrock commented 5 months ago

Thx @teppokoivula I agree. So what do you think about $modules->get("FooModule", false) ?

I have in my mind that there is already at least one method in the api that works like I describe. I know that there has been some discussion about that, but I cant remember what it was :) Maybe Adrian knows it?

I'm closing it as this was more a discussion and priority 5000