Bertrand Mansion [2007-05-03 05:51 UTC] This is useless. PHP callbacks are already quite comprehensible and don't need to be complicated.
Alexey Borzov [2007-05-03 07:21 UTC] A proper way to use callbacks in OO code is to implement something like Command pattern, not create a useless wrapper around call_user_func().
Travis Swicegood [2007-05-03 15:39 UTC] I feel I need to explain this proposal a little better, my apologies for not doing so in the first place.
The main motivation behind this class is type hinting and callback validation; the ability to type hint for PHP_Callback object that has been pre-validated lends itself to a much more OO design.
This does follow the Command pattern if you use Wikipedia's definition: "... the Command pattern is a design pattern in which objects are used to represent actions."
Had I just implemented PHP_Callback::execute($callback[, $param, ...]), then yes this would be quite useless. However, by instantiating a PHP_Callback, you verify that the object represents a valid action within PHP's user-land. Calling addParameter() and/or addParameterByRef() you can load the callback so it can be repeated multiple times.
I hope that helps clarify the intent.
Joe Stump [2007-05-03 15:51 UTC] I like the idea behind it (being able to typehint a callback in PHP5's OOP). That being said ...
1.) Please look over the CS as the headers/docblocks need a lot of cleanup.
2.) Please consider a bit more lenient license such as the BSD or Apache license (LGPL is an acceptable license, however, I prefer the two I mention. Feel free to ignore me).
3.) Is callback typehinting going to be in PHP6? Is it even possible to typehint a callback? I know that callback is a "type" inside of PHP extensions ...
David Soria Parra [2007-05-03 16:14 UTC] I think it's not needed. As Bertrand said, there is quite a comprehensible callback mechanism in PHP and I don't think we need such a pure OO class in the repository just to fit all design patterns. It's just confusing and not needed, as this pattern is more needed by strict OO languages.
Maybe someone needs such a class, but it's too easy to implement as there is a own PEAR package needed.
But if there are people that want to have it in the repos, I personally prefer BSD/Apache license.
Alexey Borzov [2007-05-03 17:00 UTC] OK, so it's not a useless wrapper around just call_user_func() but a useless wrapper around call_user_func() and is_callable(). Still not impressed.
Joshua Eichorn [2007-05-03 17:02 UTC] In the future lets try to keep our comments constructive. Our goal isn't to chase away everyone who wants to join PEAR its to get fresh blood and useful packages in.
I see another benefit being that the validity of the callback is tested at creation time, where you could actually do something about it failing instead of at call time, and only if the user remembers to use is_callable
Joe: as far as I know a callback in userland is just a string or an array not a separate data type
Joshua Eichorn [2007-05-03 17:04 UTC] Alexey: What other features do you see needed in a callback object
The big features for me are, exceptions if I use a bad callback and type checking. Neither of which i get from raw exceptions.
What else is needed.
Alexey Borzov [2007-05-03 17:18 UTC] Exactly, we need to get *useful* packages in. This, among other things, means leaving useless packages out. Look at Zend Framework's discussions on proposed packages.
The *only* thing the package does is removing the need to do the following check
if (!is_callable($callback)) {
throw new Foo_Package_Exception('Not a valid callback');
}
I don't see any features needed since I don't see the need for a callback object in the first place.
Joshua Eichorn [2007-05-03 17:32 UTC] The point of a library is to remove the need for copy paste. With standard callbacks i need to check them and throw exceptions at creation and callback time. Because this is a hassle no one does this and code quality suffers.
Just because a class is simple doesn't mean its not useful. The whole point of PEAR is so I don't have to re-implement everything that PHP is missing I can pull on the work of what others have done.
Bertrand Mansion [2007-05-03 18:57 UTC] This is PHP, a scripting language. The less cruft, the faster.
There are a lot of other interesting packages PEAR has a need for (right now I am thinking about Text_Unicode, Text_Markdown, XML_Feed_Writer...).
Joe Stump [2007-05-03 19:06 UTC] Bertrand: Please keep comments on topic. You shouldn't respond to a proposal and say "You should have coded X instead". Also, PHP cycles are easy to scale so keeping things lean and fast is kind of a moot point. If this was a DB package I'd see your point.
As it stands, this is an interesting package. As Joshua Eichorn pointed out, this simplifies writing better code. I like two things about it:
1.) It reduces 4 lines of code to 1.
2.) It allows me to do:
public function foo($a, PHP_Callback $callback) {
}
I find both of these reasons make this a legitimate package. I did, however, have issues with the CS, which I've already addressed.
David Soria Parra [2007-05-03 21:29 UTC] Of course it reduce 2-4 lines to code to one, but I don't think it's a great simplification to build a PEAR package from that.
I think it's okay to argue a little bit with the language. For sure it allows type hinting, but thats not what people expect from a dynamic typed language. To stand for that argument you have to add Integer, String, etc classes too, just for the type hinting.
Joe Stump [2007-05-03 21:34 UTC] David: I'm all for allowing type hinting on normal data types. And I don't think is a slippery slope. Arrays, objects and callbacks are complex data types and deserver more attention than, say, string.
Bertrand Mansion [2007-05-04 08:04 UTC] > Also, PHP cycles are easy to scale so keeping things lean and fast is kind of a moot point. If this was a DB package I'd see your point.
Obviously, you don't know what you are talking about. It just takes common sense to know beforehand this is going to be slow.
Without this package:
- 1 function call : call_user_func_(array)
With this package:
- Load the package
- Load PHP/Callback/Exception.php
- Load PEAR_Exception
- Parse all of them
- Call the constructor which itself calls a few other functions
- Call execute which itself calls a few other functions
> David: I'm all for allowing type hinting on normal data types. And I don't think is a slippery slope. Arrays, objects and callbacks are complex data types and deserver more attention than, say, string.
This is silly. Why do you use PHP in the first place? I suggest that you look at other languages that were implemented this way: Python, Ruby, Java, etc. If type hinting has to be globalized in PHP (it already exists, but I understand what you mean even if that's not the correct term to describe it...), it has to be done at the language level, not in PEAR.
Somebody already proposed a package that wrapped a PHP array in an object. This was refused IIRC. I think the same happened to a String object.
Travis Swicegood [2007-05-04 15:37 UTC] I've attempted to stay above the fray here and allow other members of PEAR bat this around. In the end, it is the community who makes the decision as to whether this package is released in PEAR or whether I just toss it in my own PEAR channel and call it a day and since I'm biased to the former my opinion shouldn't carry as much weight. That said, I think there is one observation that hasn't been made yet.
From all of the comments against, it seems that they all have the common thread of a methodological difference when it comes to programming. It seems to askew object-oriented design in favor of the faster, straight procedural style of PHP. I am in no way attempting to force the use of this in every instance in PEAR. It is code that I have used quite extensively in my own programming, and from talking with a few others it appears to provide some missing functionality that quite arguably should be in the core.
On the note of not using this due to speed reasons: where speed is a necessity I would recommend against using PEAR or your own objects/functions and follow a strictly procedural approach. There are cases for both types of use of PHP and that is definitely one of its strengths as a language. It doesn't allows you the flexibility to code in a style that makes the most sense for the task you're tackling.
I may be wrong, but it seems to me that because one developer or another wouldn't specifically use a package due methodological differences in coding style (procedural/functional versus object-oriented, etc.) shouldn't constitute a reason in and of itself to keep a package out of PEAR when it fulfills a very defined purpose within PHP.
Travis Swicegood [2007-05-04 23:42 UTC] Based on the feedback I have received, I have made a few adjustments to the docblocks within the code. I also realized that I wasn't packaging PHP_Callback_Exception, which is kind of a big deal. :-)
The latest version is available from http://plumb.domain51.com/pear/
Please take a look at let me know if there's anything additional that needs to be handled here. It was brought up that I do not specify the @param tags in the __construct(), that is intentional. I am using the PEAR_Exception object as a model of how to document an object that can be instantiated in several different manners.
Assuming no other changes are brought, I'll be moving this to the voting phase the first part of next week.
Mark Wiesemann [2007-05-05 06:58 UTC] Three comments:
- Do you really want to use the LGPL license?
- Coding Standards: comments for the two class properties are missing
- your first version number should be 0.1.0 (you might already know that, of course, it's just a friendly reminder)
Travis Swicegood [2007-05-06 00:30 UTC] Mark:
1) Yes, I really do want the LGPL. I'm a firm believer in keeping my code free, but don't mind other people using it without having to make their code free. :-)
2) My personal belief is that private properties are implementation details and thus shouldn't be documented (i.e., since you can never access PHP_Callback::$callback, it doesn't matter that it exists). If that were only my personal belief, I would have documented them per the CS, however, I'm following the example of PEAR_Exception which validated my point by not documenting any of its private properties either.
3) Yes, I am aware of that. I moved to 0.0.9 from the earlier 0.0.1 and 0.0.2 versions to specify that this is almost ready for prime-time. Prior to moving to a vote, I will change the version number accordingly. Thanks for the reminder, though.
Lukas Smith [2007-05-06 01:29 UTC] You are overlooking that documentation is not only for users, but also for maintainers. Especially as we the PEAR community will take over responsibility if you ever loose interest or get run over by a bus, we would greatly appreciate finding documentation for all of the code. Then again, I was not all that good about documenting all of my proprieties in my packages, so do not consider this as an attempt on my side to throw stones (it would easily backfire), but just a reminder about what the perfect world could look like (then again in a perfect world people do not loose interest or get run over by a bus).
Johannes Schlüter [2007-05-06 09:17 UTC] Ok, I don't really care but I think one point was missed in this whole discussion: How often do you really need a general purpose callback mechanism?
In my experience my "callbacks" need to handle specific parameters or something like that. But these aren't checked by this package. But there's a technology in PHP which is quite good for this: It's called "interfaces". IN places I want to have a custom callback I expect a object implementing a specific interface and call a method on that.
In cases where you need a special purpose callback one should also think on the API usability - it's nice to think on developers of PEAR classes but I think it's way more important to think on the average user using the package. I think every PEAR user knows PHP callbacks (from usort(), preg_replace_callback(), ....) so using that should be straight forward. But using PHP_Callback the user first has to find out what PHP_Callback is and how to use it and as a user, not being interested in implementation details, this looks "strange".
Feel free to ignore my comment - I won't vote anyways :-)
Alexey Borzov [2007-05-07 17:42 UTC] JFYI: http://www.php.net/manual/en/language.oop5.reflection.php
Travis Swicegood [2007-05-08 14:39 UTC] That's actually a really good idea, Alexey. I can use __call() to provide access to all of those methods with practically no overhead (just parsing of __call()) unless they use any of those methods.
One thing is figuring out how to document them. phpDocumentor >= 1.4 has a new @method call, but unfortunately it just adds a new list item to the class summary, not a full fledged method declaration to the documentation. The only thing I can come up with is including a note at the class docblock level that notes that if isFunction() is true, all of the ReflectionFunction methods are available, otherwise all of the ReflectionMethod methods are available.
Any thoughts?
Travis Swicegood [2007-05-08 14:40 UTC] Also, using the callback via usort() has been brought to my attention. I'm planning on adding a read-only callback property to PHP_Callback to handle this situation.
Travis Swicegood [2007-05-08 22:25 UTC] Alexey, or anyone who has a thought one way or the other, I'm trying to come up with a legitimate use case for including the Reflection code. I see it as a potentially useful tool to have, but I'm not sure why anyone would want do Reflection on a callback. Any thoughts one way or the other?
|
