[Cache] Fix storing binary keys when using pgsql #49848
Conversation
| Q | A |
|---|---|
| Branch? | 5.4 |
| Bug fix? | yes |
| New feature? | no |
| Deprecations? | no |
| Tickets | Fix #49748 |
| License | MIT |
| Doc PR | - |
|
Thank you @nicolas-grekas. |
…nt operator (weaverryan) This PR was merged into the 5.4 branch. Discussion ---------- [Cache] Fix php7 compat on 5.4: null coalescing assignment operator | Q | A | ------------- | --- | Branch? | 5.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | None | License | MIT | Doc PR | Not needed It looks like #49848 accidentally introduced a syntax not compat with php 7.2/7.3. So, an annoying little PR to fix that. Commits ------- 7e5fe59 [Cache] Removing null coalescing assignment operator on 5.4
| */ | ||
| protected function getId($key) | ||
| { | ||
| if ('pgsql' !== $this->driver ?? ($this->getConnection() ? $this->driver : null)) { |
There was a problem hiding this comment.
This is one of the most difficult to read if statements I have come across in recent memory. Perhaps something like this would help ... ?
// Ensure that driver has been declared.
$this->getConnection();
if ('pgsql' !== $this->driver) {Or maybe adding a getDriver() helper ... ?
/**
* @return string|null
*/
private function getDriver()
{
$this->getConnection(); // also defines driver
return $this->driver;
}There was a problem hiding this comment.
this code path is on the hot path, it's written in a way that skips calling the method unless it's really needed
There was a problem hiding this comment.
Okay, then how about:
/**
* @return string
*/
private function getDriver()
{
if (null === $this->driver) {
$this->getConnection(); // defines driver
}
return $this->driver;
}There was a problem hiding this comment.
that's still add a method call on the hot path while we can avoid it
There was a problem hiding this comment.
Is the micro-micro-micro optimization worth the cost of cognitive load? I guess that's for Symfony to decide...
There was a problem hiding this comment.
All µ-optims add up when they're on the hot path so 🤷
There was a problem hiding this comment.
Sure, I understand that, but this is in a database backed cache adapter. Variance in latency/IO is going to have far more impact than one method call ever will. 🤷🏼♂️