Conversation
|
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. |
|
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 :) |
|
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). |
|
That's alright, I don't agree but I can live with it. Thanks for the ArrayAccess. |
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.