Skip to content

Conversation

natanfelles
Copy link
Contributor

@natanfelles natanfelles commented Dec 2, 2018

Description

I think these classes were done to be extended.

If you think there is no such need, please just close it.

Checklist:

  • Securely signed commits
  • Component(s) with PHPdocs
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@jim-parry
Copy link
Contributor

jim-parry commented Dec 2, 2018

Good intent, but this breaks all sorts of unit testing. Some of the classes might be abstract in a normal O-O world, but this is PHP & PHPunit's "alternate reality" :-/
We instantiate them to test them.

@natanfelles
Copy link
Contributor Author

Yeah. I think we could do something like:

function setUp()
{
    $this->abstract = new class extends AbstractClass {};
}

Then will pass.

@jim-parry
Copy link
Contributor

That feels "funny".
The fact that Model has a constructor tells me that is is meant to be instantiable.
Controller, on the other hand, is not meant to be instantiable. That has come in conversation before, with the intent being that an extending class might have a constructor, but we don't want to rely on that inside our class.
Message feels like a concrete class to me, given its flexibility.

I think it better to keep these as base classes, and not mess with trying to treat them as abstract. Fewer hassles.
It also looks like making the controller abstract messes up some of the output buffering - risky tests in travis-ci.

@jim-parry
Copy link
Contributor

@lonnieezell Do you think these make sense as abstract base classes?

@lonnieezell
Copy link
Member

The other question to look at, besides are they meant to be instantiated, is where or not they have any methods the extending class are required to implement. If the answer is no, to that, I don't see any benefit from making them an abstract class. The definition I've always understood for an abstract class was it was one that had abstract methods. None of these do, so I say keep them as non-abstract classes.

@jim-parry
Copy link
Contributor

So, not a good class for abstract classes, sorry.

@jim-parry jim-parry closed this Dec 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants