Skip to content
This repository has been archived by the owner on Sep 20, 2019. It is now read-only.

[3.x] Revert to using laravel-schemaless-attributes for meta_data #194

Merged
merged 2 commits into from
Sep 6, 2019
Merged

[3.x] Revert to using laravel-schemaless-attributes for meta_data #194

merged 2 commits into from
Sep 6, 2019

Conversation

rodrigopedra
Copy link
Contributor

@rodrigopedra rodrigopedra commented Sep 5, 2019

In the version 3.x of laravel-event-projector the laravel-schemaless-attributes dependency was removed.

That dependency allowed us to use the following syntax to update an EloquentStoredEvent's meta_data attribute:

$event->meta_data['ip'] = '127.0.0.1';

This syntax is the one outlined in this package documentation.

With the dependency removal this syntax no longer works and an ErrorException: Indirect modification of overloaded property is thrown when trying to set a key in the meta_data property using the above notation.

This error is due that attributes from an Eloquent Model are retrieved due through the __get magic method and not accessed directly. This is a PHP limitation as it returns a copy of the value instead of a reference to the underlying array.

When using the laravel-schemaless-attributes package an instance of the SchemalessAttributes class was returned which implements the ArrayAccess interface. As objects are returned by reference, the syntax in the snippet above works as expected.

One workaround is to use the following syntax:

$event->meta_data = ['ip' => '127.0.0.1'];

This will pass a new array to the __set magic method on the EloquentStoredEvent model and it will replace the underlying meta_data attribute.

The downside is if you have multiple places (listeners for example) that can modify the meta_data property. The user should keep track of the previous value to prevent overriding the whole array and loosing previous changes.

I added a test to make sure the documented syntax works. I appreciate if you can review the tests where I have two doubts:

  1. I needed to use a database raw cast to JSON to compare against the meta_data column in the database
  2. I added a EloquentStoredEvent::flushEventListeners(); to the end of the test, not sure if that is needed.

I will happily make any changes to improve this test

EDIT I removed the MySQL specific syntax due to tests failing with PostgreSQL


Epilogue

The tests already have a test against manipulating an event's meta_data property, why this PR is needed?

Indeed there are already two tests that test against an event's meta_data modification, but those tests assert against the new StoredEvent class which is a wrapper around the EloquentStoredEvent.

In this new class the meta_data is a first-party property, so it is not accessed through the __get magic method, thus the modification through the offset set syntax is allowed.

@riasvdv
Copy link
Member

riasvdv commented Sep 6, 2019

Looks good to me! I thought this wasn't necessary anymore but it indeed is when you want to use the eloquent lifecycle methods to add meta data, thanks!

@riasvdv riasvdv merged commit 0da941d into spatie:master Sep 6, 2019
@rodrigopedra rodrigopedra deleted the patch-model branch September 6, 2019 11:10
@rodrigopedra
Copy link
Contributor Author

Great! Thank you a lot

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants