search

coral-erm / coral

CORAL ERM main repository
http://coral-erm.org/
Other
52 stars 64 forks source link

Adopt an MVC pattern #80

Open tuxayo opened 8 years ago

tuxayo commented 8 years ago

@jsavell

Right now, our data classes are sprinkled with view and business logic and our controllers and views are merged into one entity. This greatly complicates modifying code or adding new features.

Are there already discussed ideas about how to start progressing toward this goal?

jsavell commented 8 years ago

I feel like it has come up before in terms of better separation of the data layer, but I can't point to anything specific.

tuxayo commented 8 years ago

It seems we almost/already have a model as most of the DB access are abstracted by the DBService class and business classes like Resource, License, Organization that look like data access objects. What else is needed?

The other big challenge is to separate the business logic from the views. What general things needs to be defined here we can continue working on #78 ?

jsavell commented 8 years ago

The Data layer is the furthest along in terms of OOP, but it still needs a lot of attention. It's not using interfaces to define modeled data, is tightly coupled to MySQL, and hasn't yet been converted to use PHP's PDO interface, which will eventually be the only option.

Also, DatabaseObject is highly specialized in how it generates its models from a DB table's fields and join relationships, and isn't well documented. It seems most developers tend to just write queries directly in methods to access the data they need, regardless of whether there's a model for it. I've been guilty of it myself when there seems to be no other option.

Methods are overrun with configuration conditionals. The Resource class has to repeatedly ask if the license or organization modules are installed to figure out which query and fields it should be using to get the right data.

That's why we need a Repository model, so, for example, the Resource class and any other class that needs to can ask the singular LicenseRepo to getLicensesByResourceId(). The configuration decisions about what database the licenses live in, or if it's in a database at all, are made once and only by the Factory that provides the LicenseRepo to code that ask for it. And just as importantly, calling classes will no longer be querying the DB directly for data that another class is responsible for modeling.

That being said, I don't think any of that has to be implemented before we start on implementing an MVC pattern, because as you said, the data is technically already accessed by the controllers through classes (with a few exceptions). Most of the change over to MVC would impact the controllers and view rendering.