spatie / laravel-event-projector

Event sourcing for Artisans 📽
https://docs.spatie.be/laravel-event-projector
MIT License
636 stars 62 forks source link

Cannot add metada after upgrading to v3 #190

Closed rodrigopedra closed 5 years ago

rodrigopedra commented 5 years ago

After upgrading to v3 I get the following exception when adding meta data to an event:

Indirect modification of overloaded property Spatie\EventProjector\Models\EloquentStoredEvent::$meta_data has no effect

I tried to recreate with a clean Laravel install and also got the problem :

Steps to reproduce

  1. Create a new Laravel App
  2. Add the package and export config
  3. Run migrations
  4. Add the test code below to your routes/web.php
  5. Access your project Home Page
<?php

use Illuminate\Support\Facades\Event;
use Illuminate\Support\Facades\Request;
use Spatie\EventProjector\ShouldBeStored;
use Spatie\EventProjector\Models\EloquentStoredEvent;

EloquentStoredEvent::creating(function ($event) {
    $event->meta_data['ip'] = Request::ip();
});

Route::get('/', function () {
    Event::dispatch(new TestEvent());

    return 'ok';
});

class TestEvent implements ShouldBeStored
{
}
rodrigopedra commented 5 years ago

As a workaround, replacing the whole meta data array works:

<?php

use Illuminate\Support\Facades\Event;
use Illuminate\Support\Facades\Request;
use Spatie\EventProjector\ShouldBeStored;
use Spatie\EventProjector\Models\EloquentStoredEvent;

EloquentStoredEvent::creating(function ($event) {
    // **** CHANGED HERE ****
    $event->meta_data = ['ip' => Request::ip()];
});

Route::get('/', function () {
    Event::dispatch(new TestEvent());

    return 'ok';
});

class TestEvent implements ShouldBeStored
{
}

I got the idea to test this workaround from this issue (in Laravel repo) description:

https://github.com/laravel/framework/issues/7672

rodrigopedra commented 5 years ago

It seems to be an issue with Laravel. I tried the following to double check:

  1. added a meta_data column to the users table and casted it to array
  2. Changed the test code as follow
<?php

use App\User;
use Illuminate\Support\Facades\Event;
use Illuminate\Support\Facades\Request;
use Spatie\EventProjector\ShouldBeStored;
use Spatie\EventProjector\Models\EloquentStoredEvent;

EloquentStoredEvent::creating(function ($event) {
    $event->meta_data['ip'] = Request::ip();
});

User::creating(function ($user) {
    $user->meta_data['ip'] = Request::ip();
});

Route::get('/', function () {
    User::create(['name' => 'dummy', 'email' => 'foo@example.com', 'password' => '123']);
    Event::dispatch(new TestEvent());

    return 'ok';
});

class TestEvent implements ShouldBeStored
{
}

Now it fails before getting to the EloquentStoredEvent event listener.

I will open an issue in the Laravel framework


EDIT

Tested the User Model with previous Laravel versions and the exception is also thown. I don't know if it is related to a PHP versioning (updated my php 7.3 yesterday), but I can assure it was working before.

I guess I will stick with the workaround

rodrigopedra commented 5 years ago

Also the docs for storing metadata in v3 are not updated to use the EloquentStoredEvent:

https://docs.spatie.be/laravel-event-projector/v3/advanced-usage/storing-metadata/

I can send a PR fixing it, but I want to know how we can handle this issue.

I am ok with the workaround, just want to confirm if there is not a solution on using the old way.

Thanks.

rodrigopedra commented 5 years ago

Ok, promise it will be the last comment for now...

Testes with a clean Laravel 5.8 install and it works as in documentation.

Steps:

  1. composer create-project --prefer-dist laravel/laravel laravel58 "5.8.*"
  2. composer require spatie/laravel-event-projector:^2.0.0
  3. publish migration, run migration
  4. Add code below to routes/web.php
  5. Navigate to home page

Seems that something changed with how the meta data is handled. Unfortunately I don't have the time now to dig in, also there are a lot of commits between the versions.

I am ok with the workaround of reassigning the meta data array. Just want to raise the issue as it was the only thing preventing me to migrate a project to Laravel 6.

If the solution is to reassign the metadata array, please let me know and I will send a PR to the docs.

If you read until here, thanks for the patience. :)

<?php

use Illuminate\Support\Facades\Event;
use Illuminate\Support\Facades\Request;
use Spatie\EventProjector\ShouldBeStored;
use Spatie\EventProjector\Models\StoredEvent;

StoredEvent::creating(function ($event) {
    $event->meta_data['ip'] = Request::ip();
});

Route::get('/', function () {
    Event::dispatch(new TestEvent());

    return 'ok';
});

class TestEvent implements ShouldBeStored
{
}
freekmurze commented 5 years ago

@riasvdv could you take a look at this one?

riasvdv commented 5 years ago

@rodrigopedra I forgot to update the docs for this, they should now be updated to reflect the StoredEvent changes. https://docs.spatie.be/laravel-event-projector/v3/advanced-usage/storing-metadata/

Since we're now using a repository to store the events, you'll need that in your projector to update the storedEvent, if you want to use model events, you'll need to create your own repository that extends the EloquentStoredEventRepository and changes the model there.

Let me know if this helps or you run into any other issues!

riasvdv commented 5 years ago

@rodrigopedra I've made a PR https://github.com/spatie/laravel-event-projector/pull/191 that would make the upgrade a lot easier if you just want to use a custom Eloquent model

rodrigopedra commented 5 years ago

Hi @riasvdv thanks for the response.

Maybe due to the fact English is not my native language I might have expressed myself in a confusing way. Or might be due to the numerous messages.

Note I never said that I am using a custom model. It is just out of the box with minimal configuration. I don't even publish the config, just the migration file. In my production app I also do not use a Custom Model.

I'll try to be more objective.

$event->meta_data['foo'] = 'bar';

Executing the code above with version 3 of this package raises the ErrorException described in this issue's opening message. I updated my sample app to version 3.1 and it fails as well.

As a workaround the code below works with version 3.

$event->meta_data = ['foo' => 'bar'];

Note that the difference is that in this code I am reassigning the whole array to the meta_data property.

It is different from what worked before and is different from the updated docs.

If you want to try it out, just follow the steps in this opening post. Besides running the migrations you need only to modify the routes/web.php file. No custom config or custom models are needed .

I am ok if there is no solution to get the old syntax back, from my research this is a limitation of PHP as the meta_data array is returned from a magic property (__get on Eloquent Model). This blog post has some related links that describes why it should not work: https://phpolyk.wordpress.com/2012/07/25/indirect-modification-of-overloaded-property/

I couldn't figure out why it worked in the previous version, if you read in one of my previous comments I tested using a plain Eloquent Model with an attribute casted to array, and the same exception is thrown in both Laravel 5.8 and Laravel 6. But somehow it worked for the meta_data from the model on this package.

In my real app, where I use this package I also do not extend the StoredEvent model. I updated to reassigning the whole array and not to setting the property as the example in the docs.

My question is if we can revert to the old syntax or if not, if we should update the docs accordingly. It would also nice to have tests. I don't know how to write them as I don't know what caused the behavior change.


EDIT My guess, (very naive guess, I didn't have the time to dig in the changes) is that the behavior changed due to the removal of the https://github.com/spatie/laravel-schemaless-attributes dependency. But again this is just a guess.

rodrigopedra commented 5 years ago

Hi @riasvdv ,

Indeed adding the laravel-schemaless-attributes dependency solved the issue.

I created PR #194 to revert on using that package.

I outlined the reasons in the PR description.

riasvdv commented 5 years ago

You're right, I've merged the PR, hope everything is fixed then!

rodrigopedra commented 5 years ago

Thanks @riasvdv ! And thanks for the patience.