2.0 - 500 Error after today's update

[makitso]

This is strange. Everything works find on my dev server but a 500 error on my public site. I rsync'ed everything and can see no differences.

[Mon Oct 08 07:06:30.253201 2018] [fcgid:warn] [pid 7179:tid 3793759040] [host www.skatekey.net] [client 67.128.192.90:55894] PHP Fatal error: Declaration of Fisharebest\Webtrees\Module\FamilyTreeNewsModule::construct($directory) must be compatible with Fisharebest\Webtrees\Module\ModuleInterface::construct(string $directory) in /var/www/html/sk/skatekey.net/app/Module/FamilyTreeNewsModule.php on line 32

[ric2016]

I don't think it's a good idea to have the constructor public function __construct(string $directory);

in the interface declaration of ModuleInterface.php - This prevents implementing modules from using other constructors (I just came across this when adjusting my custom modules to the current 2.x version). What is the purpose of having this in the interface declaration? See also e.g. here

[fisharebest]

I don't think it's a good idea to have the constructor

I agree.

What is the purpose of having this in the interface declaration?

If I remember correctly, there was a problem when you had two modules with the same name.

This could happen if someone copied a module from modules_v3/my_module to modules_v3/my_module.backup.

By using the module's folder name, we got round this problem.

Probably not the best solution, but a quick solution was required at the time.

[ric2016]

I see - but having this in the interface doesn't really enforce a unique name, a module could just use a constant string here. Or for that matter return something else in

public function getName(): string;

Anyway, as long as a module extends AbstractModule, and uses its actual dynamic directory when calling AbstractModule's constructor, and doesn't override getName, shouldn't everything work just as well without this in the interface?

[fisharebest]

I am not trying to justify this code - just explain it.

The module system was partially written for PhpGedView (but not implemented). We quickly completed it to create a module system for webtrees.

The module code is very old, and needs rewriting.

[ric2016]

I'm just suggesting a modification of ModuleInterface - this is a new class that didn't exist in 1.7 so parts of the module code are already being rewritten.

[fisharebest]

I'd like to create a system that didn't break when the webtrees core is upgraded.

It is OK if the module does not work when the core is upgraded. But it is a problem if there is a fatal PHP error, as we cannot catch this and everything stops working.

I was thinking that we have pairs of trait and interface. (Laravel uses this approach).

If a module creates a chart, then it will do two things:

1) implement ChartInterface 2) Use ChartTrait

If we only add new methods functions (and not change existing methods) then we should be able to make changes, add new methods, etc.

We could also add versions, eg. ChartV1Interface and ChartV1Trait.

Now that we are using PHP7, we can have anonymous classes.

This might be a good choice for modules, as we do not need to worry about duplicate class names, etc.

Perhaps create a new issue with ideas and suggests for a new module system. These comments (on a closed issue) will get lost...

[ric2016]

I didn't mean to make this about large structural changes, or a new module system. I can't seem to communicate my point, so I'll just give up here.

[fisharebest]

My point is that the large structural changes are needed for 2.0, so your proposed change will be part of that.

[ric2016]

Now I get it ;-) I was under the impression that 2.0 was basically finished, and that these changes would come in a later version.

[fisharebest]

That's why we are still alpha. Once the API/structure is final, we can go to beta.