is_callable() und private Methoden
Stefan Froehlich
| Call to private Incoterm::__clone() from context 'Collection'
Der Ausloeser dafuer fand sich in der clone-Funktion meiner
Collection, die (offenbar erfolglos) versucht, automatisch zwischen
Multitons und duplizierbaren Objekten zu unterscheiden:
| public function __clone() {
| if (count($this->items) > 0) {
| $item = reset($this->items);
| if (is_callable(array($item, '__clone'))) {
| $temp = array();
| foreach ($this->items as $key => $item) {
| $temp[$key] = clone $item;
| }
| $this->items = $temp;
| }
| }
| }
Dazu muss ich noch ergaenzen, dass alle Multitons, so auch die
Klasse "Incoterm", auf einer gemeinsamen Basisklasse beruhen, in der
__clone() als privat deklariert ist und bei Aufruf aus der eigenen
Klasse heraus eine Exception wirft.
Setze ich nun in den Code hinter die 3. Zeile die Anweisungen:
| echo phpversion();
| var_dump($item);
| var_dump(is_callable(array($item, '__clone')));
...kommt das erwartete, aber falsche Ergebnis:
| 5.3.3-7
| object(Incoterm)#287 (3) {...}
| bool(true)
Dass es falsch ist, merkt man dann kurze Zeit spaeter an der
Fehlermeldung. Nun habe ich versucht, das Beispiel auf einen
Trivialfall herunterzubrechen, d.h.:
| <?php
|
| abstract class A {
| private function __clone() { throw new Exception('failed'); }
| }
|
| class B extends A { }
|
| $b = new B();
| var_dump(is_callable(array($b, '__clone')));
|
| ?>
Dummerweise kommt HIER aber vollkommen korrekt:
| bool(false)
heraus. Und nun? Der einzige Unterschied ist: der Echtfall laeuft
ueber den Apache, der Trivialfall ueber CLI. Hat jemand irgendeine
Idee, was ich da noch uebersehen haben koennte?
Servus,
Stefan
--
http://kontaktinser.at/ - die kostenlose Kontaktboerse fuer Oesterreich
Offizieller Erstbesucher(TM) von mmeike
Stefan - der fiesste Unsinn, den es je gab.
(Sloganizer)
Helmut Chang
> Gerade eben ist mir mein Programm um die Ohren geflogen:
> | Call to private Incoterm::__clone() from context 'Collection'
>
> Der Ausloeser dafuer fand sich in der clone-Funktion meiner
> Collection, die (offenbar erfolglos) versucht, automatisch zwischen
> Multitons und duplizierbaren Objekten zu unterscheiden:
>
> | public function __clone() {
> | if (count($this->items)> 0) {
> | $item = reset($this->items);
> | if (is_callable(array($item, '__clone'))) {
Und du kannst sicherstellen, dass alle Elemente von $this->items
denselben Typ haben? Weil du prüfst ja nur das erste Element.
> | $temp = array();
> | foreach ($this->items as $key => $item) {
> | $temp[$key] = clone $item;
> | }
> | $this->items = $temp;
> | }
> | }
> | }
>
> Dazu muss ich noch ergaenzen, dass alle Multitons, so auch die
> Klasse "Incoterm", auf einer gemeinsamen Basisklasse beruhen, in der
> __clone() als privat deklariert ist und bei Aufruf aus der eigenen
> Klasse heraus eine Exception wirft.
Za wos denn das? Wäre es nicht sinnvoller, wenn deine Multitons ein
ICloneable implementieren (oder auch nicht, je nach dem...)?
Und was ist jetzt mit Incoterm? Überschreibt das __clone() oder nicht?
Ganz klar ist mir jetzt nicht, was passieren soll...
Im Zusammenhang, was dein Code tut (oder tun sollte)
-> du hast eine Collection, die cloneable sein soll
-> die Elemente der Collection können nun cloneable sein oder auch
nicht.
-> sind die Elemente nicht cloneable (wobei du davon ausgehst, dass
entweder alle oder kein Element cloneable ist), wird ein Clon der
Collection erstellt, der leer ist??
Im Prinzip ist dir dein Programm um die Ohren geflogen, weil du versucht
hast, eine Collection zu clonen, die gar nicht clonebar ist, weil deren
Elemente nicht clonebar sind. Aaaber...:
> Setze ich nun in den Code hinter die 3. Zeile die Anweisungen:
>
> | echo phpversion();
> | var_dump($item);
> | var_dump(is_callable(array($item, '__clone')));
>
> ...kommt das erwartete, aber falsche Ergebnis:
>
> | 5.3.3-7
> | object(Incoterm)#287 (3) {...}
> | bool(true)
Wieso erwartest du dieses Ergebnis, wenns doch falsch ist? Und was genau
ist jetzt falsch? Das Incoterm::__clone() callable ist (nehm ich an...)?
Das sähe dann tatsächlich u. U. nach einem Bug aus...
> Dass es falsch ist, merkt man dann kurze Zeit spaeter an der
> Fehlermeldung. Nun habe ich versucht, das Beispiel auf einen
> Trivialfall herunterzubrechen, d.h.:
>
> |<?php
> |
> | abstract class A {
> | private function __clone() { throw new Exception('failed'); }
> | }
> |
> | class B extends A { }
> |
> | $b = new B();
> | var_dump(is_callable(array($b, '__clone')));
> |
> | ?>
>
> Dummerweise kommt HIER aber vollkommen korrekt:
>
> | bool(false)
>
> heraus.
Gut...
> Und nun? Der einzige Unterschied ist: der Echtfall laeuft
> ueber den Apache, der Trivialfall ueber CLI.
...najo, nicht ganz. Der Kontext ist ein anderer... Und das macht bei
PHP oft etwas aus ;). Was passiert, wenn du den Trivialfall über Apache
laufen lässt? Kannst du den Fall mit der Collection übers CLI testen?
Ansonsten sehe ich trotzdem den Sinn der privaten
__clone()-Implementierung in der Basisklasse nicht...
Gruß, Helmut
Stefan Froehlich
> > | public function __clone() {
> > | if (count($this->items)> 0) {
> > | $item = reset($this->items);
> > | if (is_callable(array($item, '__clone'))) {
> Und du kannst sicherstellen, dass alle Elemente von $this->items
> denselben Typ haben? Weil du prüfst ja nur das erste Element.
In der Collection schon, ja. Frueher habe ich einfach darauf vertraut.
> > Dazu muss ich noch ergaenzen, dass alle Multitons, so auch die Klasse
> > "Incoterm", auf einer gemeinsamen Basisklasse beruhen, in der __clone()
> > als privat deklariert ist und bei Aufruf aus der eigenen Klasse heraus
> > eine Exception wirft.
> Za wos denn das?
Weil derartige Objekte prinzipiell nicht clonebar sind/sein duerfen. Da
moechte ich, dass mir das Programm um die Ohren fliegt, sollte es
versehentlich doch einmal passieren.
> Im Prinzip ist dir dein Programm um die Ohren geflogen, weil du versucht
> hast, eine Collection zu clonen, die gar nicht clonebar ist, weil deren
> Elemente nicht clonebar sind.
Ah, nein: die Collection ist sehr wohl clonebar, nur muss sie auf die
gleichen Objektinstanzen referenzieren, wie das Original. Bei anderen
Objekten (die keine Multitons sind) ist das hingegen nicht der Fall.
> > | echo phpversion();
> > | var_dump($item);
> > | var_dump(is_callable(array($item, '__clone')));
> > ...kommt das erwartete, aber falsche Ergebnis:
> > | 5.3.3-7
> > | object(Incoterm)#287 (3) {...}
> > | bool(true)
>
> Wieso erwartest du dieses Ergebnis, wenns doch falsch ist? Und was genau
> ist jetzt falsch?
Weil's halt dem (unerwarteten) Verhalten des Programms entspricht.
> > Und nun? Der einzige Unterschied ist: der Echtfall laeuft
> > ueber den Apache, der Trivialfall ueber CLI.
> ...najo, nicht ganz. Der Kontext ist ein anderer... Und das macht bei PHP
> oft etwas aus ;)
Klar - die Schwierigkeit lag in diesem Fall darin, das Original in seiner
Komplexitaet so weit zu reduzieren, dass ich den Code in gleichem Kontext
ueberhaupt via CLI aufrufen konnte.
Und siehe da - der Fehlerteufel ist inzwischen gefunden. Es gab frueher
einmal neben der privaten __clone()-Implementierung eine ebensolche
__call()-Implementierung (die gleichermassen eine Exception wirft und von
Klassen, die __call() tatsaechlich benoetigen explizit ueberschrieben
werden muss).
Seit dem PHP-Update vorige Woche meckert PHP aber "private __call()" als
unzulaessig an, weshalb ich die Signatur auf "public __call()" geaendert
habe.
Nun liest sich das Trivialbeispiel also so:
| abstract class A {
| private function __clone() { throw new Exception('failed'); }
| public function __call($method, Array $parameters) { throw new Exception('failed'); }
| }
|
| class B extends A { }
|
| $b = new B();
| var_dump($b);
| var_dump(is_callable(array($b, '__clone')));
...und (was mir nicht bewusst war) __call() ersetzt nicht nur fehlende
Methoden, sondern auch solche mit unzureichenden Zugriffsrechten. Ist
in dem Fall halt ein bisserl unpraktisch...
Da __call() im Gegensatz zu __get() und __set() bei Abwesenheit aber nicht
automatisch von PHP mit Inhalt befuellt wird, kann ich (im Gegensatz zu den
beiden anderen) zum Glueck aber auch ganz gut darauf verzichten.
Servus,
Stefan
--
http://kontaktinser.at/ - die kostenlose Kontaktboerse fuer Oesterreich
Offizieller Erstbesucher(TM) von mmeike
Stefan - das Gefühl zu schwanken!
(Sloganizer)
Stefan Froehlich
> __call() ersetzt nicht nur fehlende Methoden, sondern auch solche mit
> unzureichenden Zugriffsrechten. Ist in dem Fall halt ein bisserl
> unpraktisch...
...und macht is_callable(), wie mir gerade an anderer Stelle aufgefallen
ist, zu einer sehr bedenklichen Geschichte. Was immer man damit
implementiert: kommt spaeter ein einziger Anwendungsfall fuer __call()
hinzu, bricht alles in sich zusammen - selbst wenn __call() nur einige
wenige Methodennamen in voellig anderem Kontext abfangen soll.
Servus,
Stefan
--
http://kontaktinser.at/ - die kostenlose Kontaktboerse fuer Oesterreich
Offizieller Erstbesucher(TM) von mmeike
Stefan - Als ob man dies immer wieder durchkauen müßte!
(Sloganizer)
Ulf K@dner
> Was immer man damit
> implementiert: kommt spaeter ein einziger Anwendungsfall fuer __call()
> hinzu, bricht alles in sich zusammen - selbst wenn __call() nur einige
> wenige Methodennamen in voellig anderem Kontext abfangen soll.
__call, __get, __set, etc. sollte man ohnehin nur dann einsetzen wenn es
keine andere sinnvolle Möglichkeit gibt das ohne selbige umzusetzen.
Nicht zuletzt weil dadurch eine extrem dynamische nicht genauer
offensichtlich definierte Schnittstelle entsteht, die halt u.a. zu
Deinen Problemen führen kann. Ich habe gerad mal über mein Framework
geschaut. __call kommt darin (ca. 250000 Loc) genau 1x vor.
Ich habe letztendlich darauf verzichtet weil damit extrem
unübersichtlicher Code entsteht. Da muss man jedesmal dermaßen viel Doku
schreiben und trotzdem bleibts unübersichtlich und wässrig was die
mögliche Schnittstelle angeht.
Würde ich an Deiner Stelle auch wieder so weit wie möglich rausschmeißen.
MfG, Ulf
Stefan Froehlich
> __call, __get, __set, etc. sollte man ohnehin nur dann einsetzen wenn es
> keine andere sinnvolle Möglichkeit gibt das ohne selbige umzusetzen.
Ich hab's, so wie Du, genau an einer Stelle in Verwendung - und dort nimmt
es mir _sehr_ viel Arbeit ab. Ansonsten waren:
> Würde ich an Deiner Stelle auch wieder so weit wie möglich rausschmeißen.
die Deklarationen "private __set()" und "private __get()" aus genau
dem Grund vorhanden, um versehentliches Aufrufen davon zu
unterbinden - im Gegensatz zu __call() sind die ja implizit
vorhanden (oder kann man das inzwischen irgendwo in der
Konfiguration abstellen?). "private __call()" kam dann eher der
Vollstaendigkeit halber noch mit dazu.
Was der Grund dafuer war, dass das mit PHP5.3 nun nicht mehr
zulaessig ist, entzieht sich meiner Kenntnis. Ich halte es
jedenfalls fuer unpraktisch.
Servus,
Stefan
--
http://kontaktinser.at/ - die kostenlose Kontaktboerse fuer Oesterreich
Offizieller Erstbesucher(TM) von mmeike
Stefan - Das isses!
(Sloganizer)
Ulf K@dner
> Ich hab's, so wie Du, genau an einer Stelle in Verwendung - und dort nimmt
> es mir _sehr_ viel Arbeit ab.
OK
>> Würde ich an Deiner Stelle auch wieder so weit wie möglich rausschmeißen.
>
> die Deklarationen "private __set()" und "private __get()" aus genau
> dem Grund vorhanden, um versehentliches Aufrufen davon zu
> unterbinden
Inwiefern? __get usw. sollte ohnehin nicht direkt aufgerufen werden.
Wenn DU vermeiden willst das in Ableitungen __get & Co. implementiert
werden hilft Dir final wass OOP-Technisch ohnehin der sauberere Weg ist:
public final function __get( $fieldName )
{
throw new \Exception(
'Dynamic fields are not supported by class "' .
__CLASS__ . '"!');
}
> Was der Grund dafuer war, dass das mit PHP5.3 nun nicht mehr
> zulaessig ist, entzieht sich meiner Kenntnis.
OK ich habe lange nicht mehr in der PHP-Entwickler Mailingliste gelesen,
aber ich würde darauf tipen das damit letztendlich einer saubereren
OOP-Umsetzung Rechnung getragen wurde. Überraschend finde ich es nicht.
MfG, Ulf
Helmut Chang
>>> Dazu muss ich noch ergaenzen, dass alle Multitons, so auch die Klasse
>>> "Incoterm", auf einer gemeinsamen Basisklasse beruhen, in der __clone()
>>> als privat deklariert ist und bei Aufruf aus der eigenen Klasse heraus
>>> eine Exception wirft.
>
>> Za wos denn das?
>
> Weil derartige Objekte prinzipiell nicht clonebar sind/sein duerfen.
Ahja, mir war irgendwie jetzt nicht (mehr) bewusst, dass in PHP Objekte
immer klonbar sind, egal ob __clone() implementiert ist oder nicht.
> Da
> moechte ich, dass mir das Programm um die Ohren fliegt, sollte es
> versehentlich doch einmal passieren.
Was in deinem ursprünglichen Beispiel ja eben passiert, aber halt nicht
ganz kontrolliert...
Ich behaupte jetzt nicht, klüger zu sein als du. Ich hätte nur die
gesamte Problematik anders gelöst:
Bei deiner Lösung mit der privaten __clone()-Methode sieht man dem
Objekt von "außen" nicht an, ob es klonebar ist oder nicht. Denn du
siehst ja von außen nicht, ob es diese Methode in der Elternklasse gibt.
Du kannst es zwar mit is_callable ermitteln, aber anscheinend ist das
auch recht fehlerträchtig...
Meine Lösung sieht in etwa so aus:
Es gibt bei mir eine absolute Basisklasse "Object". Das kann aber
genauso ja deine Mutliton-Basisklasse sein. Da drin gibt es eine
öffentliche Methode __clone():
class Object {
final public function __clone() {
}
}
Weiters gibt es das Interface "ICloneable". Das ist zwar bei mir leer,
liefert aber den Hinweis, dass das Objekt klonbar ist.
Damit sieht die tatsächliche Implementierung von Object so aus:
class Object {
final public function __clone() {
if (!($this instanceof ICloneable))
throw new Excpetion('Cannot clone ...');
$this->_specificClone();
}
protected function _specificClone() {
// Override in concrete implementations...
}
}
Wenn du jetzt noch __clone() eine saubere Dokumentation verpasst, sollte
jedem Anwender der Klasse klar sein, dass er vor dem klonen prüfen muss
und wie. Deine __clone()-Implementierung in der Collection würde dann so
aussehen:
public function __clone() {
if (count($this->items) > 0) {
$item = reset($this->items);
if ($item instanceof ICloneable) {
$temp = array();
foreach ($this->items as $key => $item)
$temp[$key] = clone $item;
$this->items = $temp;
}
}
}
oder
public function __clone() {
if (count($this->items) > 0) {
$item = reset($this->items);
try {
$temp = array();
foreach ($this->items as $key => $item)
$temp[$key] = clone $item;
$this->items = $temp;
} catch { }
}
}
Sieht zwar nicht sooo viel anders aus wie deine Lösung. Aber hier legt
die Schnittstelle des Objekts fest, ob dieses geklont werden kann oder
nicht, und das ist IMHO besser.
Aber ich stelle das gern zur Diskussion und behaupte nicht, das sei die
beste oder einzig richtige Lösung ;).
> Ah, nein: die Collection ist sehr wohl clonebar, nur muss sie auf die
> gleichen Objektinstanzen referenzieren, wie das Original. Bei anderen
> Objekten (die keine Multitons sind) ist das hingegen nicht der Fall.
Ok, nicht mitgedacht meinerseits...
> Nun liest sich das Trivialbeispiel also so:
>
> | abstract class A {
> | private function __clone() { throw new Exception('failed'); }
Das sieht irgendwie komisch aus... ;). Vielleicht übersehe ich ja etwas,
aber ist es nicht so, dass __clone() in diesem Fall ohnehin nie
aufgerufen werden kann?
> | public function __call($method, Array $parameters) { throw new Exception('failed'); }
> [...]
>
> ...und (was mir nicht bewusst war) __call() ersetzt nicht nur fehlende
> Methoden, sondern auch solche mit unzureichenden Zugriffsrechten. Ist
> in dem Fall halt ein bisserl unpraktisch...
Allerdings!
> Da __call() im Gegensatz zu __get() und __set() bei Abwesenheit aber nicht
> automatisch von PHP mit Inhalt befuellt wird,...
Ich nehme an, du meinst damit so etwas:
class A {
}
$a = new A();
$a->a = 'Foo';
Da finde ich, dass PHP ohnehin ein ziemlich inkonsistentes Verhalten zeigt:
abstract class A {
private $a;
}
class B extends A {
}
$b = new B();
$b->a = 'Foo';
var_dump($b);
// object(B)#1 (2) {
// ["a":"A":private]=>
// NULL
// ["a"]=>
// string(3) "Foo"
// }
B::$a = 'foo';
// Fatal error: Cannot access property B::$a in...
B::$b = 'foo';
// Fatal error: Access to undeclared static property: B::$b in...
Gruß, Helmut
Stefan Froehlich
On Mon, 28 Feb 2011 08:33:05 Helmut Chang wrote:
> >>> dass alle Multitons, [...] auf einer gemeinsamen Basisklasse
> >>> beruhen, in der __clone() als privat deklariert ist
> >> Za wos denn das?
> > Weil derartige Objekte prinzipiell nicht clonebar sind/sein
> > duerfen.
> Ahja, mir war irgendwie jetzt nicht (mehr) bewusst, dass in PHP
> Objekte immer klonbar sind, egal ob __clone() implementiert ist
> oder nicht.
Ja. Mir ist das auch erst aufgefallen, als das Kind das erste Mal in
den Brunnen gefallen ist. Ueber __get und __set stolpert man in der
Regel schneller.
> Ich behaupte jetzt nicht, klüger zu sein als du. Ich hätte nur die
> gesamte Problematik anders gelöst:
Mhm, habe ich inzwischen ohnehin (muessen). Das alte System stammte
halt aus einer Zeit, wo ich noch kein durchgehendes Konzept fuer
Exceptions hatte - die Deklaration als "private" erschien mir damals
als konsequente Fortfuehrung des Singleton-Konzepts mit privatem
Konstruktor (letztendlich ist __clone dem Konstruktor ja sehr
aehnlich).
> [...] Damit sieht die tatsächliche Implementierung von Object so
> aus:
> class Object {
>
> final public function __clone() {
> if (!($this instanceof ICloneable))
> throw new Excpetion('Cannot clone ...');
>
> $this->_specificClone();
> }
>
> protected function _specificClone() {
> // Override in concrete implementations...
> }
> }
Dumme Frage zum Tag: ist die Deklaration als "final" samt der
Auslagerung nach _specificClone() auch die Folge einer historischen
Entwicklung, oder hat das einen bestimmten Grund? Waere __clone
nicht als final deklariert, koennte man es direkt ueberschreiben,
anstatt das mit _specificClone() tun zu muessen. Ad hoc sehe ich
darin keine grossen Nachteile.
> Deine __clone()-Implementierung in der Collection würde dann so
> aussehen:
> [...]
> if ($item instanceof ICloneable) {
> $temp = array();
> foreach ($this->items as $key => $item)
> $temp[$key] = clone $item;
>
> $this->items = $temp;
> }
Hm, ja. Das erscheint mir jedenfalls besser zu sein, als der
Workaround ueber Reflection, den ich mir ad hoch einmal gebastelt
habe.
> try {
> $temp = array();
> foreach ($this->items as $key => $item)
> $temp[$key] = clone $item;
>
> $this->items = $temp;
> } catch { }
Ist auch nett, aber vermutlich langsamer, als die Abfrage auf ein
unterstuetztes Interface.
> Sieht zwar nicht sooo viel anders aus wie deine Lösung.
Doch, doch - und das beim entscheidenden Punkt :-)
> > | abstract class A {
> > | private function __clone() { throw new Exception('failed'); }
> Das sieht irgendwie komisch aus... ;). Vielleicht übersehe ich ja etwas,
> aber ist es nicht so, dass __clone() in diesem Fall ohnehin nie
> aufgerufen werden kann?
Wie schon weiter oben geschrieben: es hat sich historisch so
entwickelt. Und: auch aus der eigenen Klasse koennte __clone()
faelschlicherweise einmal aufgerufen werden. Ein Sicherheitsnetz
ist nie verkehrt.
> > Da __call() im Gegensatz zu __get() und __set() bei Abwesenheit
> > aber nicht automatisch von PHP mit Inhalt befuellt wird,...
> Ich nehme an, du meinst damit so etwas:
> class A {
> }
> $a = new A();
> $a->a = 'Foo';
Ja, genau. Ich weiss noch, dass ich geschaut habe, wie ein Autobus,
als ich das erste Mal einen ganz banalen Tippfehler in einer meiner
Klassen (nach ewig langer Suche) gefunden hatte. Wenige Stunden
danach hatte ich die magischen Methoden mittels einer gemeinsamen
Basisklasse samt und sonders deaktiviert.
> Da finde ich, dass PHP ohnehin ein ziemlich inkonsistentes
> Verhalten zeigt:
> abstract class A {
> private $a;
> }
>
> class B extends A {
> }
>
> $b = new B();
> $b->a = 'Foo';
Hoppala... es gibt viele Moeglichkeiten, sich ins Knie zu schiessen.
Das ist eine davon.
Servus,
Stefan
--
http://kontaktinser.at/ - die kostenlose Kontaktboerse fuer Oesterreich
Offizieller Erstbesucher(TM) von mmeike
Himmlisch bleibt himmlisch: Trotz Stefan!
(Sloganizer)