PHP class public, private en protected

@Rob

Het hangt er vanaf hoe strict je je handhaving wil toepassen.

Als jij het prima vindt dat iemand dit kan doen ...

<?php

$person = new Person();
$person->age = 'Rob';
$person->name = 30;

?>
Dan kun je met public properties werken. Als je er achteraf achterkomt dat je toch een vorm van validatie wil gaan toepassen, hoe weet je dan welke property het betreft in je magic set method?

Dan verbouw ik intern de public $age naar protected $_age:

<?php

class Person extends BaseObject{
  protected $_age = null;

  protected function getAge(){
    return $this->_age;
  }

  protected function setAge($value){
    if(!is_numeric($value) || ($value < 0)) throw new \Exception('Not a valid age');
    $this->_age = $value;
  }
}

?>

De invulling van __get() en __set() in BaseObject zorgen er dan voor dat de calls bij getAge() en setAge() uitkomen (zoals ik al zei: overhead).

Voor "gebruikers" van dit object (ook intern) blijft alles dan gelijk.

<?php

$person = new Person();
$person->age  = 'Rob'; //throws error
$person->name = 30;

?>

Omgekeerd kan je de magic setter ook gebruiken om fouten in je code op te sporen, bv.:

<?php
class basisobject {
  function __set(string $eigenschap, $waarde) : void {
    trigger_error(get_class($this) . '->'. $eigenschap . ' is niet gedefineerd');
  }
}

class mijnobject  extends basisobject {
  public $email;
  function __construct() {$this->emails = 1;} 
}

$object = new mijnobject;  // geeft error
?>

EDIT: naar aanleiding van Ward zijn opmerking even de code aangepast dat het goed fout gaat :)
En ik kom er net achter dat de magic setter niet meer nodig is in PHP8, dan krijg je sowieso een waarschuwing bij het benaderen van vanalles wat je niet eer hebt gedefineerd:
https://github.com/php/php-src/blob/php-8.0.0RC3/UPGRADING

<?php
class Foo
{
    public int $email;
  
    function __construct()
    {
        $this->$emails = 1;
    }
}

$foo = new Foo();
?>

Dat geeft sowieso al een fout:

Notice: Undefined variable: emails in ... on line 9

Rob Doemaarwat op 04/11/2020 14:12:38

De invulling van __get() en __set() in BaseObject zorgen er dan voor dat de calls bij getAge() en setAge() uitkomen (zoals ik al zei: overhead).

De vraag is inderdaad of je dit moet willen.

Ward van der Put op 02/11/2020 13:31:56

Plaatje werkt niet meer? Upload hem anders even bij ImgBB.com.

Ik ben sowieso een beetje allergisch voor getters en setters, al helemaal de magic variant. Het heeft zeker zijn plaats maar ik ben altijd terughoudend met een object volgooien met getDitDing() en setDitAndereDing().

Dit hele betoog is een kwestie van smaak maar naar mijn mening verdwijnt daarmee het expliciete gedrag van je code en daarmee de leesbaarheid.
Alles in je codebase, maakt niet uit waar, kan middels zo'n getter/setter een object ophalen uit de databse, deze muteren en weer opslaan. Dat hoeft geen probleem te zijn maar het maakt dingen als interne validatie en vervolgacties soms lastig te begrijpen.

Bijvoorbeeld dit stukje (psuedocode), dat kun je overal plakken en het werkt zonder morren:

<?php
$admin = $db->findUserByName('admin');
$admin->setPassword('test123');
$db->save($admin);
?>

Als je het op een meer expliciete manier bouwt word duidelijker wat je wil doen. Dan is ook duidelijk dat daar wat validatie plaatsvind in je model (model is een laag, niet een enkel object).

<?php
$admin = $db->findUserByName('admin');
try {
	$admin->changePassword('test123');
	$admin->saveChanges();
catch (PasswordException $e) {
	// Foutafhandeling
}?>

Dat kun je vervolgens zo ver uitbouwen als je wil, bijvoorbeeld met een command/query model maar dat gaat voor de meeste kleine applicaties veel te ver.

Punt is dat als je operaties/mutaties op je model (en eigenlijk alle methods in je codebase) expliciet maakt getters en setters niet nodig zijn en het gedrag je code veel leesbaarder word.
In bovenstaande voorbeelden is het logischer dat er bij changePassword() meer komt kijken dan alleen het setten van een nieuwe waarde, bijvoorbeeld een "your password was changed" mailtje versturen of lengte/complexiteit valideren.

Mocht iemand geïnteresseerd zijn kan ik wel een uitgebreid voorbeeld op GitHub o.i.d. plaatsen

Als $admin = $db->findUserByName('admin'), dan verwacht je bij $admin->changePassword('test123') een databaseoperatie of andere objectmutatie. Dat er plotseling ergens ook een e-mail uitgaat, is een ongewenst neveneffect. Niet onlogisch misschien, maar wel onzichtbaar en misschien ook niet altijd nodig maar je kunt het mailen hier niet meer verhinderen.

Daar is niets expliciet aan, maar een impliciete bron van lastig te vinden bugs: waar komt nou toch die mail vandaan?

Een soortgelijk probleem introduceer je hier, maar dan omgekeerd: er gebeurt juist te weinig.

<?php
$admin = $db->findUserByName('admin');
$admin->changePassword('test123');
$admin->saveChanges();
?>

Kennelijk wijzigt changePassword() het wachtwoord niet, want daarvoor is aansluitend nog een saveChanges() nodig. En die gaat iemand vergeten of op de verkeerde plaats te vroeg of te laat aanroepen.

Eigenlijk is changePassword() dus meer een setter (wanneer we even vergeten en vergeven dat die stiekem ook nog een verborgen dubbele functie als password mailer heeft). Dan is dit juist explicieter:

<?php
$admin = $db->findUserByName('admin');
$admin->setPassword('test123');
$db->save($admin);
?>

@Thom nvt

Ik ben het eens met Ward. Code die uit zichzelf een mailtje verstuurt is zeer onwenselijk. Iemand die de code leest of ermee aan de slag gaat, ziet niet dat er een mail wordt verstuurd. Alleen degene die de betreffende functie changePassword heeft geschreven, is hiervan op de hoogte. Daarnaast moet je ervoor zorgen dat een functie maar 1 ding doet. Moet er nog iets anders gebeuren? Maak dan een nieuwe functie daarvoor.

In plaats van een set of change, kun je ook kiezen voor update. Dan is gelijk duidelijk dat het wachtwoord in de database wordt geüpdatet. Ook is het vreemd hoe jij hier een admin ophaalt en blijkbaar op voorhand op basis van de username al weet dat het een admin is. Is er maar 1 admin in je systeem? Haal die dan op met een aparte functie, of (logischer) gebruik $user als variabelenaam.

<?php

$user = new User($id); // $id wordt bepaald aan de hand van gebruikersnaam/wachtwoord
$user->updatePassword('blabla');
$user->mailCredentials();

?>
That's it. Hoe duidelijk wil je het hebben.

Ook die try en catch die jij erbij zet zogenaamd voor de duidelijkheid ... die zet je normaliter in je class (eenmalig) of overkoepelend in je framework. Maar waarom zou je die in je normale code flow zetten? Dat moet je dan iedere keer herhalen en dat wil je juist niet. Je moet het jezelf zo makkelijk mogelijk maken, waarvan bovenstaande code een voorbeeld is.

Fair enough, op basis van bovenstaand (gebrekkig) voorbeeld ben ik het wel met jullie eens, daar had ik wat meer moeite in mogen stoppen.

Het is lastig om dit principe goed uit te leggen in één enkele forumpost, er komt nog heel wat meer bij kijken.
Het is gebaseerd op layered architecture/DDD en event-driven applicaties en daar zijn hele boeken over vol geschreven ("DDD in PHP" is een echte aanrader als je met complexe applicaties en business-cases werkt).

Het punt van expliciet maken gaat wel op, het voorbeeld van Ozzie is daarin wat beter neergezet.
Ik probeer de namen van functies en classes altijd zo dicht mogelijk op de realiteit te houden, je code is immers een model (vereenvoudiging) van de werkelijkheid.
In de "echte wereld" stel je bijvoorbeeld niet een wachtwoord in op een persoon. Je wijst hem toe of hij/zij wijzigt hem. "set" komt niet voor en is ambigu. Is het de eerste toewijzing? Een wijziging? Door wie mag dit gedaan worden?
Als je die business verantwoordelijkheid opsplitst in "changePassword" en "assignPassword" kun je bijvoorbeeld verschillende permissie-checks doen, beter een audit-trail bijhouden en, onder bepaalde voorwaarden, makkelijker debuggen.

Zoals gezegd had mijn voorbeel wat beter/uitgebreider kunnen zijn maar ik vind het lastig om dit soort principes kort en bondig uit te leggen.

De exception-handling in de normale code flow is een heel andere discussie die ik nu niet ga aanbreken.