OOP: Bad practice?

[ChadNomad]

Hi. I've been learning OOP and watched a

about a "secure" OOP PHP login script. It was pretty good, and helped me to understand some OOP approaches, but I couldn't help thinking some of it was wrong. I'm not sure so feedback appreciated!

 

The class starts of something like this:

<?php
class Login
{
  private $_id;
  private $_username;
  private $_password;
  private $_passmd5;

  private $_errors;
  private $_access;
  private $_login;
  private $_token;

  public function __construct()
  {
    $this->_errors = array();
    $this->_login  = isset($_POST['login'])? 1 : 0;
    $this->_access = 0;
    $this->_token  = $_POST['token'];

    $this->_id       = 0;
    $this->_username = ($this->_login)? $this->filter($_POST['username']) : $_SESSION['username'];
    $this->_password = ($this->_login)? $this->filter($_POST['password']) : '';
    $this->_passmd5  = ($this->_login)? md5($this->_password) : $_SESSION['password'];

  }

 

Isn't "hard" setting variables, like the POST vars in the contruct bad? Shouldn't they be passed through elsewhere?

 

I've learnt that OOP needs to be reusable and manageable as it's primarily the point of using OOP in the first place. I might be wrong but I noticed straight away that the above class doesn't seem reusable (in the true sense).

 

Hopefully i'm getting the hang of it...

 

Thanks

 

[gizmola]

Typically I'd expect to see a user class, with a login method.  There are way too many books about OOP out there, but one that focuses on "Design Patterns" might be helpful.

[rwwd]

There is a thing with php class engine that I think needs to be addressed, the example that you show would work ONLY in php5, and you would be surprised in how many servers out there are still running php4, and don't plan to upgrade till php6 comes out!

 

__construct() is used in essence to load things when the class is first instantiated, at least this is how I have always understood it to be, I use it to load in all of my controller array's and constants (excellent for multi language projects I find.. (see iL8n))

 

The only issue I have with that is that there is no need at ALL to pass $_POST or even $_GET data, or realistically ANY superglobal into a function, becasue by definition it is globally available throughout your script (SO long as it is set - has state), so the filter function can have the $_POST key removed (so long as the contents of filter() explicitly asks for that key).  Saying that though, you can use callback functions to sanitise the entire array with a chosen function, this is a great help, don't forget that ANY post data is technically a string UNTIL you typecast it - at least that is how I understand things to be.

 

Rw

[eran]

The only issue I have with that is that there is no need at ALL to pass $_POST or even $_GET data, or realistically ANY superglobal into a function,

There is a reason, and that is if you are not always expecting the data to come from a superglobal. If you pass in an array, you can create the same object using a form submission, from inside another object, from database data and so forth. By passing the data as an array instead of relying on it existing in superglobals you are increasing the re-usability of the class.

[rwwd]

^^

I repeat my last point, by definition super global is accessible anywhere so long as they are set, they can be accessed - therefore no need to pass them into a function. BUT if the programmer is going by assigned variable name this will then need to be a passed parameter - but this approach takes up unnecessary memory, so long as the data is sanitised correctly you can use the $_POST/$_GET/$_SESSION/$_COOKIE without the need to assign them to 'storage' variables....

 

Rw

[gizmola]

^^

I repeat my last point, by definition super global is accessible anywhere so long as they are set, they can be accessed - therefore no need to pass them into a function. BUT if the programmer is going by assigned variable name this will then need to be a passed parameter - but this approach takes up unnecessary memory, so long as the data is sanitised correctly you can use the $_POST/$_GET/$_SESSION/$_COOKIE without the need to assign them to 'storage' variables....

 

Rw

 

 

You seemed to miss the point of Eran's post entirely, which was that the specific use of the superglobs make the implementation inflexible and non reusable, not to mention, impossible to unit test with the available php unit testing frameworks (phpunit and simpletest).

[rwwd]

$this->filter($_POST['username'])

 

Could be:-

 

$this->filter('username')

 

Not necessary! I may be missing some point or other, but this is what I am referring to, superglobals need not be passed into functions. Not saying that they can't be, just that it is unnecessary to do so - what you can do is pass the 'key' into the function so that you can check it's value from reference, I accept that.

 

There is a reason, and that is if you are not always expecting the data to come from a superglobal.

My point was about the code I have quoted, I'm not disagreeing with you, I think I just need a top up from the coffee pot! My head aches....

 

I'm not wanting to argue, and apologies if I have missed the point, but so far as I have been taught, and to the best of my knowledge, and certainly from classes that I have written in the past, Superglobals need not be passed into functions as by definition they are globals - accessible anywhere so long as they are set/have state!

 

Just to add: I'm not saying that this practise is wrong, just not preferred if it can be helped.

 

Rw

[trq]

Superglobals need not be passed into functions as by definition they are globals - accessible anywhere so long as they are set/have state!

 

This would however to you classes explicitly to those global variables. Relying on global data is generally a poor design decision, which inevitability makes your code less flexible / reusable.

 

Functions / Methods should always ask for the data they need instead of it somehow magically appearing within them.

[rwwd]

Functions / Methods should always ask for the data they need instead of it somehow magically appearing within them.

 

I accept that, and that perspective of design made me think of my last class, maybe I shall revisit it and see if it is flawed for transportability, which it most likely is. But this is what learning and mistakes is for.

 

But I stand by my logic of the *suggestion* that superglobals need not be passed into functions as a parameter, this is how I sanitise data, I re write the array with callback, and as yet no complaints.

 

Rw

[ignace]

Typically I'd expect to see a user class, with a login method.

 

 

Typically I wouldn't because until the user successfully logs in we can't actually speak of a "User" as we know nothing about him only that he's a Guest/Visitor (a Special Case of "User" at best). In the login procedure we create a personification of the User on the system which leads me to believe that the login procedure should be handled by a Creational Pattern: like Factory (or Builder if the personification requires multiple steps as could be the case with an ACL).

 

That's of course the modeler inside of me, from an SRP-perspective a login() method on a User-class is of course perfectly fine.

[ignace]

@OP

 

1) Using globals inside functions/classes is bad practice.

2) Login::filter() is a violation of SRP (Single-Responsibility, and quite a few others: DRY for example) as I doubt you want to create an instance of Login class whenever you need to filter data.