Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revision for 0.2 #16

Merged
merged 39 commits into from
Apr 10, 2020
Merged

Revision for 0.2 #16

merged 39 commits into from
Apr 10, 2020

Conversation

mecha
Copy link
Member

@mecha mecha commented Nov 6, 2019

Revises the interfaces for the 0.2 spec.

  • Module setup() now returns a service provider.
  • Module run() now requires the container argument.
  • Removed interfaces that are no longer required.

Also:

  • Now requires at least PHP 7.0.
  • Removed dependency on Dhii exceptions, stringable interface, and Xpmock.
  • Now using PHPUnit 6.
  • Switched from NetBeans to PHPStorm, and added a lot of relevant configuration.
  • Using Docker to develop and test, and included PHPStorm integration configuration for many features.

@mecha mecha self-assigned this Nov 6, 2019
composer.json Show resolved Hide resolved
@mecha
Copy link
Member Author

mecha commented Nov 6, 2019

@XedinUnknown What do you think about changing the namespace of the package to just Dhii\Modular?

@mecha
Copy link
Member Author

mecha commented Jan 10, 2020

@XedinUnknown Any change we can not let this die?

@XedinUnknown
Copy link
Member

Definitely not going to die.

@mecha
Copy link
Member Author

mecha commented Feb 11, 2020

@XedinUnknown We can. But then it's not worth declaring that Exception is thrown IMO.

@XedinUnknown
Copy link
Member

Well, many people assume that when no @throws is declared it means it doesn't throw. So IMO exception is still important.

composer.json Show resolved Hide resolved
.travis.yml Outdated Show resolved Hide resolved
@mecha
Copy link
Member Author

mecha commented Mar 15, 2020

@XedinUnknown We had briefly talked about PHP version recently. Are we bumping the minimum required version to 7.1 for this package? (I'm in favor)

Also, as I mention here, maybe we could relax the allowed exception type thrown by modules to just Exception?

I was thinking ... it might make more sense to relax it to RuntimeException.

@mecha mecha requested a review from XedinUnknown March 15, 2020 10:18

->getModule()
->new();
$mock = $this->getImplementingMockBuilder('Exception', [TestSubject::class])
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The use of the trait and this method is causing builds to fail.

@XedinUnknown This test does not require the test subject interface to be implemented by a mock that extends Exception. Any reason why this is being used?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test does not require the test subject interface to be implemented by a mock that extends Exception. Any reason why this is being used?

It doesn't, but it should, because the subject must be throwable. Fixing...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why should it be throwable?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because it's something you... throw?

Copy link
Member Author

@mecha mecha Mar 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is testing the interface, and nothing about the test nor the interface has anything to do with throwing. Contrary to what you said, the subject does not need to be throwable. Implementations of the interface perhaps, but not the interface. And the test you wrote clearly demonstrates this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the subject does not need to be throwable

Of course it needs to be throwable. You cannot catch something that is not throwable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But you're not catching it. It's a bloody interface. And your test just checks if its module-aware.

@XedinUnknown
Copy link
Member

Looks like it's not possible to satisfy both PHPUnit 6 and 7. PHP 7.1 it is, then...

@XedinUnknown XedinUnknown added this to the v0.2 milestone Mar 16, 2020
Copy link
Member Author

@mecha mecha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I stopped reviewing the readme at around "Module Loading" because I started to notice a pattern.

It seems as though you are conflating module standards with module system standards. The conventions that you are putting forth here are very similar to what EDDBK had, where modules are customly installed and created using factories. I'm thankful that, at least, your examples are not glob-ing directories to detect modules through the file system! 😆

I suggest pruning the standard, and avoid setting conventions on code structure, files organization, construction, etc. The only important part of a module standard is the module class itself and its services. Everything else is a module system's responsibility.

That's assuming an application will use a module system at all. If exotic features like sorting or boxing are not needed, then this will get the job done.

$modules = [
	new FooModule(),
	new BarModule(['some' => 'config']),
	new LoremModule($enableSomeFeature = true),
	new IpsumModule(__FILE__)
];
$providers = array_map(fn($mod) => $mod->setup(), $modules);
$container = new Container($providers);

foreach ($modules as $module) {
	$module->run($container);
}

What I want to say is, standard-compliant modules should be usable in any application, regardless of how they are loaded.

Think of it this way: a Skyrim mod works regardless of how you install it; manually, or with a manager like NMM, MO or Vortex 🤣

Comment on lines +25 to +27
In your module's pacakge, create a file that returns a module factory. This factory MUST return an instance
of `ModuleInterface` from this pacakge. By convention, this file has
the name `module.php`, and is located in the root directory. Below is a very basic example. In real life,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is a factory function necessary? A consumer app just call the module's constructor, the class of which is autoloaded by Composer.

Copy link
Member

@XedinUnknown XedinUnknown Apr 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because you cannot and should not standardize the module's constructor, as every module can have their own requirements. But you can and should standardize a factory, in the sense that all module factories will receive the same parameters.

of `ModuleInterface` from this pacakge. By convention, this file has
the name `module.php`, and is located in the root directory. Below is a very basic example. In real life,
the service provider and the module will often have named classes of their own, and factories and extensions
will be located in `services.php` and `extensions.php` respectively, by convention.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lol I forgot about this "convention". Are we keeping it? I don't like it, to be honest. It just adds makes a package feel inconsistent, with a mix of lowercase.php PHP scripts and PascalCase.php PSR-4 style class files.

I'd omit this from the readme. Module authors can get as creative as they want with their modules, but I wouldn't establish something like this as convention unless there's a good reason to.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Module authors can still get as creative as they want, because nobody is forcing them to follow the convention. These files can really be anywhere and be called anything, because only the module itself interacts with them. However, it makes it faster for other module users to understand the module.

A special case is module.php. It contains the module factory function. This convention is useful if an application wants to specify modules by name such as package name. For this to work, all module files have to have the same name and relative location.

I was recently asked by several people to describe how to use the module system, and that's what I did here, because people are confused about how to get started with it. I agree that this all convention and opinion. Ultimately, it belongs in some technical post written by one of us and shared in relevant dev channels. But for now, this is the centralized location which everyone will look at, and they will know how to gets started quickly.

README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
@@ -49,6 +49,12 @@ public function testCanBeCreated()
'A valid instance of the test subject could not be created'
);

$this->assertInstanceOf(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So let me get this straight.

You're taking an interface, using a mock-hack to make it throwable, then asserting that the mock is throwable?

Sounds legit 🤣

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤷‍♂️

@XedinUnknown
Copy link
Member

XedinUnknown commented Apr 10, 2020

I know you're not happy with the documentation I wrote, and I myself see that it could be done better, according to some of your suggestions, which are much appreciated. So here's what I'm going to do.

I will merge this documentation, and make a long-time-coming release. At some point after this, we can take the time to review the docs and make an update, because this will not break BC. We can then publish a co-authored (or separate) paper on how we use the module system, in which we describe the conventions we use, if any. IMO Medium is a good place for this kind of stuff, at least until we have our own blog (which at this point I don't feel is necessary).

@XedinUnknown XedinUnknown merged commit a83bb8b into develop Apr 10, 2020
@XedinUnknown XedinUnknown deleted the 0.2.x branch April 10, 2020 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants