Skip to content
This repository has been archived by the owner on Nov 11, 2020. It is now read-only.

Fix EventArgs so you can modify data. #98

Closed
wants to merge 1 commit into from
Closed

Conversation

jwage
Copy link
Member

@jwage jwage commented Mar 8, 2013

fixes #94

I think all we need is something like this?

@@ -37,7 +37,7 @@ class EventArgs extends BaseEventArgs
public function __construct($invoker, &$data = null)
{
$this->invoker = $invoker;
$this->data = $data;
$this->data =& $data;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless getData() returns values by reference and users assign that return value by reference in turn (see: here), I don't think this accomplishes anything. @henrikbjorn mentioned that he didn't actually want to change the contents of the data (realistically, the results), but rather change it to something else entirely. His use case sounds like implementing a slim ODM atop our collection class. In that case, even working with references isn't enough -- as he is limited to changing the contents of the data.

On a side note, having the constructor arguments passed by reference means we can't use literals (came up while I was writing a unit test).

I think we can do without references throughout the event classes and simply re-assign the query or results accordingly in the classes that dispatch the events. That would allow complete flexibility to alter or reassign the data, and copy-on-write semantics should avoid needless memory usage.

@jmikola
Copy link
Member

jmikola commented Apr 23, 2013

Based on input from @beberlei, I think what we need here are explicit setter methods, as is done in ORM: https://github.com/doctrine/doctrine2/blob/master/lib/Doctrine/ORM/Event/PreUpdateEventArgs.php (see setNewValue()).

@henrikbjorn: Would that address your use case?

@henrikbjorn
Copy link
Contributor

Yes i need to be able to replace the whole data structure. I think it would.

@jmikola
Copy link
Member

jmikola commented May 2, 2013

Closing in favor of #106.

@jmikola jmikola closed this May 2, 2013
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.

Events from Collection are not that useful.
3 participants