Skip to main content
added 473 characters in body
Source Link
Your Common Sense
  • 9.1k
  • 1
  • 22
  • 51
  • Connection encoding is better to be set in the DSN, as it's going to be more generic and supported by all drivers.
  • reading the configuration right in the class violates the Single responsibility principle. I would make this class to accept an array of parameters, as to where these parameters are taken from - an .ini, .yml or .env file - is a distinct matter.
  • "Facilitate maintenance" option is somewhat defeated by the hardcoding of the configuration file name. What if you want to change it in the future as well? A typical web application has dozens of configuration options, it's just impractical to put them in different files whose names are hardcoded in different modules. To make a maintainable application it's better to have a distinct module for configuration that will supply different options to all other modules.
  • And all the hassle related to protecting the configuration should be delegated elsewhere. After all, database credentials are not only settings that have to be protected - there are admin email, salt, API keys, etc.
  • protecting an ini file adding .php as one of its extensions is too risky. It would work merely by accident (It would work only under Apache web-server if a certain configuration option is set). Why not to name it straight settings.php and thus make sure it will be always interpreted as PHP as long as it is called through a web-server with PHP support?
  • I would also add a possibility to add/override the PDO options
  • Connection encoding is better to be set in the DSN, as it's going to be more generic and supported by all drivers.
  • reading the configuration right in the class violates the Single responsibility principle. I would make this class to accept an array of parameters, as to where these parameters are taken from - an .ini, .yml or .env file - is a distinct matter.
  • And all the hassle related to protecting the configuration should be delegated elsewhere. After all, database credentials are not only settings that have to be protected - there are admin email, salt, API keys, etc.
  • protecting an ini file adding .php as one of its extensions is too risky. It would work merely by accident (It would work only under Apache web-server if a certain configuration option is set). Why not to name it straight settings.php and thus make sure it will be always interpreted as PHP as long as it is called through a web-server with PHP support?
  • I would also add a possibility to add/override the PDO options
  • Connection encoding is better to be set in the DSN, as it's going to be more generic and supported by all drivers.
  • reading the configuration right in the class violates the Single responsibility principle. I would make this class to accept an array of parameters, as to where these parameters are taken from - an .ini, .yml or .env file - is a distinct matter.
  • "Facilitate maintenance" option is somewhat defeated by the hardcoding of the configuration file name. What if you want to change it in the future as well? A typical web application has dozens of configuration options, it's just impractical to put them in different files whose names are hardcoded in different modules. To make a maintainable application it's better to have a distinct module for configuration that will supply different options to all other modules.
  • And all the hassle related to protecting the configuration should be delegated elsewhere. After all, database credentials are not only settings that have to be protected - there are admin email, salt, API keys, etc.
  • protecting an ini file adding .php as one of its extensions is too risky. It would work merely by accident (It would work only under Apache web-server if a certain configuration option is set). Why not to name it straight settings.php and thus make sure it will be always interpreted as PHP as long as it is called through a web-server with PHP support?
  • I would also add a possibility to add/override the PDO options
added 4 characters in body
Source Link
Your Common Sense
  • 9.1k
  • 1
  • 22
  • 51
  • First of all, it violates the Liskov substitution principle. I am guilty for doing it too, so I cannot blame you too much but if you want your code to follow the best practices, it's better to make your class not extend PDO but either

  • make a PDO instance a property of your class publicly accessible through a property or a method.

  • or - if you want to have a class with PDO's functionality but a different constructor, you have to re-create in your class all the functionality supported by PDO. Although sounds too laborious, it is not that hard as it seems but it pays back in the future.

  • Next, error reporting for the connection is rather inflexible. An exception is a precious thing that can be handled in many different ways, logging included. So I would rather re-throw a new exception, like

      try {
          parent::__construct($dsn, $user, $pass, $options);
      } catch (PDOException $e) {
          throw new \PDOException($e->getMessage(), (int)$e->getCode());
      }
    
  • Connection encoding is better to be set in the DSN, as it's going to be more generic and supported by all drivers.
  • reading the configuration right in the class violates the Single responsibility principle. I would make this class to accept an array of parameters, as to where these parameters are taken from - froman .ini.ini, .yml.yml or .env.env file - is a distinct matter.
  • And all the hassle related to protecting the configuration should be delegated elsewhere. After all, database credentials are not only settings that have to be protected - there are admin email, salt, API keys, etc.
  • protecting an ini file adding .php as one of its extensions is too risky. It would work merely by accident (It would work only under Apache web-server if a certain configuration option is set). Why not to name it straight settings.php and thus make sure it will be always interpreted as PHP as long as it is called through a web-server with PHP support?
  • I would also add a possibility to add/override the PDO options
  • First of all, it violates the Liskov substitution principle. I am guilty for doing it too, so I cannot blame you too much but if you want your code to follow the best practices, it's better to make your class not extend PDO but either

  • make a PDO instance a property of your class publicly accessible through a property or a method.

  • or - if you want to have a class with PDO's functionality but a different constructor, you have re-create in your class all the functionality supported by PDO. Although sounds too laborious, it is not that hard as it seems but it pays back in the future.

  • Next, error reporting for the connection is rather inflexible. An exception is a precious thing that can be handled in many different ways, logging included. So I would rather re-throw a new exception, like

      try {
          parent::__construct($dsn, $user, $pass, $options);
      } catch (PDOException $e) {
          throw new \PDOException($e->getMessage(), (int)$e->getCode());
      }
    
  • Connection encoding is better to be set in the DSN, as it's going to be more generic and supported by all drivers.
  • reading the configuration right in the class violates the Single responsibility principle. I would make this class to accept an array of parameters, as to where these parameters are taken from - from .ini, .yml or .env file - is a distinct matter.
  • And all the hassle related to protecting the configuration should be delegated elsewhere. After all, database credentials are not only settings that have to be protected - there are admin email, salt, API keys, etc.
  • protecting an ini file adding .php as one of its extensions is too risky. It would work merely by accident (It would work only under Apache web-server if a certain configuration option is set). Why not to name it straight settings.php and thus make sure it will be always interpreted as PHP as long as it is called through a web-server with PHP support?
  • I would also add a possibility to add/override the PDO options
  • First of all, it violates the Liskov substitution principle. I am guilty for doing it too, so I cannot blame you too much but if you want your code to follow the best practices, it's better to make your class not extend PDO but either

  • make a PDO instance a property of your class publicly accessible through a property or a method.

  • or - if you want to have a class with PDO's functionality but a different constructor, you have to re-create in your class all the functionality supported by PDO. Although sounds too laborious, it is not that hard as it seems but it pays back in the future.

  • Next, error reporting for the connection is rather inflexible. An exception is a precious thing that can be handled in many different ways, logging included. So I would rather re-throw a new exception, like

      try {
          parent::__construct($dsn, $user, $pass, $options);
      } catch (PDOException $e) {
          throw new \PDOException($e->getMessage(), (int)$e->getCode());
      }
    
  • Connection encoding is better to be set in the DSN, as it's going to be more generic and supported by all drivers.
  • reading the configuration right in the class violates the Single responsibility principle. I would make this class to accept an array of parameters, as to where these parameters are taken from - an .ini, .yml or .env file - is a distinct matter.
  • And all the hassle related to protecting the configuration should be delegated elsewhere. After all, database credentials are not only settings that have to be protected - there are admin email, salt, API keys, etc.
  • protecting an ini file adding .php as one of its extensions is too risky. It would work merely by accident (It would work only under Apache web-server if a certain configuration option is set). Why not to name it straight settings.php and thus make sure it will be always interpreted as PHP as long as it is called through a web-server with PHP support?
  • I would also add a possibility to add/override the PDO options
added 351 characters in body
Source Link
Your Common Sense
  • 9.1k
  • 1
  • 22
  • 51
  • First of all, it violates the Liskov substitution principle. I am guilty for doing it too, so I cannot blame you too much but if you want your code to follow the best practices, it's better to make your class not extend PDO but either 

  • make a PDO instance a property of your class publicly accessible through a property or a method,.

  • or - if you canwant to have a class with PDO's functionality but a different constructor, you have re-create in your class all the functionality supported by PDO. Although sounds too laborious, it is not that hard as it seems but it pays back in the future.

  • Next, error reporting for the connection is rather inflexible. An exception is a precious thing that can be handled in many different ways, logging included. So I would rather re-throw a new exception, like

      try {
          parent::__construct($dsn, $user, $pass, $options);
      } catch (PDOException $e) {
          throw new \PDOException($e->getMessage(), (int)$e->getCode());
      }
    

so it won't expose the connection credentials in the stack trace but still it can be caught elsewhere or simply logged if a corresponding PHP configuration directive says so.

  • Connection encoding is better to be set in the DSN, as it's going to be more generic and supported by all drivers.
  • reading the configuration right in the class violates the Single responsibility principle. I would make this class to accept an array of parameters, as to where these parameters are taken from - from .ini, .yml or .env file - is a distinct matter.
  • And all the hassle related to protecting the configuration should be delegated elsewhere. After all, database credentials are not only settings that have to be protected - there are admin email, salt, API keys, etc.
  • protecting an ini file adding .php as one of its extensions is too risky. itIt would work merely by accident (It would work only under Apache web-server if a certain configuration option is set). Why not to name it straight settings.php and thus make sure it will be always interpreted as PHP as long as it is called through a web-server with PHP support.?
  • I would also add a possibility to add/override the PDO options
  • First of all, it violates the Liskov substitution principle. I am guilty for doing it too, so I cannot blame you too much but if you want your code to follow the best practices, it's better to make your class not extend PDO but either make a PDO instance a property of your class publicly accessible through a property or a method, or you can re-create in your class all the functionality supported by PDO.

  • Next, error reporting for the connection is rather inflexible. An exception is a precious thing that can be handled in many different ways, logging included. So I would rather re-throw a new exception, like

      try {
          parent::__construct($dsn, $user, $pass, $options);
      } catch (PDOException $e) {
          throw new \PDOException($e->getMessage(), (int)$e->getCode());
      }
    

so it can be caught elsewhere or simply logged if a corresponding PHP configuration directive says so.

  • Connection encoding is better to be set in the DSN, as it's going to be more generic and supported by all drivers.
  • reading the configuration right in the class violates the Single responsibility principle. I would make this class to accept an array of parameters, as to where these parameters are taken from - from .ini, .yml or .env file - is a distinct matter.
  • And all the hassle related to protecting the configuration should be delegated elsewhere. After all, database credentials are not only settings that have to be protected - there are admin email, salt, API keys, etc.
  • protecting an ini file adding .php as one of its extensions is too risky. it would work merely by accident. Why not to name it straight settings.php and thus make sure it will be always interpreted as PHP as long as it is called through a web-server with PHP support.
  • I would also add a possibility to add/override the PDO options
  • First of all, it violates the Liskov substitution principle. I am guilty for doing it too, so I cannot blame you too much but if you want your code to follow the best practices, it's better to make your class not extend PDO but either 

  • make a PDO instance a property of your class publicly accessible through a property or a method.

  • or - if you want to have a class with PDO's functionality but a different constructor, you have re-create in your class all the functionality supported by PDO. Although sounds too laborious, it is not that hard as it seems but it pays back in the future.

  • Next, error reporting for the connection is rather inflexible. An exception is a precious thing that can be handled in many different ways, logging included. So I would rather re-throw a new exception, like

      try {
          parent::__construct($dsn, $user, $pass, $options);
      } catch (PDOException $e) {
          throw new \PDOException($e->getMessage(), (int)$e->getCode());
      }
    

so it won't expose the connection credentials in the stack trace but still it can be caught elsewhere or simply logged if a corresponding PHP configuration directive says so.

  • Connection encoding is better to be set in the DSN, as it's going to be more generic and supported by all drivers.
  • reading the configuration right in the class violates the Single responsibility principle. I would make this class to accept an array of parameters, as to where these parameters are taken from - from .ini, .yml or .env file - is a distinct matter.
  • And all the hassle related to protecting the configuration should be delegated elsewhere. After all, database credentials are not only settings that have to be protected - there are admin email, salt, API keys, etc.
  • protecting an ini file adding .php as one of its extensions is too risky. It would work merely by accident (It would work only under Apache web-server if a certain configuration option is set). Why not to name it straight settings.php and thus make sure it will be always interpreted as PHP as long as it is called through a web-server with PHP support?
  • I would also add a possibility to add/override the PDO options
deleted 4 characters in body
Source Link
Your Common Sense
  • 9.1k
  • 1
  • 22
  • 51
Loading
deleted 74 characters in body
Source Link
Your Common Sense
  • 9.1k
  • 1
  • 22
  • 51
Loading
Source Link
Your Common Sense
  • 9.1k
  • 1
  • 22
  • 51
Loading