Replace direct access to sites with getSites #305
Conversation
|
Looking good! As far as I'm concerned, this can be merged! |
| @@ -119,18 +119,19 @@ public function readTarget($target) | |||
| $environment = $exploded[1]; | |||
| } | |||
|
|
|||
| if (!array_key_exists($site, $this->sites)) { | |||
| $sites = $this->getSites(); | |||
There was a problem hiding this comment.
@LOBsTerr By calling the $this->getSites(); method the this->sites array wiill be filled with sites data.
There was a problem hiding this comment.
@jmolivas a proposal by nvaken here hechoendrupal/drupal-console#3761 to have a single point of access to sites property.
Theoretically, the $this->site can be empty, if we didn't call $this->getSite() before in https://github.com/hechoendrupal/drupal-console-core/blob/master/src/Command/Debug/SiteCommand.php#L70
There was a problem hiding this comment.
@LOBsTerr What I mean is to add the $this->getSites(); above this line:
if (!array_key_exists($site, $this->sites)) {
Then $this->sites will contain data since $this->getSites(); call will add that.
There was a problem hiding this comment.
So what you’re saying is call $this->getSites() without assigning it to $sites and just keep getting data from $this->sites directly? I think assigning it to a local variable (like @LOBsTerr did) is best practice performance wise, isn’t it?
There was a problem hiding this comment.
@nvaken exactly I was trying to say that but is true local scope var is faster than the class one, we can keep this PR as it is.
No description provided.