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

Feature/events #233

Closed
wants to merge 5 commits into from
Closed

Feature/events #233

wants to merge 5 commits into from

Conversation

fago
Copy link
Owner

@fago fago commented Sep 22, 2015

Improve how events are defined.

@klausi
Copy link
Collaborator

klausi commented Sep 24, 2015

Meh, public properties. But trying to build a method name could also be ugly:

$name_parts = array_map('ucfirst', explode('_', $context_name));
$method = 'get' . implode('', $name_parts);
$value = $event->$method;

Not sure what would be best.

@fago
Copy link
Owner Author

fago commented Dec 7, 2015

Yes, still I think supporting methods like this also would be good - so you can easily add some simple calculations etc. Whether methods can provide a solution for lazy-loading we'll have to figure out separately.

Howsoever, I think public properties are better than methods here if there is no need to add any logic. Thus, the event class basically is a simple class based replacemented for an array, so it's properly documented and has auto-completion. I think public properties are the best compromise combining the advantages of getter/setter methods (properly defined & documented!) and GenericEvent (ease of defining it, DX), thus should be the way to go. I don't see writability of them as problematic - first off I don't see that we have to prevent that, second GenericEvent has write support also.

@klausi
Copy link
Collaborator

klausi commented Dec 7, 2015

I find it a bit ugly that you have to write more boilerplate code that just duplicates the meta data info from rules.rules.events.yml.

But yeah, I don't mind too much.

@fago fago closed this Jan 28, 2016
@fago fago deleted the feature/events branch February 6, 2016 15:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants