Support for PHP 7.1 #31

prolic · 2016-10-06T14:26:05Z

Support for PHP 7.1
uses develop branch of prooph/common branch ("prooph/common" : "dev-develop as 4.0")
uses develop branch of prooph/event-store branch ("prooph/event-store" : "dev-develop as 7.0")
add snapshot namespace (moved from event-store)
add aggregate namespace (moved from event-store)
add docheader
update copyright

oqq · 2016-10-06T17:42:57Z

examples/quickstart.php

         */
-        public static function nameNew($username)
+        public static function nameNew(string $username): User
        {
            //Perform assertions before raising a event
            \Assert\that($username)->notEmpty()->string();


unnecessary "string" assert

oqq · 2016-10-06T17:43:19Z

examples/quickstart.php

        {
            return $this->name;
        }

        /**
         * @param $newName
         */
-        public function changeName($newName)
+        public function changeName(string $newName): void
        {
            \Assert\that($newName)->notEmpty()->string();


unnecessary string assert

oqq · 2016-10-06T17:44:28Z

src/AggregateChanged.php

-     * @param string $aggregateId
-     */
-    protected function setAggregateId($aggregateId)
+    protected function setAggregateId(string $aggregateId): void
    {
        Assertion::string($aggregateId);


unnecessary string assert

oqq

I prefer to remove "beberlei/assert" as dependency, since it is used only to determine if a value is empty. Require a package for that is much overload.

oqq · 2016-10-10T07:28:35Z

examples/quickstart.php

        {
            //Perform assertions before raising a event
-            \Assert\that($username)->notEmpty()->string();
+            Assertion::notEmpty($username);


This line should be replaced, since the "beberlei/assert" dependency is not available in composer anymore.

it's part of prooph/common, so it's still a dependency

You are right.. I did not thought about that.
But then beberlei/assert should stay in composer.json as dependecy, when it is used src/AggregateChanged.php, right?

I did not want to repeat all requirements from prooph/common in all components, since it's already a dependency there. This also helps updating dependencies, as we only have to do it at one place.

Ok it seems to be a question of philosophy.
I disagree, but thats your decision. ☺️

@codeliner your thoughts?

oh, that's a good question ... both PoVs are valid.
When you rely on a dependency from another package and someone in the future decides to remove the dependency then you won't recognize it until hopefully a unit test or integration test will fail. So in general I would do it like suggested by @oqq but prooph/common is our "things we need in every package" package, so in this case @prolic 's argument is also a good one.

Let's go for the save version and handle all dependencies of a package in its composer.json. I already see the PR made by someone who think that we just forgot to add the dependency....

If prooph/common was third party I'd agree with @oqq but prooph/common is maintained by us so @prolic's idea with this is best suited imo.

oqq · 2016-10-10T07:28:45Z

examples/quickstart.php

        {
-            \Assert\that($newName)->notEmpty()->string();
+            Assertion::notEmpty($newName);


This line should be replaced, since the "beberlei/assert" dependency is not available in composer anymore.

oqq · 2016-10-10T07:29:10Z

src/AggregateChanged.php

    {
-        Assertion::string($aggregateId);
        Assertion::notEmpty($aggregateId);


This line should be replaced, since the "beberlei/assert" dependency is not available in composer anymore.

prolic · 2016-10-26T12:48:08Z

@codeliner finished! puh

oqq · 2016-10-26T17:22:03Z

src/Aggregate/AggregateRepository.php

+    /**
+     * @param object $aggregateRoot
+     *
+     * @return int


duplicate return type hint

oqq · 2016-10-26T17:22:38Z

src/Aggregate/AggregateRepository.php

+    }
+
+    /**
+     * @param string $aggregateId


duplicate argument type hint

oqq · 2016-10-26T17:24:09Z

src/Aggregate/AggregateRepository.php

+    /**
+     * @param object $eventSourcedAggregateRoot
+     *
+     * @throws Exception\AggregateTypeException


This is exception is not throw in this method

oqq · 2016-10-26T17:25:04Z

src/Aggregate/AggregateRepository.php

+    /**
+     * Returns null if no stream events can be found for aggregate root otherwise the reconstituted aggregate root
+     *
+     * @param string $aggregateId


argument type hint duplicate

oqq · 2016-10-26T17:26:23Z

src/Aggregate/AggregateTranslator.php

+    /**
+     * @param object $eventSourcedAggregateRoot
+     *
+     * @return int


return type hint duplicate

oqq · 2016-10-26T17:34:27Z

src/Aggregate/ConfigurableAggregateTranslator.php

+     * @param object $eventSourcedAggregateRoot
+     *
+     * @return int
+     */


missing 'throws' annotation

oqq · 2016-10-26T17:35:06Z

src/Aggregate/ConfigurableAggregateTranslator.php

+
+    /**
+     * @param AggregateType $aggregateType
+     * @param Iterator $historyEvents


argument type hint duplicates

oqq · 2016-10-26T17:41:15Z

src/Aggregate/ConfigurableAggregateTranslator.php

+
+        $recordedEvents = $eventSourcedAggregateRoot->{$this->popRecordedEventsMethodName}();
+
+        if (! is_array($recordedEvents) && ! $recordedEvents instanceof \Traversable) {


How about to use the new is_iterable function?

oqq · 2016-10-26T17:46:00Z

src/Snapshot/Snapshot.php

+     * @param string $aggregateId
+     * @param object $aggregateRoot
+     * @param int $lastVersion
+     * @param DateTimeImmutable $createdAt


Some duplicate parameter type hints

yes, but only some... waiting for object type hints :)

oqq · 2016-10-26T17:49:39Z

tests/Mock/RepositoryMock.php

+        return $this->aggregateTranslator;
+    }
+
+    public function accessDeterminedStreamName(?string $aggregateId = null): StreamName


Is this a duplication from 'nullable'? '?string' AND '= null'

@oqq yes, the ? is optional here but it's perfectly valid and it's is more explicit this way. https://wiki.php.net/rfc/nullable_types

codeliner

2 small things and a note. I'm off again. merge when ready.

codeliner · 2016-10-26T17:33:52Z

README.md

@@ -9,7 +9,7 @@ Simple and lightweight event sourcing library with out of the box support for [P

 #Installation

-You can install ProophEventSourcing via composer by adding `"prooph/event-sourcing": "~4.0"` as requirement to your composer.json.
+You can install ProophEventSourcing via composer by adding `"prooph/event-sourcing": "^5.0"` as requirement to your composer.json.


we should promote the composer require command which is available since some time now.
composer require prooph/event-sourcing (true for other prooph packages, too)

codeliner · 2016-10-26T17:41:33Z

examples/quickstart.php

        {
            //Perform assertions before raising a event
-            \Assert\that($username)->notEmpty()->string();
+            Assertion::notEmpty($username);


oh, that's a good question ... both PoVs are valid.
When you rely on a dependency from another package and someone in the future decides to remove the dependency then you won't recognize it until hopefully a unit test or integration test will fail. So in general I would do it like suggested by @oqq but prooph/common is our "things we need in every package" package, so in this case @prolic 's argument is also a good one.

Let's go for the save version and handle all dependencies of a package in its composer.json. I already see the PR made by someone who think that we just forgot to add the dependency....

codeliner · 2016-10-26T17:49:06Z

src/Aggregate/AggregateRepository.php

+    protected function enrichEventMetadata(Message $domainEvent, string $aggregateId): Message
+    {
+        $domainEvent = $domainEvent->withAddedMetadata('aggregate_id', $aggregateId);
+        return $domainEvent->withAddedMetadata('aggregate_type', $this->aggregateType->toString());


only a note, nothing required for this PR but it came to my mind when reading these lines:

when we add the aggregate version to event metadata we should also prefix it like:
$domainEvent = $domainEvent->withAddedMetadata('aggregate_version', $version);

@bweston92 this is what I meant in the prooph chat regarding version vs. position. Now the concept is really clear IMHO.

Version is no longer part of the Message interface, because only domain events have one. And the event store does not rely on the version, because it does not even know that it exists.

lucascourot · 2016-10-26T19:43:21Z

src/Aggregate/ConfigurableAggregateTranslator.php

+     * @param object $eventSourcedAggregateRoot
+     * @param Iterator $events
+     *
+     * @return void


duplicated return type hint

lucascourot · 2016-10-26T19:47:10Z

src/Aggregate/AggregateRepository.php

+/**
+ * Class AggregateRepository
+ *
+ * @package Prooph\EventSourcing\Aggregate


what's the point of duplicating the name and the namespace in the class comment? It doesn't add any value and it forces contributors to maintain these comments over time.

I am with @lucascourot here. Since every 'package' annotation is equal to the namespace it should be omitted at all. https://www.phpdoc.org/docs/latest/references/phpdoc/tags/package.html

Also true for "Class WhatEver" comments. TBH i am using this also 2 years ago but removed it 1 year later. ;)

lucascourot · 2016-10-26T19:56:02Z

src/Aggregate/AggregateRepository.php

+     *
+     * @throws Exception\AggregateTypeException
+     */
+    public function addAggregateRoot($eventSourcedAggregateRoot): void


Why can't we type these AggregateRoot arguments with the Prooph\EventSourcing\AggregateRoot type?

@lucascourot it is not necessary to implement this interface to get working with event sourcing. That is intentional and documented.. somewhere ;)

@oqq ok, got it!

lucascourot · 2016-10-26T19:57:33Z

src/Aggregate/ConfigurableAggregateTranslator.php

+    private $staticReconstituteFromHistoryMethodName = 'reconstituteFromHistory';
+
+    /**
+     * @var null|callable


?callable is not a valid type for that tag and would not work how expected in IDEs.

@oqq why not? It's valid in php for argument types.

Valid since PHP 7.1 as type hint. But I am not sure if it is supported by phpDocumentor, PhpStorm and doctrines annotation reader (as examples).

lucascourot · 2016-10-26T19:57:40Z

src/Aggregate/ConfigurableAggregateTranslator.php

+    private $eventToMessageCallback = null;
+
+    /**
+     * @var null|callable


prolic added the enhancement label Oct 6, 2016

prolic added this to the 5.0 Release milestone Oct 6, 2016

prolic changed the base branch from master to develop October 6, 2016 14:26

prolic force-pushed the php_7_1 branch from 84b0f5a to b84435b Compare October 6, 2016 14:31

prolic added 7 commits October 6, 2016 22:34

update composer, readme, travis and docheader

198248b

update copyright

a731df2

update copyright

8a1a6c8

declare(strict_types=1);

f165e4d

update for PHP 7.1

06a64b6

clean up

1ccad07

apply php cs fixes

0a61a25

prolic force-pushed the php_7_1 branch from b84435b to 0a61a25 Compare October 6, 2016 14:35

prolic added 3 commits October 6, 2016 22:41

fix copyright formatting

4f63421

update quickstart

d2cb11e

some clean up

6062395

oqq reviewed Oct 6, 2016

View reviewed changes

prolic added 2 commits October 7, 2016 01:53

some clean up

c5795de

update phpunit to 5.6

02bb1cf

prolic added the BC break label Oct 7, 2016

fix quick start

b981411

oqq reviewed Oct 10, 2016

View reviewed changes

prolic added 5 commits October 26, 2016 18:45

update license

5ecf08a

add version to AggregateChanged metadata

2c5a923

add version to AggregateChanged metadata

a08bdf1

add Aggregate namespace from event-store

4df0a96

add snapshot and aggregate namespace

3494da7

prolic assigned codeliner Oct 26, 2016

oqq reviewed Oct 26, 2016

View reviewed changes

codeliner requested changes Oct 26, 2016

View reviewed changes

lucascourot reviewed Oct 26, 2016

View reviewed changes

prolic added 6 commits October 27, 2016 12:48

update exceptions

1e59b06

remove class comments

ce1938f

integrate feedback from review

cd643a0

remove class comments in tests

37fb5d2

cleanup

33a53f7

use underscore for internal metadata

438ba50

prolic merged commit 1023f48 into develop Oct 27, 2016

prolic deleted the php_7_1 branch October 27, 2016 05:15


		$recordedEvents = $eventSourcedAggregateRoot->{$this->popRecordedEventsMethodName}();

		if (! is_array($recordedEvents) && ! $recordedEvents instanceof \Traversable) {

Support for PHP 7.1 #31

Support for PHP 7.1 #31

Conversation

prolic commented Oct 6, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oqq left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prolic commented Oct 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codeliner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oqq Oct 26, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oqq Oct 26, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prolic commented Oct 6, 2016 •

edited

Loading

oqq Oct 26, 2016 •

edited

Loading

oqq Oct 26, 2016 •

edited

Loading