processwire / processwire-issues

ProcessWire issue reports.
45 stars 2 forks source link

Modules extending non-core base classes #1178

Open teppokoivula opened 4 years ago

teppokoivula commented 4 years ago

Short description of the issue

This feels like a borderline feature request or maybe even a topic to discuss on the forum, so feel free to move it (or let me know if I should close and open a feature request issue / forum topic instead), but here's what I've been trying to do:

This setup seemed to work fine at first, but then I did a modules refresh... and BOOM! Fatal error. The problem, it would seem, is that during modules refresh ProcessWire loads the child module's class file, and since at that point PHP can't find the base class I'm trying to extend, the whole thing comes crashing down.

I've been trying to figure out some way to do this, but so far no luck — I can't seem to find any way to tell ProcessWire that this module really depends on another one, and even the class file shouldn't be loaded before it. I declared in .info.json a "requires" dependency for another module that would in turn load the base class with PHP's require(), but that doesn't seem to help.

Is there any way to get past this, apart from just not defining custom base classes for modules? I mean... sure, the module could also become a "factory" that spits out a class that extends the base class, but I'd prefer to avoid such extra layer :)

Note: the only forum topic I could find that seemed to touch the same problem was from way back (2017), and had zero answers: https://processwire.com/talk/topic/16959-extending-modules-issue-after-28-upgrade/.

Expected behavior

It would be great to be able to extend other modules or custom base classes in new ProcessWire modules. In fact I would've expected "requires" to give ProcessWire a signal that the required module needs to be loaded first, but in this case it doesn't — or, rather, this module probably wouldn't be instantiated before its dependency, but it seems that this doesn't prevent the class file itself getting loaded.

Actual behavior

Failure, as described earlier.

Optional: Suggestion for a possible fix

I'm really not sure. Maybe ProcessWire could avoid loading the class file (right away) in case .info.json is in place and states a requirement for another module, unless there's some other downside to that approach?

Or perhaps this would require a separate config setting, something along the lines of 'requires-loaded' (working title), so that Modules.php would know that it needs to load those module classes first. Not sure how much sense that would make, of course, considering that this doesn't seem like a particularly popular request.

... or maybe there's something really obvious I've missed :)

BernhardBaumrock commented 4 years ago

Hi @teppokoivula could you explain the setup with sample files and explain again what module gets loaded when and where etc? :)

<?php namespace ProcessWire;
class Foo extends ... implements ...
getModuleInfo() {
  autoload = true
}
<?php namespace MyModule;
class Bar extends ... implements ...
  autoload = false
Toutouwai commented 4 years ago

Try using a number in the autoload item of getModuleInfo().

See how TracyDebugger appears at the top of the list of module files which are otherwise sorted alphabetically (and I think this determines the load order): 2020-05-26_154743

Tracy uses an autoload value of 9999 because it's important that it be loaded first whenever possible. But you could use any number greater than 1 for the base module. See how UniqueImageVariations jumps up to second place when I change 'autoload' => true to 'autoload' => 2 2020-05-26_155814

teppokoivula commented 4 years ago

Hi @teppokoivula could you explain the setup with sample files and explain again what module gets loaded when and where etc? :)

Sure thing, @BernhardBaumrock :)

<?php namespace ProcessWire;
class ChildModule extends BaseClass implements Module {
    ...
}

In my case BaseClass is actually not an installed module, but rather a class that gets included by another module's class (as soon as it's loaded, i.e. even before the class declaration itself.)

And that's it. There's no autoload involved — ChildModule is not autoloaded, and that had no effect on this issue (I did try it as well, just in case). In this case the problem (loading the class file that extends a non-core class) occurs before the whole autoloading process has a chance to kick in.

Try using a number in the autoload item of getModuleInfo().

There's no autoload involved here — but I did try this, and it didn't make any difference.

This issue not related to ProcessWire's autoload process per se, but to the way ProcessWire includes the class files to pull information from them (and admittedly I'm not entirely familiar with the specifics of that).

As with Tom's old support forum topic, I'm seeing something pretty odd here, though: if I extend a module that has already been "found" (cached), this works just fine. The problem occurs if neither module is known to ProcessWire, although yesterday I couldn't get it to work consistently either way. So the behaviour appears to be a little inconsistent.

I'm hopeful that there's still some way around this :)

Based on some tests it seems that one way to reproduce the problem consistently would be 1) creating modules A and B, neither of which have yet to be introduced to ProcessWire, and 2) making one of them extend the other. First module refresh should trigger the error, yet if the refresh is first called once when just the module that extends a core class is in the system, and only then you add the second one (that extends the first one) and call refresh, it's all OK.

Might have to do further testing :D

BernhardBaumrock commented 4 years ago
<?php namespace ProcessWire;
require_once('BaseClass.php');
class ChildModule extends BaseClass implements Module {
    ...
}

This is not an option?

teppokoivula commented 4 years ago

@BernhardBaumrock, in my initial post I mentioned one reason why this would be less than optimal:

It also belongs under another module, so I can't just load the file in my "child module" before declaring the class, since I can't be (entirely) sure where that other module is installed.

Another problem (though this is a little opinionated) is that this would be "surprising". The idea is that developing child modules using particular "custom module type" would be very simple, but requiring a separate class file is something that no module developer would expect.

... and, also, in my opinion including the ChildModule class in the system (without the "parent" module) and hitting "refresh" for modules should not break the admin until the module file is manually removed. This wouldn't resolve that part.

Edit: Bernhard's suggestion brings one idea to mind. If I lowered my expectations regarding the ease of development a bit, I could add instructions to require the base class with Composer. That could be better than manually copying it from elsewhere or expect the parent module to be found from the expected/default directory. Not a perfect solution, but could be an option.

ryancramerdesign commented 4 years ago

@teppokoivula What you are describing is what the getModuleInfo "requires" property is supposed to do. Having the external info file is also key to making that work, but sounds like you already did that. I tried to duplicate the issue here but can't seem to. Here's what I did to test. Please let me know if I'm doing something different than you did.

First I setup my base module class, which I called TestBase:

/site/modules/TestBase/TestBase.info.php

<?php namespace ProcessWire;
$info = [ 
  'title' => 'Test Base', 
  'summary' => 'Test Base Summary', 
  'version' => 1 
];

/site/modules/TestBase/TestBase.module

<?php namespace ProcessWire;
class TestBase extends WireData implements Module {
  function hello() { return $this->className(); }
}

Next I setup my extending module, called TestHello:

/site/modules/TestHello/TestHello.info.php

<?php namespace ProcessWire;
$info = [ 
  'title' => 'Test Hello', 
  'summary' => 'Test Hello Summary', 
  'version' => 1,
  'requires' => 'TestBase',
];

/site/modules/TestHello/TestHello.module

<?php namespace ProcessWire;
class TestHello extends TestBase implements Module {
  // intentionally blank
}

Then I did a Modules Refresh in the admin, clicked Install for TestBase, and then Install for TestHello. And used this code below to test:

echo $modules->get('TestHello')->hello(); // Outputs: TestHello
echo $modules->get('TestBase')->hello(); // Outputs: TestBase
matjazpotocnik commented 1 year ago

@teppokoivula any news about this issue? Can we close it?

teppokoivula commented 1 year ago

Thanks for the heads-up, I'll get back to this one ASAP.