We have a problem with SubscriberInterface::getStatus() and setStatus(). They are documented as taking bool, which seems intuitive and sensible because it matches UserInterface::isActive() so you can write code like this from Subscriber::fillFromAccount():
$this->setStatus($account->isActive());
However there are also constants SubscriberInterface::INACTIVE and ACTIVE and this is what the code uses in the majority of cases - hence there are warnings for code that seems very reasonable.
$this->assertFalse($subscriber->getStatus());
| Comment | File | Size | Author |
|---|---|---|---|
| #3 | simplenews.subs-status.3103733-3.patch | 4.08 KB | adamps |
Comments
Comment #2
adamps commentedThere is a potential reason to have an integer status: so that we can extend the field to add a new value, for example, "unconfirmed". However there are two problems with that.
Subscriber::fillFromAccount(). This code wouldn't really work if the two fields have different legal values.Therefore I propose that we stick with the commented interface and make the field a boolean throughout. We have two options for the constants INACTIVE/ACTIVE:
Seeing as we are right now at the perfect time to make non-BC changes, I prefer option 1.
Comment #3
adamps commentedOr we can use bool on the functions but integer internally, which matches User.php and is fully BC. Here's a patch that tries that.
Comment #5
adamps commentedThat gets the tests working so I've checked it in. I'm happy to make further changes if anyone has any review comments.
Comment #6
adamps commented