Code Review Asked by Vladimirov on December 14, 2020
I’m a beginner and I wonder if Is what I have created a good practice as a structure and way of working for input data processing?
Is the way I wrote the index.php file a correct and secure way of manipulation?
The majority of my programming experience (and I do mainly do it for fun) comes from following tutorials on google, so I apologize in advance if this seems an exceptionally daft question – but I do want to start improving my code.
Whenever I have needed to make a multidimensional array, my naming has always placed the counter in the first element.
<?php
$file = dirname(__FILE__) . '/Validator.php';
if (file_exists($file)) {
require $file;
}
function get_intersect(...$arrays){
$instersect_arrays = array();
foreach($arrays as $array){
if(!empty($array)){
array_push($instersect_arrays,$array);
}
}
return call_user_func_array('array_intersect', $instersect_arrays);
}
function in_array_r($needle, $haystack, $strict = false) {
foreach ($haystack as $item) {
if (($strict ? $item === $needle : $item == $needle) || (is_array($item) && in_array_r($needle, $item, $strict))) {
return true;
}
}
return false;
}
$langs_all = [
'bg' => [ 'id' => '1', 'code' => 'bg', ],
'us' => [ 'id' => '2', 'code' => 'us', ],
'gr' => [ 'id' => '3', 'code' => 'gr', ],
];
$langs_enabled = [
'bg' => [ 'id' => '1', 'code' => 'bg', ],
'us' => [ 'id' => '2', 'code' => 'us', ],
];
$langs_default = [
'bg' => [ 'id' => '1', 'code' => 'bg', ],
];
$langs_real = array();
$langs_real = get_intersect($langs_enabled, $langs_all);
$stop = [];
if(isset($_POST['submit'])) {
$array = filter_input_array(INPUT_POST, FILTER_SANITIZE_STRING, true);
$return = array();
foreach ( array_keys($array) as $fieldKey ) {
foreach ( $array[$fieldKey] as $lang_code => $value ) {
if( in_array_r( $lang_code, $langs_real) ) {
$return[$fieldKey][$lang_code] = $value;
}
}
}
foreach ( $langs_default as $lang_code => $value ) {
// Set data and validation rules
$rules = array(
'title['.$lang_code.']' => 'required:Title|min:8|max:200',
'description['.$lang_code.']' => 'norequired:Description|min:20'
);
$data = array(
'title['.$lang_code.']' => $return['title'][$lang_code],
'description['.$lang_code.']' => $return['description'][$lang_code],
);
}
// Run validation
$validator = new Validator();
if ($validator->validate($data, $rules) == true) {
// Validation passed. Set user values.
echo "<h1>SUCCESS</h1>";
print_r ( $return );
} else {
// Validation failed. Dump validation errors.
var_dump($validator->getErrors());
}
}
?>
<form action="<?php echo htmlspecialchars($_SERVER["PHP_SELF"]); ?>" method="POST">
<label for="title[bg]">title bg</label> <input type="text" name="title[bg]" id="title[bg]" /><br />
<label for="title[us]">title us</label> <input type="text" name="title[us]" id="title[us]" /><br />
<label for="title[gr]">title gr</label> <input type="text" name="title[gr]" id="title[gr]" /><br /><br />
<label for="description[bg]">description bg</label> <input type="text" name="description[bg]" id="description[bg]" /><br />
<label for="description[us]">description us</label> <input type="text" name="description[us]" id="description[us]" /><br />
<label for="description[gr]">description gr</label> <input type="text" name="description[gr]" id="description[gr]" /><br />
<input type="submit" name="submit" />
</form>
Validator.php
<?php
class Validator
{
/**
* Validation errors
* @var array
*/
private $errors = array();
private $Msg = array();
/**
* Validate data against a set of rules and set errors in the $this->errors
* array
* @param array $data
* @param array $rules
* @return boolean
*/
public function validate (Array $data, Array $rules)
{
$valid = true;
foreach ($rules as $item => $ruleset) {
// required|email|min:8|msg:title
$ruleset = explode('|', $ruleset);
foreach ($ruleset as $rule) {
$pos = strpos($rule, ':');
if ($pos !== false) {
$parameter = substr($rule, $pos + 1);
$rule = substr($rule, 0, $pos);
}
else {
$parameter = '';
}
// validateEmail($item, $value, $param)
$methodName = 'validate' . ucfirst($rule);
$value = isset($data[$item]) ? $data[$item] : NULL;
if (method_exists($this, $methodName)) {
$this->$methodName($item, $value, $parameter) OR $valid = false;
}
}
}
return $valid;
}
/**
* Get validation errors
* @return array:
*/
public function getErrors ()
{
return $this->errors;
}
/**
* Validate the $value of $item to see if it is present and not empty
* @param string $item
* @param string $value
* @param string $parameter
* @return boolean
*/
private function validateRequired ($item, $value, $parameter)
{
if ($value === '' || $value === NULL || $parameter == false) {
$this->Msg[$item] = $parameter;
$Msg = isset($parameter) ? $parameter : $item;
$this->errors[$item][] = 'The '.$Msg.' field is required';
return false;
}
return true;
}
/**
* Validate the $value of $item to see if it is present and not empty
* @param string $item
* @param string $value
* @param string $parameter
* @return boolean
*/
private function validateNorequired ($item, $value, $parameter)
{
$this->Msg[$item] = $parameter;
return true;
}
/**
* Validate the $value of $item to see if it is a valid email address
* @param string $item
* @param string $value
* @param string $parameter
* @return boolean
*/
private function validateEmail ($item, $value, $parameter)
{
if (! filter_var($value, FILTER_VALIDATE_EMAIL)) {
$Msg[$item] = isset($this->Msg[$item]) ? $this->Msg[$item] : $item;
$this->errors[$item][] = 'The ' . $Msg[$item] . ' field should be a valid email addres';
return false;
}
return true;
}
/**
* Validate the $value of $item to see if it is fo at least $param
* characters long
* @param string $item
* @param string $value
* @param string $parameter
* @return boolean
*/
private function validateMin ($item, $value, $parameter)
{
if (strlen($value) >= $parameter == false) {
$Msg[$item] = isset($this->Msg[$item]) ? $this->Msg[$item] : $item;
$this->errors[$item][] = 'The ' . $Msg[$item] . ' field should have a minimum length of ' . $parameter;
return false;
}
return true;
}
/**
* Validate the $value of $item to see if it is fo at least $param
* characters long
* @param string $item
* @param string $value
* @param string $parameter
* @return boolean
*/
private function validateMax ($item, $value, $parameter)
{
if (strlen($value) < $parameter == false) {
$Msg[$item] = isset($this->Msg[$item]) ? $this->Msg[$item] : $item;
$this->errors[$item][] = 'The ' . $Msg[$item] . ' field should have a maximum length of ' . $parameter;
return false;
}
return true;
}
}
```
The thing that strikes me first is that there are 3 declared lookup arrays with redundant data in them. I see that the script needs to combine and filter these arrays. I would begin the refactor by eliminating redundant data -- declare a single lookup and add any additional metadata within the subarrays.
Until your app has code values that differ from the keys in $lang, remove this unnecessary/redundant column.
Declare the key of the default item for instant referencing/maintenance.
$defaultLang = 'bg';
$langs = [
'bg' => ['id' => 1, 'active' => true],
'us' => ['id' => 2, 'active' => true],
'gr' => ['id' => 3, 'active' => false],
];
This way you have a time complexity of O(1) to find the default entry. You probably free yourself from needing those helper functions too.
Even your html form markup generation can be DRYed out by leveraging the single lookup array.
The explicit truthy check of == true can be omitted. By removing those 8 characters the exact same condition is evaluated for an identical result.
Inside of the inner loop of the validate method, I recommend unconditionally exploding on the optional colon. An early return is appropriate.
public function validate (array $data, array $rules): bool
{
foreach ($rules as $item => $ruleset) {
// required|email|min:8|msg:title
$ruleset = explode('|', $ruleset);
foreach ($ruleset as $rule) {
[$rule, $param] = array_pad(explode(':', $rule, 2), 2, '');
$methodName = 'validate' . ucfirst($rule);
if (!method_exists($this, $methodName)
|| !$this->$methodName($item, $data[$item] ?? null, $parameter)
) {
return false;
}
}
}
return true;
}
...you might prefer to throw an exception if a dynamically called method does not exist.
Some other DRYing techniques can be realized in validateRequired(). I always try to eliminate single-use variables.
private function validateRequired ($item, $value, $parameter)
{
if ((string)$value === '' || !$parameter) {
$this->Msg[$item] = $parameter;
$this->errors[$item][] = 'The ' . ($parameter ?? $item) . ' field is required';
return false;
}
return true;
}
In fact, all isset() calls that exist in your posted code can be swapped out for null coalescing operators.
Answered by mickmackusa on December 14, 2020
Get help from others!
Recent Answers
Recent Questions
© 2024 TransWikia.com. All rights reserved. Sites we Love: PCI Database, UKBizDB, Menu Kuliner, Sharing RPP