Glpi key use exceptions not errors #21623
Conversation
| $this->ensureKeyIsValid(); | ||
| return file_get_contents($this->keyfile); |
There was a problem hiding this comment.
This makes it read the key file twice every time it needs to perform an encrypt/decrypt. Should be avoided.
There was a problem hiding this comment.
That's the downside of having 2 methods. The footprint of reading this local file et probably close to null. So I choose this approach (2 methods vs 1), it looks really better than having a single method that does both (checking key & show the message) and violate SRP principle.
Maybe we can have a local variable to keep the errors and avoid 2 calls to filegetcontents.
There was a problem hiding this comment.
I think you are taking textbook concepts a bit too far here. The single purpose of the get method is to get the key. A part of that happens to be ensuring the key is valid, and that validation is only done here.
Indeed though, it probably doesn't make that much difference in this specific case.
There was a problem hiding this comment.
I think you are taking textbook concepts a bit too far here. The single purpose of the get method is to get the key. A part of that happens to be ensuring the key is valid, and that validation is only done here.
Indeed though, it probably doesn't make that much difference in this specific case.
(Looks like my answer was lost)
That's not too far, that's the basis. Experience learnt me that's a very important point. Writing, reading, testing, maintaining, everything is easier respecting separation of concerns.
Only very strong reasons can justify not to respect it. I don't see any reason to do it here.
And we can use ensure method elsewhere in the future.
| $glpiNetwork = new self(); | ||
| $glpiNetwork->showForConfig(); |
There was a problem hiding this comment.
| $glpiNetwork = new self(); | |
| $glpiNetwork->showForConfig(); | |
| self::showForConfig(); |
There was a problem hiding this comment.
I intented to have the less possible changes, but I can change it if it hurt your eyes 😆
There was a problem hiding this comment.
It's only just because it is a static method and should be called statically.
| // Make sure the tester plugin is never deactived by a test as it would | ||
| // impact others tests that depend on it. | ||
| $this->assertTrue(Plugin::isPluginActive('tester')); | ||
| // $this->assertTrue(Plugin::isPluginActive('tester')); |
There was a problem hiding this comment.
| // $this->assertTrue(Plugin::isPluginActive('tester')); | |
| $this->assertTrue(Plugin::isPluginActive('tester')); |
There was a problem hiding this comment.
Oops, should not be committed. I'll fix it
| // Make sure the tester plugin is never deactived by a test as it would | ||
| // impact others tests that depend on it. | ||
| $this->assertTrue(Plugin::isPluginActive('tester')); | ||
| // $this->assertTrue(Plugin::isPluginActive('tester')); |
There was a problem hiding this comment.
| // $this->assertTrue(Plugin::isPluginActive('tester')); | |
| $this->assertTrue(Plugin::isPluginActive('tester')); |
There was a problem hiding this comment.
Do not spent too much time reviewing, it's not ready yet.
2 participants
wip
Checklist before requesting a review
Please delete options that are not relevant.
Description