Skip to content

Simpler events#44

Closed
Seldaek wants to merge 0 commit intofabpot:masterfrom
Seldaek:simple_events
Closed

Simpler events#44
Seldaek wants to merge 0 commit intofabpot:masterfrom
Seldaek:simple_events

Conversation

@Seldaek
Copy link
Copy Markdown

@Seldaek Seldaek commented Oct 3, 2010

You said no magic. ArrayAccess is magic, and it hides the fact that we're dealing with objects, I think it shouldn't be used if not necessary.

Also throwing an exception + checking if the key is set in the getParameter() method is bad imo, because people that do things right will call hasParameter() first, in this case the code is wasteful, and those that don't care won't check and their code will explode with an exception while it could have ran fine with simple php warnings.

@fabpot
Copy link
Copy Markdown
Owner

fabpot commented Oct 4, 2010

Yes, I said no magic, but \ArrayAccess is commonly used in many frameworks. It provides a nice short syntax for common cases. As for the removal of the exception in getParameter(), I don't understand why you would want to do that. If the parameter does not exist, I don't want to just fallback to a PHP notice.

@Seldaek
Copy link
Copy Markdown
Author

Seldaek commented Oct 4, 2010

Well then let me explain some more: ArrayAccess, while commonly use, is pretty harmful in cases where it hides logic. Look at Doctrine1 and all the magic getters, setters, callers, it's really hell to work with and can cause performance hits because people don't really think about what's being executed when they do $foo['bar'], it just looks like an array read. Therefore, unless it's really adding a lot of convenience (which it isn't in this case imo), I would advise against using it to make the code more explicit.

About the Exception, it's just a performance thing. If I code properly and call hasParameter before getParameter to avoid warnings, I don't want the getParameter code to re-do the check for me wasting cycles. I know it's not much of a performance hit but I don't think it's necessary either so why have it. Also for other people that want to code sloppily and not check whether parameters exist and just ignore notices (some people do), in this case they can't just ignore the notice since it's an exception and not a php notice.

If that fails to convince you I guess I rest my case and this can be closed :)

@fabpot
Copy link
Copy Markdown
Owner

fabpot commented Oct 6, 2010

ok, I have merge the removal of ArrayAccess as the event system is only used by "advanced" users, and they do not need the "magic".

I'm still not convinced for the getParameter() method as we avoid PHP Notices/Warnings everywhere in the Symfony2 code. Perhaps we can return null if the parameter does not exist? But then we still need to check for existence and you won't have any performance benefit (if any).

@Seldaek
Copy link
Copy Markdown
Author

Seldaek commented Oct 6, 2010

That's alright, I don't agree but I can live with it. Thanks for the ArrayAccess.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants