Skip to main content
added 206 characters in body
Source Link
tim
  • 25.3k
  • 3
  • 31
  • 76

Security

SQL Injection

You are (very likely) open to SQL injection, as you put user input directly into SQL queries. You need to use prepared statements instead. This goes for all statements (select, insert, ...).

Hashing

You also have to use a proper password hashing function. md5 hasn't been acceptable for at least 15 years. Use password_hash instead.

Misc

Structure

  • your interfaces are unneeded, as they are unlikely to be used in different situations. Your IUserInfoRepository may work as a generic IRepository though, as likely other objects need to be selected, inserted, etc. Your IUserInfo interface could partly work as an IBaseObject interface which contains getId and possibly getCreatedAtand getUpdatedAt (if these are values that you store for all objects).
  • Your getById function returns an array, while your getByEmail function returns a User. This should be handled the same way.
  • You should pass on the specific required fields to insert, not just some input array. Arrays are bad for usability, as a user of your class would need to read the documentation, or in your case - as you don't have any - actually look at the code.

Other

  • add PHPDoc style comments to your functions to explain what the arguments need to be, what the return values are, etc.
  • I would throw an exception if an email doesn't exist instead of returning null, to avoid excessive null checks.
  • always use curly brackets
  • you don't need one-time variables such as $email = $input['email'];.
  • upper-case your SQL keywords to increase readability
  • be less generic with your variable names. obj could be userData, query could be userSelectQuery, etc.

Security

SQL Injection

You are (very likely) open to SQL injection, as you put user input directly into SQL queries. You need to use prepared statements instead. This goes for all statements (select, insert, ...).

Hashing

You also have to use a proper password hashing function. md5 hasn't been acceptable for at least 15 years. Use password_hash instead.

Misc

Structure

  • your interfaces are unneeded, as they are unlikely to be used in different situations. Your IUserInfoRepository may work as a generic IRepository as likely other objects need to be selected, inserted, etc.
  • Your getById function returns an array, while your getByEmail function returns a User. This should be handled the same way.
  • You should pass on the specific required fields to insert, not just some input array. Arrays are bad for usability, as a user of your class would need to read the documentation, or in your case - as you don't have any - actually look at the code.

Other

  • add PHPDoc style comments to your functions to explain what the arguments need to be, what the return values are, etc.
  • I would throw an exception if an email doesn't exist instead of returning null, to avoid excessive null checks.
  • always use curly brackets
  • you don't need one-time variables such as $email = $input['email'];.
  • upper-case your SQL keywords to increase readability
  • be less generic with your variable names. obj could be userData, query could be userSelectQuery, etc.

Security

SQL Injection

You are (very likely) open to SQL injection, as you put user input directly into SQL queries. You need to use prepared statements instead. This goes for all statements (select, insert, ...).

Hashing

You also have to use a proper password hashing function. md5 hasn't been acceptable for at least 15 years. Use password_hash instead.

Misc

Structure

  • your interfaces are unneeded, as they are unlikely to be used in different situations. Your IUserInfoRepository may work as a generic IRepository though, as likely other objects need to be selected, inserted, etc. Your IUserInfo interface could partly work as an IBaseObject interface which contains getId and possibly getCreatedAtand getUpdatedAt (if these are values that you store for all objects).
  • Your getById function returns an array, while your getByEmail function returns a User. This should be handled the same way.
  • You should pass on the specific required fields to insert, not just some input array. Arrays are bad for usability, as a user of your class would need to read the documentation, or in your case - as you don't have any - actually look at the code.

Other

  • add PHPDoc style comments to your functions to explain what the arguments need to be, what the return values are, etc.
  • I would throw an exception if an email doesn't exist instead of returning null, to avoid excessive null checks.
  • always use curly brackets
  • you don't need one-time variables such as $email = $input['email'];.
  • upper-case your SQL keywords to increase readability
  • be less generic with your variable names. obj could be userData, query could be userSelectQuery, etc.
Source Link
tim
  • 25.3k
  • 3
  • 31
  • 76

Security

SQL Injection

You are (very likely) open to SQL injection, as you put user input directly into SQL queries. You need to use prepared statements instead. This goes for all statements (select, insert, ...).

Hashing

You also have to use a proper password hashing function. md5 hasn't been acceptable for at least 15 years. Use password_hash instead.

Misc

Structure

  • your interfaces are unneeded, as they are unlikely to be used in different situations. Your IUserInfoRepository may work as a generic IRepository as likely other objects need to be selected, inserted, etc.
  • Your getById function returns an array, while your getByEmail function returns a User. This should be handled the same way.
  • You should pass on the specific required fields to insert, not just some input array. Arrays are bad for usability, as a user of your class would need to read the documentation, or in your case - as you don't have any - actually look at the code.

Other

  • add PHPDoc style comments to your functions to explain what the arguments need to be, what the return values are, etc.
  • I would throw an exception if an email doesn't exist instead of returning null, to avoid excessive null checks.
  • always use curly brackets
  • you don't need one-time variables such as $email = $input['email'];.
  • upper-case your SQL keywords to increase readability
  • be less generic with your variable names. obj could be userData, query could be userSelectQuery, etc.