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

Decouple classes using dependency injection #1164

Merged
merged 23 commits into from
May 1, 2020
Merged

Decouple classes using dependency injection #1164

merged 23 commits into from
May 1, 2020

Conversation

jtojnar
Copy link
Member

@jtojnar jtojnar commented Dec 29, 2019

Instead of instantiating its dependencies in each class, we are going to switch to dependency injection. We will declare the dependencies using typehinted constructor arguments and pass already instantiated dependency objects to the constructor. This will loosen the coupling between classes and allow us to pass mock objects for testing.

Rather than creating and passing instances of dependencies manually, we will let Dice dependency injection container library instantiate them for us. We are setting CONTAINER value in fatfree’s Hive to a function wrapping around Dice->create method (Dice does not support PSR-11), which will make fatfree ask Dice to instantiate controllers. Dice will recursively instantiate all of the controller’s dependencies and then instantiate the controller itself.

At the moment, only a small fraction of selfoss was converted but more is coming soon.

@jtojnar
Copy link
Member Author

jtojnar commented Dec 29, 2019

The hardest part to convert are the DAOs, which are pretty messy. They use something like bridge design pattern but everything is spiked with global data container/service locator (f3’s Hive). Plus inheritance being used as code deduplication technique.

Class Diagram1

Ideally, we would end up with something like the following:

Class Diagram4

Bridges will need to take dependency container and create the backends using that.

@jtojnar
Copy link
Member Author

jtojnar commented Dec 29, 2019

Late static binding is pretty awesome:

<?php
class FooStmt {
	public static function test() {
		echo "FooStmt::test" . PHP_EOL;
	}
}

class BarStmt extends FooStmt {
	public static function test() {
		echo "BarStmt::test" . PHP_EOL;
	}
}

class Foo {
	static $stmt = FooStmt::class;

	public static function test() {
		echo get_called_class() . PHP_EOL;
		echo self::$stmt . PHP_EOL;
		echo static::$stmt . PHP_EOL;

		static::$stmt::test();
	}
}

class Bar extends Foo {
	static $stmt = BarStmt::class;
}

Foo::test();
Bar::test();

produces

Foo
FooStmt
FooStmt
FooStmt::test
Bar
FooStmt
BarStmt
BarStmt::test

We can use that to avoid inheriting from Database.

@jtojnar
Copy link
Member Author

jtojnar commented Feb 5, 2020

With late static bindings it is closer to

Class Diagram3

@jtojnar jtojnar marked this pull request as ready for review April 19, 2020 11:34
@jtojnar jtojnar added this to the 2.19 milestone Apr 26, 2020
jtojnar added 21 commits May 1, 2020 04:09
BaseController does not really depend on View helper so it the helper
is only instantiated there to save us some typing. But if we want to inject
it via dependency injection container, we would need to pass it in each
controller’s constructor to BaseController anyway. So we can as well move
the instantiation there.
Using inheritance to share few helper methods is not a proper design
and, if we wanted to use dependency injection autowiring, we would
have to pass the dependencies down in every child class constructor.
For now, let’s move the methods to the Authentication helper, which,
while not the best place, having too many responsibilities already,
is at least thematicaly relevant.

Since the BaseController is now empty, we can get rid of it.
Instead of instantiating its dependencies in each class, we are going
to switch to dependency injection. We will declare the dependencies using
typehinted constructor arguments and pass already instantiated dependency
objects to the constructor. This will loosen the coupling between classes
and allow us to pass mock objects for testing.

Rather than creating and pasing instances of dependencies manually,
we will let Dice dependency injection container library instantiate them
for us. We are setting CONTAINER value in fatfree’s Hive to a function
wrapping around Dice->create method (Dice does not support PSR-11),
which will make fatfree ask Dice to instantiate controllers.
Dice will recursively instantiate all of the controller’s dependencies
and then instantiate the controller itself.

For now, we are only injecting View helpers.
Also the controllers are often instantiating other controllers, including
themselves. This is a bad design but we are not going to address it here.
Another step in decoupling selfoss.
One more step in decoupling selfoss classes.

We mark helpers\Authenticate class shared since we only want a single
instance of it – mainly because the helper configures session cookie
before starting a session, and the configuration cannot be repeated
after the session has been started.

We still need to let the DI container construct the database classes
so that we can inject Auth helper there too. Until then, we will
keep registering it with the service locator.
This will allow us to only inject the necessary classes.

No other changes were made.
The structure of DAOs is pretty convoluted: sqlite and pgsql items/sources/tags inherit from the corresponding mysql classes to facilitate code reuse. Additionally, the mysql classes inherited from mysql\database, which instantiated statements helper based on the database type.

We move the statements initalization to each individual class to allow us break the dependency on database. Late static binding allows us to do this pretty conveniently while retaining the inheritance as code deduplication hack (to be fixed later).
Apparently PHP 5.6 does not support calling functions using `static::$stmt::fn()` so we need to save `static::$stmt` into a local variable.
So that we do not need to inject ContentLoader to Index.
In order not to inject unnecessary DAOs for Authentication and About endpoints.
We create an interface for each DAO type and configure the DI container
to return the implementation corresponding to the backend selected
in the configuration.
Top-level DAOs will then just ask for the interface and Dice will
provide the correct instance.
We now inject the logger everywhere.

This means some spouts (spouts\rss\feed and spouts\reddit\reddit2
in particular) now take dependencies in their constructor.
That also means their children will need to pass them up in constructor
chain if they have constructors of their own.

Spouts also no longer have getImageHelper method, the helper is injected
through constructor where necessary.

Similarly, the WebClient is injected to its users instead of getting
it using singleton class.

Lastly, the return type annotation of daos\Sources::validate
was narrowed down  to get rid of Psalm error.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant