The
mb_strlen($configFileNameWithPath) > 0checks don't need the> 0part at the end.mb-strlenmb_strlen()will always return an integer-type value, so you only need to check if it is truthy. As I think about it, I don't see the value inmb_strlen()-- it will be potentially more accurate in non-zero cases, but you only intend to differentiate between zero and non-zero. For this reason, maybe just usestrlen().IMPORTANT: A constructor returns nothing. You should not be returning
true. Please read Constructor returning value?.$envVarNamesand$settingConfigurationNameare related arrays, but you have not declared them as such. I recommend that you logically construct a lookup array (as a class property so that only processing code is in the method) which is associative. This will set you up to employ aforeach()loop instead of counting the length of an array on every iteration.private $envConfigNames = [ 'EHJWT_JWT_SECRET' => 'jwtSecret', 'EHJWT_DSN' => 'dsn', 'EHJWT_DB_USER' => 'dbUser', 'EHJWT_DB_PASS' => 'dbPassword', ]; private function setConfigurationsFromEnvVars(): void { foreach ($this->$envConfigNames as $envName => $configName) { $value = getenv($envName); if (strlen($value)) { $this->configurations[$configName] = $value; } }Use this lookup array again in
setConfigurationsFromConfigFile()andsetConfigurationsFromArguments().Instead of
gettype($configFileSettings) !== 'array', it is more concise to use!is_array($configFileSettings).I, generally, do not endorse the use of "variable variables", but I don't find your usage to be offensive or pulling your code into a disheveled state. The syntax can be as simple as `$value = $$settingName;
$value = $$settingName;You never need to check if a variable/element
isset()beforeunset()ing it. Remove the condition and justunset()it.I do not endorse the unconditional return of
truein methods. It is not informative, it is just useless noise. If there is no chance of returningfalse, just don't return anything (:void).I prefer to avoid single-use variables (unless there is a clear and valuable benefit in the declarative variable name).
createToken()seems like the values should be directly fed into theimplode()call.I recommend declaring the return types on all methods (except the constructor). This will help with debugging and enforce strict/predictable types in your application.
public function loadToken(string $tokenString): boolConsistently use ALL-CAPS when writing sql keywords -- this will improve the readability of your code. (e.g. convert
wheretoWHERE)Even if variables fed to your sql are coming from a safe place, I recommend consistently using prepared statements with bound parameters. When you have a whole application written this way, you will be less likely to have other devs on your team "sneak in" unsafe values when they copy-pasta from your un-parameterized query.
I do not like that you are creating a new pdo connection for every query. You should modify
makeRevocationTableDatabaseConnection()so that if there is already an established connection, then it is re-used.Return the boolean evaluation for more concise code.
private function verifyThreeMembers(array $array): bool { return count($array) === 3; }For methods that return a boolean value, I often start the method name with
isorhas(or a similar/suitable verb) so that all devs that read the code will infer from its name that it returns a boolean. When sorting my methods in my IDE (phpstorm), this serves to group the boolean-returning methods together.With pdo, I prefer to pass the values directly into the
execute()function to avoid thebindParam()calls.Use
fetchAll()when returning an entire unaltered result set instead of iteratively fetching the result set.return $stmt->fetchAll(PDO::FETCH_ASSOC);
The
mb_strlen($configFileNameWithPath) > 0checks don't need the> 0part at the end.mb-strlen()will always return an integer-type value, so you only need to check if it is truthy. As I think about it, I don't see the value inmb_strlen()-- it will be potentially more accurate in non-zero cases, but you only intend to differentiate between zero and non-zero. For this reason, maybe just usestrlen().IMPORTANT: A constructor returns nothing. You should not be returning
true. Please read Constructor returning value?.$envVarNamesand$settingConfigurationNameare related arrays, but you have not declared them as such. I recommend that you logically construct a lookup array (as a class property so that only processing code is in the method) which is associative. This will set you up to employ aforeach()loop instead of counting the length of an array on every iteration.private $envConfigNames = [ 'EHJWT_JWT_SECRET' => 'jwtSecret', 'EHJWT_DSN' => 'dsn', 'EHJWT_DB_USER' => 'dbUser', 'EHJWT_DB_PASS' => 'dbPassword', ]; private function setConfigurationsFromEnvVars(): void { foreach ($this->$envConfigNames as $envName => $configName) { $value = getenv($envName); if (strlen($value)) { $this->configurations[$configName] = $value; } }Use this lookup array again in
setConfigurationsFromConfigFile()andsetConfigurationsFromArguments().Instead of
gettype($configFileSettings) !== 'array', it is more concise to use!is_array($configFileSettings).I, generally, do not endorse the use of "variable variables", but I don't find your usage to be offensive or pulling your code into a disheveled state. The syntax can be as simple as `$value = $$settingName;
You never need to check if a variable/element
isset()beforeunset()ing it. Remove the condition and justunset()it.I do not endorse the unconditional return of
truein methods. It is not informative, it is just useless noise. If there is no chance of returningfalse, just don't return anything (:void).I prefer to avoid single-use variables (unless there is a clear and valuable benefit in the declarative variable name).
createToken()seems like the values should be directly fed into theimplode()call.I recommend declaring the return types on all methods (except the constructor). This will with debugging and enforce strict/predictable types in your application.
public function loadToken(string $tokenString): boolConsistently use ALL-CAPS when writing sql keywords -- this will improve the readability of your code. (e.g. convert
wheretoWHERE)Even if variables fed to your sql are coming from a safe place, I recommend consistently using prepared statements with bound parameters. When you have a whole application written this way, you will be less likely to have other devs on your team "sneak in" unsafe values when they copy-pasta from your un-parameterized query.
I do not like that you are creating a new pdo connection for every query. You should modify
makeRevocationTableDatabaseConnection()so that if there is already an established connection, then it is re-used.Return the boolean evaluation for more concise code.
private function verifyThreeMembers(array $array): bool { return count($array) === 3; }For methods that return a boolean value, I often start the method name with
isorhas(or a similar/suitable verb) so that all devs that read the code will infer from its name that it returns a boolean. When sorting my methods in my IDE (phpstorm), this serves to group the boolean-returning methods together.With pdo, I prefer to pass the values directly into the
execute()function to avoid thebindParam()calls.Use
fetchAll()when returning an entire unaltered result set instead of iteratively fetching the result set.return $stmt->fetchAll(PDO::FETCH_ASSOC);
The
mb_strlen($configFileNameWithPath) > 0checks don't need the> 0part at the end.mb_strlen()will always return an integer-type value, so you only need to check if it is truthy. As I think about it, I don't see the value inmb_strlen()-- it will be potentially more accurate in non-zero cases, but you only intend to differentiate between zero and non-zero. For this reason, maybe just usestrlen().IMPORTANT: A constructor returns nothing. You should not be returning
true. Please read Constructor returning value?.$envVarNamesand$settingConfigurationNameare related arrays, but you have not declared them as such. I recommend that you logically construct a lookup array (as a class property so that only processing code is in the method) which is associative. This will set you up to employ aforeach()loop instead of counting the length of an array on every iteration.private $envConfigNames = [ 'EHJWT_JWT_SECRET' => 'jwtSecret', 'EHJWT_DSN' => 'dsn', 'EHJWT_DB_USER' => 'dbUser', 'EHJWT_DB_PASS' => 'dbPassword', ]; private function setConfigurationsFromEnvVars(): void { foreach ($this->$envConfigNames as $envName => $configName) { $value = getenv($envName); if (strlen($value)) { $this->configurations[$configName] = $value; } }Use this lookup array again in
setConfigurationsFromConfigFile()andsetConfigurationsFromArguments().Instead of
gettype($configFileSettings) !== 'array', it is more concise to use!is_array($configFileSettings).I, generally, do not endorse the use of "variable variables", but I don't find your usage to be offensive or pulling your code into a disheveled state. The syntax can be as simple as
$value = $$settingName;You never need to check if a variable/element
isset()beforeunset()ing it. Remove the condition and justunset()it.I do not endorse the unconditional return of
truein methods. It is not informative, it is just useless noise. If there is no chance of returningfalse, just don't return anything (:void).I prefer to avoid single-use variables (unless there is a clear and valuable benefit in the declarative variable name).
createToken()seems like the values should be directly fed into theimplode()call.I recommend declaring the return types on all methods (except the constructor). This will help with debugging and enforce strict/predictable types in your application.
public function loadToken(string $tokenString): boolConsistently use ALL-CAPS when writing sql keywords -- this will improve the readability of your code. (e.g. convert
wheretoWHERE)Even if variables fed to your sql are coming from a safe place, I recommend consistently using prepared statements with bound parameters. When you have a whole application written this way, you will be less likely to have other devs on your team "sneak in" unsafe values when they copy-pasta from your un-parameterized query.
I do not like that you are creating a new pdo connection for every query. You should modify
makeRevocationTableDatabaseConnection()so that if there is already an established connection, then it is re-used.Return the boolean evaluation for more concise code.
private function verifyThreeMembers(array $array): bool { return count($array) === 3; }For methods that return a boolean value, I often start the method name with
isorhas(or a similar/suitable verb) so that all devs that read the code will infer from its name that it returns a boolean. When sorting my methods in my IDE (phpstorm), this serves to group the boolean-returning methods together.With pdo, I prefer to pass the values directly into the
execute()function to avoid thebindParam()calls.Use
fetchAll()when returning an entire unaltered result set instead of iteratively fetching the result set.return $stmt->fetchAll(PDO::FETCH_ASSOC);
The
mb_strlen($configFileNameWithPath) > 0checks don't need the> 0part at the end.mb-strlen()will always return an integer-type value, so you only need to check if it is truthy. As I think about it, I don't see the value inmb_strlen()-- it will be potentially more accurate in non-zero cases, but you only intend to differentiate between zero and non-zero. For this reason, maybe just usestrlen().IMPORTANT: A constructor returns nothing. You should not be returning
true. Please read Constructor returning value?.$envVarNamesand$settingConfigurationNameare related arrays, but you have not declared them as such. I recommend that you logically construct a lookup array (as a class property so that only processing code is in the method) which is associative. This will set you up to employ aforeach()loop instead of counting the length of an array on every iteration.private $envConfigNames = [ 'EHJWT_JWT_SECRET' => 'jwtSecret', 'EHJWT_DSN' => 'dsn', 'EHJWT_DB_USER' => 'dbUser', 'EHJWT_DB_PASS' => 'dbPassword', ]; private function setConfigurationsFromEnvVars(): void { foreach ($this->$envConfigNames as $envName => $configName) { $value = getenv($envName); if (strlen($value)) { $this->configurations[$configName] = $value; } }Use this lookup array again in
setConfigurationsFromConfigFile()andsetConfigurationsFromArguments().Instead of
gettype($configFileSettings) !== 'array', it is more concise to use!is_array($configFileSettings).I, generally, do not endorse the use of "variable variables", but I don't find your usage to be offensive or pulling your code into a disheveled state. The syntax can be as simple as `$value = $$settingName;
You never need to check if a variable/element
isset()beforeunset()ing it. Remove the condition and justunset()it.I do not endorse the unconditional return of
truein methods. It is not informative, it is just useless noise. If there is no chance of returningfalse, just don't return anything (:void).I prefer to avoid single-use variables (unless there is a clear and valuable benefit in the declarative variable name).
createToken()seems like the values should be directly fed into theimplode()call.I recommend declaring the return types on all methods (except the constructor). This will with debugging and enforce strict/predictable types in your application.
public function loadToken(string $tokenString): boolConsistently use ALL-CAPS when writing sql keywords -- this will improve the readability of your code. (e.g. convert
wheretoWHERE)Even if variables fed to your sql are coming from a safe place, I recommend consistently using prepared statements with bound parameters. When you have a whole application written this way, you will be less likely to have other devs on your team "sneak in" unsafe values when they copy-pasta from your un-parameterized query.
I do not like that you are creating a new pdo connection for every query. You should modify
makeRevocationTableDatabaseConnection()so that if there is already an established connection, then it is re-used.Return the boolean evaluation for more concise code.
private function verifyThreeMembers(array $array): bool { return count($array) === 3; }For methods that return a boolean value, I often start the method name with
isorhas(or a similar/suitable verb) so that all devs that read the code will infer from its name that it returns a boolean. When sorting my methods in my IDE (phpstorm), this serves to group the boolean-returning methods together.With pdo, I prefer to pass the values directly into the
execute()function to avoid thebindParam()calls.Use
fetchAll()when returning an entire unaltered result set instead of iteratively fetching the result set.return $stmt->fetchAll(PDO::FETCH_ASSOC);