[BugFix] Little bug fix inside product model

[koemeet]

You could not set a master variant if the non-master variant was already added to the product. This patch fixed it.

[pjedrzejewski]

We do not need this else, cause we have the return statement above. :)

[koemeet]

Yes I need to wrap the second condition in a seperate if statement. Had it like that before but refactored without thinking. :P

[koemeet]

@pjedrzejewski Ready for merge

[stloyd]

There is no need for true === as isMaster() returns boolean.

[koemeet]

@stloyd Removed true === from condition.

[koemeet]

@pjedrzejewski @stloyd Can you review please. I think it is ready for merging.

[stloyd]

TBH. I would merge both ifs as there no point to split cause in first if you do nothing if second one will not pass.

[koemeet]

But then you would end up with two if statements checking if the product contains the variant. If that is cleaner, I can do it like this. Is this what you're proposing: (performance wise, this would be slower)

if ($this->variants->contains($masterVariant) && $masterVariant->isMaster()) {
    return $this;
}

if (!$this->variants->contains($masterVariant)) {
    $masterVariant->setProduct($this);
    $this->variants->add($masterVariant);
}

[stloyd]

Sorry, I'm sleepy so I can be wrong =) but what about?

if (!$masterVariant->isMaster()) {
    $masterVariant->setMaster(true);
}

if (!$this->variants->contains($masterVariant)) {
    $masterVariant->setProduct($this);
    $this->variants->add($masterVariant);
}

ps. phpspec would be your friend here =)

[koemeet]

Yeah me too. But this would be better, we do not even need the top lines. Also, the check for isMaster is also not really necessary, right?

[koemeet]

@stloyd Code is cleaned up now, I removed the if statement for isMaster, because it will always be true.

[stloyd]

@pjedrzejewski Looks good for me.

[pjedrzejewski]

Thanks @steffenbrem!

[koemeet]

@pjedrzejewski @stloyd But what if the product already had a masterVariant which has isMaster set to true? Wouldn't this fail when you call getMasterVariants? You could get unexpected results then. I think we first have to make sure that when the product contains any masterVariant, it's isMaster property is set to false.