[RESOLVED] Missing argument 2 for Validation

[cluelessPHP]

So I'm missing two arguments,

If I do this,

private 
	$password,
	$repeatPassword,
	$username,
	$email;

public static function validateRepeatPassword($password, $repeatPassword)
	{
		return $repeatPassword === $password;
	}

if I do this

public static function validateRepeatPassword($password, $repeatPassword = "")
	{
		return $repeatPassword === $password;
	}

The error is removed but the validation fails to work?

[NogDog]

What does the code that is actually calling the method look like? That is presumably where you are not using both arguments.

[cluelessPHP]

<?php  

class Validation 
{
	private 
	$password,
	$repeatPassword,
	$username,
	$email;


private static
$minPassword = 5,
$confirmPassword,
$minUsername = 3,
$maxUsername = 14,
$validEmail,
$alphaNumericID = "/^[A-Za-z0-9]*$/";

public static function validateEmail($email)
{	
	return (filter_var($email, FILTER_VALIDATE_EMAIL) != self::$validEmail);
}

public static function validatePassword($password)
{
	return(strlen($password) >= self::$minPassword);
}

public static function validateRepeatPassword($password, $repeatPassword = "")
{
	return $repeatPassword === $password;
}

public static function validateUsername($username)
	{
		return strlen($username) >= self::$minUsername 
		&& (strlen($username)) <= self::$maxUsername 
		&& ($username) ===  self::$alphaNumericID;
	}
}

That's it currently, having an argument with !==, ==, === ect. on one or two of them

[NogDog]

That type of error would be caused when you try to actually use that method. It's not saying there's anything wrong with how the method is defined, but how you are calling it.

// would expect that error:
Validation::validateRepeatPassword($foo);

// would not expect that error:
Validation::validateRepeatPassword($foo, $bar);

[cluelessPHP]

So I need to figure out how to put this..

http://php.net/manual/en/function.call-user-func-array.php

into here?

	$errors = array();
$fields = array(
  'username' => array(
    'validator' => 'validateUsername',
    'message'   => 'Username be between three and fourteen characters'
  ),
  'email' => array(
    'validator' => 'validateEmail',
    'message'   => 'invalid email',
  ),
  'password' => array(
    'validator' => 'validatePassword',
    'message'   => 'invalid password'
  ),
  'repeatPassword' => array(
    'validator' => 'validateRepeatPassword',
    'message'   => 'passwords must match'
  )
);

[NogDog]

(lightbulb goes on)

You may need to take that check out of that DRY loop and do it separately, since it doesn't fit the same pattern as the others of just one parameter.

I suppose you could add some sort of conditional in that loop to call it differently in that case, but then you add complexity, and I'd rather avoid complexity if not needed.

[cluelessPHP]

<?php 

error_reporting(-1); // reports all errors    

ini_set("display_errors", "1"); // shows all errors    

ini_set("log_errors", 1);    

ini_set("error_log", "register.log");  


require "loadclasses.php";

$database = new Database;

$post = filter_input_array(INPUT_POST, FILTER_SANITIZE_STRING);		

$password = $username = $email = $repeatPassword = "";	


$username = $post['username'];
$password = $post['password'];
$repeatPassword = $post['repeatPassword'];
$email = $post['email'];	
$activationCode = substr(str_shuffle("abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ") , 0, 1) . substr(md5(time()) , 1);
$filename = 'tmp_profile.jpg';
$filepath = 'uploads/images/' . 'tmp_profile.jpg';
$filetype = 'jpg';

$errors = array();
$fields = array(
  'username' => array(
    'validator' => 'validateUsername',
    'message'   => 'Username be between three and fourteen characters'
  ),
  'email' => array(
    'validator' => 'validateEmail',
    'message'   => 'invalid email',
  ),
  'password' => array(
    'validator' => 'validatePassword',
    'message'   => 'invalid password'
  )
);

if(!Validation::validateRepeatPassword($password, $repeatPassword)) 
{ 
    $errors[] = ["name" => "repeatPassword", "error" => "Passwords must match"]; 
} 

foreach($_POST as $key => $value) 
	{
  if(isset($fields[$key])) 
	{
			if(!Validation::{$fields[$key]['validator']}($value)) 
			{
				$errors[] = ['name' => $key, 'error' => $fields[$key]['message']];
			}
	}
}
if(empty($errors)) 
	{
		try
		{
			$database->query('INSERT INTO username (username_id, password ,activecode, joinedDate, eMail, active, img_name,img_path, img_type)
			VALUES
			(:username, :password,:activationCode, NOW(), :email, 0, :filename, :filepath, :filetype)');
			$database->bind(':username', $username);
			$database->bind(':email', $email);
			$database->bind(':password', password_hash($post['password'], PASSWORD_DEFAULT));
			$database->bind(':activationCode', $activationCode);
			$database->bind(':filename', $filename);
			$database->bind(':filepath', $filepath);
			$database->bind(':filetype', $filetype);
			$database->execute();
			return true;
		}
		catch(Exception $e)
			{
				$errors[] = ["name" => "username", "error" => "Username or E-mail may already be registered"];
			}

}	  

header('Content-Type: application/json');

if (isset($success))
{
	echo json_encode($success);
}
else
{
	echo json_encode(["errors" => $errors]);
}		

Reasonable? I think I should put

$activationCode = substr(str_shuffle("abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ") , 0, 1) . substr(md5(time()) , 1); 

in the validation class or is it better there?

[Weedpacket]

Getting clever (for some values of "clever"), [font=monospace]validateRepeatPassword[/font] could be written as a higher-order function:

function validateRepeatPassword($password)
{
    return function($repeatPassword)use($password)
    {
        return $repeatPassword == $password;
    };
}

Then...

$fields = array(
  'username' => array(
    'validator' => 'validateUsername',
    'message'   => 'Username be between three and fourteen characters'
  ),
  'email' => array(
    'validator' => 'validateEmail',
    'message'   => 'invalid email',
  ),
  'password' => array(
    'validator' => 'validatePassword',
    'message'   => 'invalid password'
  ),
  'repeatPassword' => array(
    'validator' => validateRepeatPassword($password),
    'message'   => 'passwords must match'
  )
);  

Which, needless to say, requires [font=monospace]$password[/font] to already have the appropriate value when that [font=monospace]$fields[/font] array is created.

An alternative idea is: since there is a Validation class as a general-purpose thing, subclass it for each form/collection-of-things-to-be-validated. That subclass would have an instance variable for each form-field/thing-to-be-validated, to be supplied when it's instantiated. The resulting object, when called on to "validate", can do its stuff by chucking fields at Validation methods and do so in any combination.

[cluelessPHP]

Weedpacket;11060791 wrote:

Getting clever (for some values of "clever"), [font=monospace]validateRepeatPassword[/font] could be written as a higher-order function:

function validateRepeatPassword($password)
{
    return function($repeatPassword)use($password)
    {
        return $repeatPassword == $password;
    };
}

Then...

$fields = array(
  'username' => array(
    'validator' => 'validateUsername',
    'message'   => 'Username be between three and fourteen characters'
  ),
  'email' => array(
    'validator' => 'validateEmail',
    'message'   => 'invalid email',
  ),
  'password' => array(
    'validator' => 'validatePassword',
    'message'   => 'invalid password'
  ),
  'repeatPassword' => array(
    'validator' => validateRepeatPassword($password),
    'message'   => 'passwords must match'
  )
);  

Which, needless to say, requires [font=monospace]$password[/font] to already have the appropriate value when that [font=monospace]$fields[/font] array is created.

An alternative idea is: since there is a Validation class as a general-purpose thing, subclass it for each form/collection-of-things-to-be-validated. That subclass would have an instance variable for each form-field/thing-to-be-validated, to be supplied when it's instantiated. The resulting object, when called on to "validate", can do its stuff by chucking fields at Validation methods and do so in any combination.

You mean something like

Register class extends Validation{
if ($_SERVER['REQUEST_METHOD'] == 'POST')
	{
	$post = filter_input_array(INPUT_POST, FILTER_SANITIZE_STRING);		

$password = $username = $email = $repeatPassword = "";	


$username = $post['username'];
$password = $post['password'];
$repeatPassword = $post['repeatPassword'];
$email = $post['email'];	
$activationCode = substr(str_shuffle("abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ") , 0, 1) . substr(md5(time()) , 1);
$filename = 'tmp_profile.jpg';
$filepath = 'uploads/images/' . 'tmp_profile.jpg';
$filetype = 'jpg';

$errors = array();
$fields = array( 
  'username' => array( 
    'validator' => 'validateUsername', 
    'message'   => 'Username be between three and fourteen characters' 
  ), 
  'email' => array( 
    'validator' => 'validateEmail', 
    'message'   => 'invalid email', 
  ), 
  'password' => array( 
    'validator' => 'validatePassword', 
    'message'   => 'invalid password' 
  ), 
  'repeatPassword' => array( 
    'validator' => validateRepeatPassword($password), 
    'message'   => 'passwords must match' 
  ) 
);  

}

   'validator' => validateRepeatPassword($password), 

I'm just in and sat down to look at this but hm seems to throw an error, about to search see what I find

[Weedpacket]

Well, those (the higher-order function and the subclass) were two separate ideas.

Subclassing Validation might look something like this:

class ValidateRegistration extends Validation
{
	function __construct($post)
	{
		$this->password  = $post['password'] ?? null;
		$this->username  = $post['username'] ?? null;
		//...
		//...
	}

private function validUsernameLength()
{
	return parent::validateUsername($this->username);
}
private function validateRepeatPassword()
{
	return parent::validateRepeatPassword($this->password, $this->repeatPassword);
}
//...


public function validate()
{
	// Changed the name from $fields because there is no longer a 1:1 mapping
	// between form fields and checks to be made on them
	$checks = [
		[[$this, 'validUsernameLength'], 'Username should be between three and fourteen characters'],
		[[$this, 'validRepeatPassword'], 'Passwords must match'],
		//...
		]
	];

	$errors = [];
	//foreach($checks as [$method, $message])  -- Uh, PHP7.1 idiom)
	foreach($checks as $check)
	{
		list($method, $message) = $check;
		if(!$method())
		{
			$errors[] = $message;
		}
	}
	return $errors;
}
}

// To call
$registrationCheck = new ValidateRegistration(filter_input_array(INPUT_POST, FILTER_SANITIZE_STRING));
$errors = $registrationCheck->validate();

I'll be the first to point out there's a lot of coupling (e.g., where the bounds on username length are stored), opportunities for refactoring, and my naming isn't the flashest, but I hope my intent shows through.

[cluelessPHP]

Reading it now, started trying to use now at least.. I had to change one or two things, might have done more harm than good though

<?php  

class Validation 
{
	private 
	$password,
	$repeatPassword,
	$username,
	$email;


private static
$minPassword = 5,
$confirmPassword,
$minUsername = 3,
$maxUsername = 14,
$validEmail,
$alphaNumericID = "/^[A-Za-z0-9]*$/";

public static function validateEmail($email)
{	
	return (filter_var($email, FILTER_VALIDATE_EMAIL) != self::$validEmail);
}

public static function validatePassword($password)
{
	return(strlen($password) >= self::$minPassword);
}

public static function validatecheckbox($agree)
{
	return(!empty($terms) >= self::$checked);
}

public static function validateRepeatPassword($password) 
{ 
		return $repeatPassword === $password; 

}  

public static function validateUsername($username)
{
		return strlen($username) >= self::$minUsername 
		&& (strlen($username)) <= self::$maxUsername 
		&& ($username) !==  self::$alphaNumericID;
}
}

<?php 

error_reporting(-1); // reports all errors   

ini_set("display_errors", "1"); // shows all errors   

ini_set("log_errors", 1);   

ini_set("error_log", "register-errors.log"); 

require ("Validation.class.php");
class ValidateRegistration extends Validation 
{ 
    function __construct($post) 
    { 
        $this->password  = $post['password'] ?? null; 
        $this->repeatPassword  = $post['repeatPassword'] ?? null; 
        $this->email  = $post['email'] ?? null; 
        $this->username  = $post['username'] ?? null; 
    } 

public static function validUsernameLength() 
{ 
    return parent::validateUsername($this->username); 
} 
public static function validEmail() 
{ 
    return parent::validateEmail($this->email); 
} 
public static function validPassword() 
{ 
    return parent::validatePassword($this->password); 
} 
public static function validateRepeatPassword() 
{ 
    return parent::validateRepeatPassword($this->password, $this->repeatPassword); 
} 
//... 


public function validate() 
{ 
    // Changed the name from $fields because there is no longer a 1:1 mapping 
    // between form fields and checks to be made on them 
    $checks = [ 
        [[$this, 'validUsernameLength'], 'Username should be between three and fourteen characters'], 
        [[$this, 'validateRepeatPassword'], 'Passwords must match'],
        [[$this, 'validEmail'], 'You must enter a vailid email'],
        [[$this, 'validPassword'], 'Password must be...']
        //... 

    ]; 

    $errors = []; 
    //foreach($checks as [$method, $message])  -- Uh, PHP7.1 idiom) 
    foreach($checks as $check) 
    { 
        list($method, $message) = $check; 
        if(!$method()) 
        { 
            $errors[] = $message; 
        } 
    } 
    return $errors; 
} 
} 

// To call 
$registrationCheck = new ValidateRegistration(filter_input_array(INPUT_POST, FILTER_SANITIZE_STRING)); 
$errors = $registrationCheck->validate();  

<br />
<b>Warning</b>:  Declaration of ValidateRegistration::validateRepeatPassword() should be compatible with Validation::validateRepeatPassword($password) in <b>E:\EasyPHP-Devserver-16.1\eds-www\gotsocial\classes\ValidationInterface.class.php</b> on line <b>9</b><br />
<br />
<b>Fatal error</b>:  Uncaught Error: Using $this when not in object context in E:\EasyPHP-Devserver-16.1\eds-www\gotsocial\classes\ValidationInterface.class.php:21
Stack trace:
#0 E:\EasyPHP-Devserver-16.1\eds-www\gotsocial\classes\ValidationInterface.class.php(56): ValidateRegistration::validUsernameLength()
#1 E:\EasyPHP-Devserver-16.1\eds-www\gotsocial\classes\ValidationInterface.class.php(67): ValidateRegistration-&gt;validate()
#2 {main}
  thrown in <b>E:\EasyPHP-Devserver-16.1\eds-www\gotsocial\classes\ValidationInterface.class.php</b> on line <b>21</b><br />

[NogDog]

Statically called methods don't have a "$this->", as "$this" means "this object" -- but no object has been instantiated. Therefore you need to use "self::" for any class-level things. At this point, it gets kind of murky for me if you want to in fact instantiate an object so that you can use the __construct() method, or whether it makes sense at this point to reconsider whether it should be accessed statically at all, or other re-factoring choices.

[cluelessPHP]

Ah the reason I changed it to static was

<b>Fatal error</b>:  Cannot make static method Validation::validateRepeatPassword() non static in class ValidateRegistration in <b>E:\EasyPHP-Devserver-16.1\eds-www\gotsocial\classes\ValidationInterface.class.php</b> on line <b>9</b><br />

It "seemed" to fix it but threw out some other errors

[cluelessPHP]

I kind of understand the problem now, I'll resolve for now and fix once I can do fully myself