Cleanup event

[michael-e]

There is no need to put half of the execution code in a separate function, so I copied it into the __trigger() function. The function name was wrong anyway (because the event was actually executed before). I also removed some coments, which actually makes the code simpler to read in this case.

[nilshoerrmann]

Hm, I was actually following the function naming and structure Symphony is proposing in it's core events.

[michael-e]

Really? Core events use an execute function? When has this been added?

[michael-e]

I checked my default installation (integration branch). There is no execute() function in events.

[nilshoerrmann]

https://github.com/symphonycms/symphony-2/blob/master/symphony/lib/toolkit/class.event.php#L148

[michael-e]

Interesting. There is no execute() function in this file, nor can I find one in the provided events.

I still see no reason to use any other function than load and __trigger. Am I missing something?

[michael-e]

The execute() function is in symphony/lib/toolkit/events/class.event.section.php. Strange. This actually means: If you extend the Event class, you must include an execute() function. If you don't, the Event class is basically broken (because it calls a non-existing function). Who was that?

[michael-e]

Still, if you want to use execute, you shouldn't pack half of the logic in the __trigger() function. Why not do it all in execute() and don't extend (i.e. change) __trigger()?

[michael-e]

Maybe one of our code gurus can help: symphonycms/symphonycms@138b779#L0L173

(I hope that emails are sent to the collabs when you add a line note.)

[michael-e]

I thought about this. This code:

https://github.com/symphonycms/symphony-2/blob/master/symphony/lib/toolkit/class.event.php#L148

is actually an invitation to overwrite the __trigger() function. It doesn not mean that you should introduce an execute() function in every event. Since using two different functions in the event it is not even useful in our, case: Please pull this.

[michael-e]

OK, let's wait for brendo here:

symphonycms/symphonycms@138b779#commitcomment-2812273

[nilshoerrmann]

Michael, I agree that less is more here – as long as the execute() function is not required we can move the functionality to the __trigger() function. If there will be a Symphony specification which functions to use inside events later on, we can still adjust our code. No need to wait for Brendan's response in my eyes.

As this pull request is out of date:
Would you mind rebasing it or just push the needed changes directly?
Thanks!

[michael-e]

No prob, will do it.