Hello Internals,
seeing the static calling of instance methods being discussed again, I want
to ask whether the following idea would maybe have a chance?
In our codebase we have one set of classes where it was very useful to be
able to call the same methods both statically and nonstatically: a set of
wrapper classes over phpredis where subclasses "know" which set of redis
servers to talk to (plus other config like retry strategies and timeouts).
In most cases calling code does not need to be concerned with these details
and just calls methods like red_global::get() or red_local::put().
By neccessity the implementation of this class set, must make use of both
__call() and __callStatic() magic methods, with both then dispatching to a
delegate phpredis instance, and in the case of __callStatic(), making
up-front a singleton like "new self" once. For a set of additional helper
methods (not present on the underlying phpredis) I additionally have a
special mapping array of real method names to internal implementation names.
A similar approach can be useful for database connection abstraction
classes.
Now for the idea how this might be made much more simple: onviously in the
light of the "static call to method using $this" warning/detection, PHP
already knows exactly where it hits such a static call to an instance
method. At that point, check whether the class has a "static instance"
already. When it has one, call the method with $this being that "static
instance". Otherwise, create the static instance, calling a new magick
method named __constructStatic() to initialize it. Only classes that have
such a __constructStatic() method, would be treated that way. For other
classes the "static call to instance method" case would be an error as
before/discussed.
This approach would clearly simplify my redis wrapper classes, maybe even
enable them to be proper subclasses of phpredis (saving an indirection to a
$this->delegate object). All the special dispatch and __callStatic
instantiation logic could go away.
The cost would be one internal pointer (to the "static instance" object)
per class, a kind of hidden "static protected" property, and only needed
when the class (or one of its superclasses) has a __constructStatic
method in the first place.
What do you all think? Interesting? Somebody hot for helping with
impementation? Then I'd make an RFC of it.
best regards
Patrick
Hi Patrick,
I don't think it should be implemented. If you are calling a static method
like a non-static method, your code has a problem. I think it would do
more harm than good. Not only for the code comprehension and logic,
but also for the skills of the people who make this mistake.
But I find this __constructStatic() method kinda appealing. I'm writing my
code so I don't need one, but I may use it if it is implemented.
Regards,
Benoit.
"Patrick Schaaf" a écrit dans le message de groupe de discussion :
Hello Internals,
seeing the static calling of instance methods being discussed again, I want
to ask whether the following idea would maybe have a chance?
In our codebase we have one set of classes where it was very useful to be
able to call the same methods both statically and nonstatically: a set of
wrapper classes over phpredis where subclasses "know" which set of redis
servers to talk to (plus other config like retry strategies and timeouts).
In most cases calling code does not need to be concerned with these details
and just calls methods like red_global::get() or red_local::put().
By neccessity the implementation of this class set, must make use of both
__call() and __callStatic() magic methods, with both then dispatching to a
delegate phpredis instance, and in the case of __callStatic(), making
up-front a singleton like "new self" once. For a set of additional helper
methods (not present on the underlying phpredis) I additionally have a
special mapping array of real method names to internal implementation names.
A similar approach can be useful for database connection abstraction
classes.
Now for the idea how this might be made much more simple: onviously in the
light of the "static call to method using $this" warning/detection, PHP
already knows exactly where it hits such a static call to an instance
method. At that point, check whether the class has a "static instance"
already. When it has one, call the method with $this being that "static
instance". Otherwise, create the static instance, calling a new magick
method named __constructStatic() to initialize it. Only classes that have
such a __constructStatic() method, would be treated that way. For other
classes the "static call to instance method" case would be an error as
before/discussed.
This approach would clearly simplify my redis wrapper classes, maybe even
enable them to be proper subclasses of phpredis (saving an indirection to a
$this->delegate object). All the special dispatch and __callStatic
instantiation logic could go away.
The cost would be one internal pointer (to the "static instance" object)
per class, a kind of hidden "static protected" property, and only needed
when the class (or one of its superclasses) has a __constructStatic
method in the first place.
What do you all think? Interesting? Somebody hot for helping with
impementation? Then I'd make an RFC of it.
best regards
Patrick
2015-02-15 21:04 GMT+02:00 Patrick Schaaf php@bof.de:
Hello Internals,
seeing the static calling of instance methods being discussed again, I want
to ask whether the following idea would maybe have a chance?In our codebase we have one set of classes where it was very useful to be
able to call the same methods both statically and nonstatically: a set of
wrapper classes over phpredis where subclasses "know" which set of redis
servers to talk to (plus other config like retry strategies and timeouts).
In most cases calling code does not need to be concerned with these details
and just calls methods like red_global::get() or red_local::put().By neccessity the implementation of this class set, must make use of both
__call() and __callStatic() magic methods, with both then dispatching to a
delegate phpredis instance, and in the case of __callStatic(), making
up-front a singleton like "new self" once. For a set of additional helper
methods (not present on the underlying phpredis) I additionally have a
special mapping array of real method names to internal implementation names.A similar approach can be useful for database connection abstraction
classes.Now for the idea how this might be made much more simple: onviously in the
light of the "static call to method using $this" warning/detection, PHP
already knows exactly where it hits such a static call to an instance
method. At that point, check whether the class has a "static instance"
already. When it has one, call the method with $this being that "static
instance". Otherwise, create the static instance, calling a new magick
method named __constructStatic() to initialize it. Only classes that have
such a __constructStatic() method, would be treated that way. For other
classes the "static call to instance method" case would be an error as
before/discussed.This approach would clearly simplify my redis wrapper classes, maybe even
enable them to be proper subclasses of phpredis (saving an indirection to a
$this->delegate object). All the special dispatch and __callStatic
instantiation logic could go away.The cost would be one internal pointer (to the "static instance" object)
per class, a kind of hidden "static protected" property, and only needed
when the class (or one of its superclasses) has a __constructStatic
method in the first place.What do you all think? Interesting? Somebody hot for helping with
impementation? Then I'd make an RFC of it.best regards
Patrick
+1
In our codebase we have one set of classes where it was very useful to be
able to call the same methods both statically and nonstatically: a set of
wrapper classes over phpredis where subclasses "know" which set of redis
servers to talk to (plus other config like retry strategies and timeouts).
In most cases calling code does not need to be concerned with these details
and just calls methods like red_global::get() or red_local::put().By neccessity the implementation of this class set, must make use of both
__call() and __callStatic() magic methods, with both then dispatching to a
delegate phpredis instance, and in the case of __callStatic(), making
up-front a singleton like "new self" once. For a set of additional helper
methods (not present on the underlying phpredis) I additionally have a
special mapping array of real method names to internal implementation names.
This sounds to me like you should just be using the Singleton pattern,
at which point you need no magic calls at all. It can even be done as a
subclass of an existing non-static class:
class MyRedis extends Redis {
private static $singleton;
public static function singleton() {
if ( is_null(self::$singleton) ) {
self::$singleton = new self; // may need parameters here
}
return self::$singleton;
}
// Other methods can go here, just like a normal subclass
}
// Now wherever in the code you want the default instance, just use this:
$value = MyRedis::singleton()->get($key);
MyRedis::singleton()->someCustomAction($anotherKey);
The nice thing about an explicit Singleton is you can migrate to
Dependency Injection (call "$redis = MyRedis::singleton()" and start
passing the instance around, until you get to the point where it is only
called once), or to a cleverer factory / service locator (store more
than one instance in different static variables or a keyed array, to
connect to different stores for different purposes).
Making static calls implicitly access a singleton seems like it would
make code harder to read, and harder to extend, in exchange for typing a
few characters on each call (you could call it something shorter, like
"::inst()" if you valued brevity) so a -1 from me.
Regards,
--
Rowan Collins
[IMSoP]
Am 15.02.2015 21:05 schrieb "Rowan Collins" rowan.collins@gmail.com:
This sounds to me like you should just be using the Singleton pattern,
Of course this is singleton under the hood.
// Now wherever in the code you want the default instance, just use this:
$value = MyRedis::singleton()->get($key);
You can surely see how this is more readable / easier to write:
$value = MyRedir::get($key);
The nice thing about an explicit Singleton is you can migrate to
Dependency Injection (call "$redis = MyRedis::singleton()" and start
passing the instance around,
I can do that with my redis class, where I need it, by calling a method
MyRedis::link() which just returns $this. But the places where I need that
are rare.
... or to a cleverer factory / service locator (store more than one
instance in different static variables or a keyed array, to connect to
different stores for different purposes).
This i do with subclasses, where the subclass define suitable static
protected properties that guide the connect logic (which sits in the
baseclass). I have a global redis, a local redis (local to the host the
code runs on), a volatile redis (global, but does not make prersistent
dumps), and so on. Calling code ready red_global::this(),
red_volatile::that(), and so on. You don't get it better readable (and
greppable) as that.
Making static calls implicitly access a singleton seems like it would
make code harder to read, and harder to extend,
I don't understand. It's the best to read to me. And the best to extend - I
just make another subclass.
Of course, there are places (in other classes) where stuff needs to use
such a redis instance without caring which one it is - i.e. an instances is
passed to a constructor there - in which case I can use the MyRedis::link()
method to get at the singleton instance.
But in most (I'd guess 90%) of the calling places, it's just the static
calls I need, and writing each of them as MyRedis::singleton()->xxx() is
more tedious.
best regards
Patrick
Am 15.02.2015 21:05 schrieb "Rowan Collins" <rowan.collins@gmail.com
mailto:rowan.collins@gmail.com>:This sounds to me like you should just be using the Singleton pattern,
Of course this is singleton under the hood.
// Now wherever in the code you want the default instance, just use this:
$value = MyRedis::singleton()->get($key);You can surely see how this is more readable / easier to write:
$value = MyRedir::get($key);
Actually, no, I find that harder to read accurately - there is no clue
there that there is actually a singleton under the hood, and that this
is actually an instance method in disguise.
For writing, it saves, in this case, 13 characters; change the method
name to inst(), and that difference goes down to 8 characters.
And if you use it lots of times in succession, you wouldn't even need to
call the accessor every time:
$redis = MyRedis::inst();
$foo = $redis->get('foo');
$bar = $redis->get('bar');
// etc
In that scenario, you've had to write one extra line at the top of the
method, and the rest of the lines are probably shorter (if the variable
name is more than one character shorter than the class name). Plus, you
have the bonus of being able to use an injected connection later.
The nice thing about an explicit Singleton is you can migrate to Dependency Injection (call "$redis =
MyRedis::singleton()" and start passing the instance around,I can do that with my redis class, where I need it, by calling a
method MyRedis::link() which just returns $this. But the places where
I need that are rare.
You may be able to do it with your current implementation, but how do
you propose to do it with an invisible singleton property implicitly
created by __constructStatic?
... or to a cleverer factory / service locator (store more than one instance in different static variables or a
keyed array, to connect to different stores for different purposes).This i do with subclasses, where the subclass define suitable static
protected properties that guide the connect logic (which sits in the
baseclass). I have a global redis, a local redis (local to the host
the code runs on), a volatile redis (global, but does not make
prersistent dumps), and so on. Calling code ready red_global::this(),
red_volatile::that(), and so on. You don't get it better readable (and
greppable) as that.
OK, but you need a new subclass every time, not just a new method, or
new parameter; that limits your options. For instance, you can't
dynamically create instances based on configuration of a reusable
library, which could be as simple as adding an optional parameter to a
standard Singleton:
class MyRedis {
private static $instances;
public static function inst($type='default') {
global $config;
if ( ! array_key_exists(self::$instances, $type) ) {
self::$instances[$type] = new self(...$config['redis_options'][$type]);
}
return self::$instances[$type];
}
}
Any function that already uses the "$redis = MyRedis::inst();" pattern
can then quickly be changed to "$redis = MyRedis::inst('volatile');" and
continue to work fine.
Making static calls implicitly access a singleton seems like it would make code harder to read, and harder to
extend,I don't understand. It's the best to read to me.
OK, we'll call that one a matter of opinion.
And the best to extend - I just make another subclass.
"Extend" was perhaps not the word I should have chosen; I was thinking
of "extending the use of the class beyond the current coding pattern".
By implementing one specific use case as a magic method, you make it
harder to do anything other than that one use case; or, you end up
creating a whole set of magic methods and features, which are then no
easier than just implementing the whole thing by hand.
I deliberately didn't go into details of the arguments against the
Singleton pattern in general in my last e-mail, because I was comparing
your "magic" code to a normal Singleton. However, those criticisms are
out there, and adding a feature to the language specifically in order to
support Singletons, in a very limited way, will be considered by some as
encouraging bad practice.
Regards,
--
Rowan Collins
[IMSoP]
Am 15.02.2015 23:34 schrieb "Rowan Collins" rowan.collins@gmail.com:
You can surely see how this is more readable / easier to write:
$value = MyRedir::get($key);
Actually, no, I find that harder to read accurately - there is no clue
there that there is actually a singleton under the hood, and that this is
actually an instance method in disguise.
Fair enough. You judge readabilidy by "an unaccustomed reader can grasp
(better or worse) what lies behind this construct". I have a different
focus, as I'm just a herder of a small group of internal developers.
The nice thing about an explicit Singleton is you can migrate to
Dependency Injection (call "$redis = MyRedis::singleton()" and start
passing the instance around,I can do that with my redis class, where I need it, by calling a
method MyRedis::link() which just returns $this. But the places where I
need that are rare.You may be able to do it with your current implementation, but how do you
propose to do it with an invisible singleton property implicitly created by
__constructStatic?
Without any caller side change, calling this already existing instance
method:
public function link() { return $this; }
OK, but you need a new subclass every time, not just a new method, or new
parameter; that limits your options. For instance, you can't dynamically
create instances based on configuration of a reusable library, which could
be as simple as adding an optional parameter to a standard Singleton:
Nothing would stop me from writing and using such a subclass, while having
fixed (in code) configuration sibling classes. Or add such a
fixed-config-singleton class method to the base class to add that to the
whole hierarchy. I don't have the need. But I could.
Any function that already uses the "$redis = MyRedis::inst();" pattern
can then quickly be changed to "$redis = MyRedis::inst('volatile');" and
continue to work fine.
Or, in my case, s/MyRedis::/MyRedisReg::inst('volatile')->/g. Piece of cake.
"Extend" was perhaps not the word I should have chosen; I was thinking of
"extending the use of the class beyond the current coding pattern". By
implementing one specific use case as a magic method, you make it harder to
do anything other than that one use case; or, you end up creating a whole
set of magic methods and features, which are then no easier than just
implementing the whole thing by hand.
Nothing of that. By base class is fully flexible! It even throws an
exception when a coder tries to singleton-like use it itself - it's just
the concrete subclasses that can be used that way.
I consciously chose this nonextensibility. Of course this is (for the given
fanout of different redis servers) nothing that a public facing library
would use! I'm watching over a four man coder show writing a single web
site's code not indented for public consumption. And there, such things not
only make sense, they happen all over the place.
Anyway, I'm pretty sure that singleton subclasses are useful in various
other one-of-a-few-specializations cases for completely different uses, and
there the patterrn might just make sense, too.
I certainly understand your criticism, from a point of view of more
publically consumable codebases / libraries. Thanks for your feedback.
best regards
Patrick
Patrick Schaaf wrote:
Am 15.02.2015 21:05 schrieb "Rowan Collins" rowan.collins@gmail.com:
// Now wherever in the code you want the default instance, just use this:
$value = MyRedis::singleton()->get($key);You can surely see how this is more readable / easier to write:
$value = MyRedir::get($key);
It's possible to add static methods or use __callStatic() to delegate to
the singleton's instance methods, though.
--
Christoph M. Becker
create the static instance
Isn't that essentially a contradiction in terms? I can't help but feel
that blurring the line between static and non-static classes/methods would
cause more harm than good.
I've never really done any work with Redis before so I'm also having
trouble understanding the use case for this given that everybody's talking
about this solely in the context of Redis.
--Kris
Am 16.02.2015 02:40 schrieb "Kris Craig" kris.craig@gmail.com:
I've never really done any work with Redis before so I'm also having
trouble understanding the use case for this given that everybody's talking
about this solely in the context of Redis.
Ahem, it's not everbody, just me :) And the issue comes up in the context
of redis because persistent connection without keeping around yet another
global $myglobalredis, and
ease of use to me in that concrete case. Yeah. I had a use case. That's
all. I'll rest my case soon if there are no others.
best regards
Patrick
Kris Craig wrote on 16/02/2015 01:40:
create the static instance
Isn't that essentially a contradiction in terms? I can't help but
feel that blurring the line between static and non-static
classes/methods would cause more harm than good.I've never really done any work with Redis before so I'm also having
trouble understanding the use case for this given that everybody's
talking about this solely in the context of Redis.
It's basically like any database connection - in a simple app, you just
need a single connection that you can run lots of queries against, so
global/singleton access is very convenient.
I do see the use case, but am not sure it's compelling enough to add a
language feature, given that a similar effect can be achieved using
existing features.
Regards,
Rowan Collins
[IMSoP]
By neccessity the implementation of this class set, must make use of both
__call() and __callStatic() magic methods, with both then dispatching to a
delegate phpredis instance, and in the case of __callStatic(), making
up-front a singleton like "new self" once. For a set of additional helper
methods (not present on the underlying phpredis) I additionally have a
special mapping array of real method names to internal implementation names.
A quick thought - if you want to stick with the "magic static call"
pattern, you can implement this much more simply by doing something
similar to Laravel's "facades" [1]:
class MyRedis extends Redis {
// extra instance methods here
}
class MyRedisFacade {
private static $instance;
public static function __callStatic($method, $params) {
if ( is_null(self::$instance) ) {
self::$instance = new MyRedis;
}
self::$instance->$method(...$params);
}
}
This basically implements in userspace what you propose to add to the
language, with the only caveat being that you must separate the concerns
of adding extra functionality, and wrapping it in a static facade, into
two separate classes.
[1] http://laravel.com/docs/4.2/facades
--
Rowan Collins
[IMSoP]
Am 16.02.2015 00:05 schrieb "Rowan Collins" rowan.collins@gmail.com:
A quick thought - if you want to stick with the "magic static call"
pattern, you can implement this much more simply by doing something similar
to Laravel's "facades" [1]:
...
This basically implements in userspace what you propose to add to the
language, with the only caveat being that you must separate the concerns of
adding extra functionality, and wrapping it in a static facade, into two
separate classes.
That was one of my intermediate steps before reaching the current approach,
but it irked me that when switching code from MyRedis static/singleton
usage to real instance usage I suddenly have to write a different class. In
my scheme, where I can use red_global::get() I can as well use $redis = new
red_global(); $redis->get(); without thinking about how that companion
class was named.
Sure, no big issue. But a small one... By itself, surely not enough reason
for a language change :)
It's all a question of how many other small such uses there would be.
best regards
Patrick