Should we refactor the main RockMigrations.module.php

[Ivan Gretsky]

Good day @bernhard!

I was testing @gebeer's PR (see this thread), and I was looking at the code of RockMigrations.module.php. It seems to me that it might be beneficial for supporting this long-time to split the code into submodules. I think that all the code that actually creates fields/templates/users... could be moved to a helper module(s). And the main one would deal with all the admin logic. This way it would be not only easier to add field-specific methods, but also possible to use those great methods in different types of workflows (including them).  I would love to see other Pro fileds' specific methods added. Maybe other commercial or just 3rd party modules too. Maybe there could be some plugin system for those?

[elabx]

Wouldn't it be enough to add methods to the class through hooks?? And have like: RockMigrationsProfieldsUtilities, instead of refactoring the main module? 

[Ivan Gretsky]

The main module is a mix of process stuff and helper functions for the main logic. And it is more than 3000 lines. So a little refactoring here seems reasonable. But adding stuff via hooks (or some other way) to implement the plugin architecture seems reasonable too.

 

[bernhard]

Hi Ivan,

I really appreciate your input and I'm very happy to see your interest in RM growing. Though I have some concerns about your suggestion. Not saying that are bad suggestions, I just try to be realistic.

First of all: I'm really, really happy with RM and how it works right now. There has been a lot of refactoring from RM1 to RM2 and that has brought a lot of improvements to the module. The API is in my opinion quite easy and stable to use and the concepts have proven to be working.

Second: It's a free module. I think it's one of the best and most powerful modules out there for PW and it can bring a HUGE value to your everyday work, especially if you work in a team or want or need more professional workflows with zero-downtime deployments on multiple environments etc.; I decided to release it for free, because I think this should be part of the core. It should be something that every developer can use to create great modules. To use it as a foundation for, eg quickly and easily create a blog module that creates all the necessary fields and templates that you get a one-click blog setup for ProcessWire. That's simply not possible to do with site profiles and it's simply a pain to do with the regular API.

Unfortunately that did not happen. Quite the contrary. People complained about it being too complicated (and didn't even use the module once). And no one ever used RM to create something useful for the community as far as I know. I'm not complaining - it is like it is, but that's the context for the question you are asking.

That said, back to topic: I've been talking with @gebeer about refactoring quite some time ago:

--bernhard--
In the long term, however, I thought it might make sense to split the API. Similar to how the wire core is split into its own classes.

$rockmigration->fields->create('foofield');
$rockmigration->templates->addField('foofield', 'bartemplate', afterfield: 'title');
$rockmigration->migrate([ ... ]);
That would really be a completely different API... That means a mega breaking change. I don't know if it's worth it......

--gebeer--
In the long run it's not wrong because it brings more structure into the API. But I think the API is well usable as it is.

But I'm still not sure if it would be better. I really like the simple API that is accessible via $rm:

Also splitting the API brings in other problems. Many methods do something with two things, eg "addFieldToTemplate" - would that be $rm->fields->addToTemplate("myfield", "mytemplate") or would that be $rm->templates->addField("mytemplate", "myfield") ?

16 hours ago, elabx said:

Wouldn't it be enough to add methods to the class through hooks?? And have like: RockMigrationsProfieldsUtilities, instead of refactoring the main module? 

I don't understand 🙂 

4 hours ago, Ivan Gretsky said:

The main module is a mix of process stuff and helper functions for the main logic. And it is more than 3000 lines. So a little refactoring here seems reasonable. But adding stuff via hooks (or some other way) to implement the plugin architecture seems reasonable too.

If you have specific ideas I'd be more than happy to hear them and think about how we can improve the module and/or the code quality.

[elabx]

4 hours ago, bernhard said:

I don't understand 🙂 

Like this:  https://processwire.com/docs/modules/hooks/#how-can-i-add-a-new-method-via-a-hook

I just feel it might make sense for it to be decoupled like because @bernhard might not use some modules and would probably be better if  the "migration utilities" of modules he doesn't use are maintained on the side. This comment was targeted more at a thought of a "plugin system".

[bernhard]

20 hours ago, elabx said:

RockMigrationsProfieldsUtilities

Ah, I always read ProfileUtilities instead of ProfieldsUtilities 😄 Well anybody could just develop his/her own module based on RM

Just create an autoload module that requires RockMigrations, then in the init() method one could do this:

public function init() {
  $this->wire->rockmigrations->fooPlugin = $this;
}

And then anybody could use that plugin like this:

$rockmigrations->fooPlugin->doSomething();

But I'm not sure how useful that would be compared to calling the module directly:

$foo = $modules->get('fooPlugin');
$foo->doSomthing();

 

20 minutes ago, elabx said:

Like this:  https://processwire.com/docs/modules/hooks/#how-can-i-add-a-new-method-via-a-hook

I know that feature, but I just don't know what benefits you are thinking of..

[elabx]

2 minutes ago, bernhard said:

But I'm not sure how useful that would be compared to calling the module directly:

Me neither haha just thought it would be the most simple way to extend RocKMigrations. Install the plugin module and just assume now you can do this in migration code:

$rm = $modules->get('RockMigrations');
$rm->migrate([...]);
$rm->fooPluginNewMethod();

 

[da²]

5 hours ago, bernhard said:

Also splitting the API brings in other problems. Many methods do something with two things, eg "addFieldToTemplate" - would that be $rm->fields->addToTemplate("myfield", "mytemplate") or would that be $rm->templates->addField("mytemplate", "myfield") ?

Refactoring doesn't mean an API change. If the only problem is to have 3000 lines in a single file, it can also be splitted in several small classes without changing the module API.

[bernhard]

Just had a look at the code and some cleanup would be nice for sure (though not necessary imho).

On 9/29/2023 at 6:56 PM, da² said:

Refactoring doesn't mean an API change. If the only problem is to have 3000 lines in a single file, it can also be splitted in several small classes without changing the module API.

How would that work? For example the method "addFieldToTemplate(...)". How would I move that method to another file without changing the api?

I know it would be possible to add that method to $rockmigrations via API addHookMethod, but that makes it a lot more complicated to develop and to also support proper intellisense. Also I guess it would have more overhead.

[da²]

2 hours ago, bernhard said:

How would that work? For example the method "addFieldToTemplate(...)". How would I move that method to another file without changing the api?

I don't know your module's code so I can't be specific, but seeing it's 3000 lignes long I think the main idea would be to split responsibilities into different classes. The Single-responsibility principle says that each class should only manage a single thing. It makes code cleaner, easier to manage and to test in the future.

I'm saying this without knowing the code so it's just generic ideas. If your module manage fields, templates and migrations, you could add classes FieldBuilder, TemplateBuilder, MigrationManager...
The module API would be something using fluent interface like this (OK I said that changing API is not necessary and I'm changing it... ^^):

$myField = $module->newField()->type(FieldType::INTEGER)->name('foo')->min(1)->max(10);
$myTemplate = $module->newTemplate()->name('bar')->addField($myField);

With newField() and newTemplate() methods returning a FieldBuilder and a TemplateBuilder.

The benefit is to move field or template specific code in classes that manages only this.

Just some thoughts... 🙂