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

Move ImmutableRecord to own repository and use ImmutableObject trait #106

Open
codeliner opened this issue Nov 8, 2018 · 7 comments
Open

Comments

@codeliner
Copy link
Member

@enumag uses a neat trick to unset public properties of an object, see this gist

This way magic __get and __set methods of the object are called instead and they can enforce immutability.

This mechanism can even be deactivated in production for performance reasons and it can be combined with the ImmutableRecord logic used for immutable data types in Event Machine powered projects.

To be able to use immutable data types without installing Event Machine, the package should be moved to its own repo.

@enumag
Copy link

enumag commented Nov 8, 2018

Note: I wrote the implementation in the gist just today as a proof of concept. I didn't even try to use is anywhere yet except the testcase.

I might be able to help with some PRs later but it depends on if I do end up trying to use the event-machine or not. With this plan I will reconsider.

Feel free to use my code from the gist under the terms of the WTFPL license. 😛

@codeliner
Copy link
Member Author

@enumag unset($this->{$property}) is brilliant. Of course I thought about magic __get and __set but this hack did not come to mind. Also thought about a way to disable the mechanism in production and with your trait it should be doable. It looks simple enough to me that I don't see problems with this approach. For example we use Reflection logic a lot in the ImmutableRecordLogic trait. Looks wild, works great ;)

@enumag
Copy link

enumag commented Nov 8, 2018

My thoughts exactly. I'm no stranger to using wild reflection tricks like this. They need to be implemented and used carefully but can help a lot. I do intend to use this trait in my project. For now just with annotated properties which of course loses some type-safety but it's not much of an issue thanks to PHPStan and possible migration to PHP 7.4 properties next year.

I already implemented one simple aggregate using that trait and some EventMachine inspiration and it looks pretty good in my opinion. No dependencies on anything outside of the domain save for that trait. And even the trait could be removed in the future if PHP ever gets support for property accessors.

<?php declare(strict_types = 1);

namespace App\Context\RealtyRegistration\DomainModel\ZipCode;

use App\Context\RealtyRegistration\DomainModel\ZipCode\Command\RegisterZipCodeCommand;
use App\Context\RealtyRegistration\DomainModel\ZipCode\Command\UnlistZipCodeCommand;
use App\Context\RealtyRegistration\DomainModel\ZipCode\Event\ZipCodeRegistered;
use App\Context\RealtyRegistration\DomainModel\ZipCode\Event\ZipCodeUnlisted;
use Generator;

final class ZipCode
{
	public static function register(RegisterZipCodeCommand $command): Generator
	{
		yield ZipCodeRegistered::create(
			function (ZipCodeRegistered $event) use ($command): void {
				$event->zipCodeId = $command->zipCodeId;
				$event->zipCodeData = $command->zipCodeData;
			}
		);
	}

	public static function unlist(ZipCodeState $state, UnlistZipCodeCommand $command): Generator
	{
		yield ZipCodeUnlisted::create(
			function (ZipCodeUnlisted $event) use ($state): void {
				$event->zipCodeId = $state->zipCodeId;
			}
		);
	}

	public static function applyZipCodeRegistered(ZipCodeRegistered $event): ZipCodeState
	{
		return ZipCodeState::create(
			function (ZipCodeState $state) use ($event): void {
				$state->zipCodeId = $event->zipCodeId;
				$state->zipCodeData = $event->zipCodeData;
			}
		);
	}

	public static function applyZipCodeUnlisted(ZipCodeState $state, ZipCodeUnlisted $event): ZipCodeState
	{
		return $state->cloneUsing(
			function (ZipCodeState $state) use ($event): void {
				$state->unlisted = true;
			}
		);
	}
}

@enumag
Copy link

enumag commented Nov 8, 2018

Thinking about it I could simplify it further by sending a cloned mutable version of the state into the apply methods and freezing it right after. WDYT? In case of the code above the last method would be changed into this:

	public static function applyZipCodeUnlisted(ZipCodeState $state, ZipCodeUnlisted $event): ZipCodeState
	{
		$state->unlisted = true;
	}

@codeliner
Copy link
Member Author

I would not pass a mutable version of the object into the function. Are the properties of ZipCodeState mutable or immutable? Mixing both is confusing and could lead to new sources of errors. Immutable data types are great because they avoid certain types of bugs.

See here: https://github.com/proophsoftware/fee-office/blob/master/src/RealtyRegistration/src/Model/Apartment.php#L78

An immutable collection of apartment attributes replaces the old one.

That's the reason why I want to combine your ImmutableObject trait with our ImmutableRecordLogic trait. I still want to use the immutable API:

$address = Address::fromRecordData([
  'street' => Street::fromString('Main Road'),
  'city' => City::fromString('Somewhere'),
]);

echo $address->street; //Main Road

$changedAddress = $address->with([
  'street' => 'Another Street'
]);

echo $address->street; //Main Road
echo $changedAddress->street; //Another Street
echo $changedAddress->city; //Somewhere

@enumag
Copy link

enumag commented Nov 8, 2018

I would not pass a mutable version of the object into the function. Are the properties of ZipCodeState mutable or immutable? Mixing both is confusing and could lead to new sources of errors

Yeah I guess you're right. I still don't like your usage of arrays though since IDE won't be able to suggest the keys. So I'll stick to my Closure initializers for that reason.

Anyway another thing I was thinking about was to move the apply methods from the Aggregate class to the State class. Currently the aggregate implements both the generators to yield new events and the apply methods to create new State which kinda violates SRP in my opinion. The main logic is always in the generators which need to be in the aggregate of course. But the apply methods are straightforward copy-pasting of event data to the state so I feel like they belong either to the State class or into some other StateUpdater class separate from the aggregate logic.

@codeliner
Copy link
Member Author

I still don't like your usage of arrays though since IDE won't be able to suggest the keys.

I use constants to get around both: typos and missing IDE support
See here for example: https://github.com/proophsoftware/fee-office/blob/master/src/RealtyRegistration/src/Model/Building/State.php#L14

You can even express key relations like shown above: State::BUILDING_ID is identical to Payload::BUILDING_ID.

I've a set of PHPStorm live + class templates to generate those classes, constants and properties quickly. Needs a bit practice but then you're very productive and typos belong to the past.

move the apply methods from the Aggregate class to the State class

You can definitely do that. For me the aggregate class turned into the last part of the naemspace, because when I work with Event Machine I always use the functional approach so those aggregate functions are just grouped in class, but each function is independent of the others. SRP is not violated because we don't talk about an object doing different things. We talk about public static functions of a class and each function does only one thing: either handle a command or apply an event ;). The functions need to be stateless and side-effect free, though. https://proophsoftware.github.io/event-machine/api/aggregates.html#3-3-1

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

No branches or pull requests

2 participants