Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Potential Features: Controllers and Validators #8

Open
rgvy opened this issue Jul 14, 2017 · 11 comments
Open

Potential Features: Controllers and Validators #8

rgvy opened this issue Jul 14, 2017 · 11 comments

Comments

@rgvy
Copy link

rgvy commented Jul 14, 2017

Hello

I am curious if potential features for this framework might include base class Controllers and/or Validators. I find that much of the code that I have to write falls in to either of these categories. So far I've just coded brutishly without much regard for scalable design patterns or clear semantics. When the project stabilizes I'd like to rewrite my core business logic (i.e. user driven CRUD) in a more maintainable way. Therefore, I am curious if well-designed base Controllers/Valdiators are appropriate for this project. I totally understand if they are not since these might need to be too specialized and/or complex... If that's the case, are you aware of any other frameworks that handle these tasks particularly well?

Thx

@ocram ocram added the question label Jul 14, 2017
@ocram
Copy link
Contributor

ocram commented Jul 14, 2017

Could you give any more concrete examples of what you're trying to do or how these additional components would be helping you? Where do you know this from? Are there any implementations that you like?

@rgvy
Copy link
Author

rgvy commented Jul 14, 2017

Basically just providing general CRUD user flow for various objects stored in the db. Examples: create/edit/delete profile/blog post/etc... Perhaps some of the logic in these controllers can be reused and base-classed. Ex. validating the posted data matches the db schema and then redirecting to the appropriate error routes. However, I would not be surprised if each application is much too specialized for there to be useful framework to handle these tasks. As for an example implementation, I am aware of Ruby on Rails. However, I believe that Rails is quite complicated and bloated and doesn't match this framework. So I am curious if there is something useful out there that might be good to learn from? I've taken a very quick look at silex and lumen, but neither fx seems particularly useful in this area. Perhaps these features are much too complex to fit with this very simple and elegant framework... If this is the case, I am quite content to write my own specialized code, but I am forced to wonder if there is an ideal and reusable solution.

@ocram
Copy link
Contributor

ocram commented Jul 14, 2017

Well, yes, writing your own classes to do this would still allow you to re-use much of your code. It's just a different level of abstraction. But if we could really make this fit in here, we should do that, of course.

Perhaps we should indeed look at the solution in Rails.

In general, I'm definitely open to such a feature, if we can implement it in a good way.

Otherwise, as we agreed, you writing that layer for a specific application yourself is still a good solution and will allow for a good amount of code re-use, though it's not optimal because sharing that layer across applications would be the ideal solution.

@rgvy
Copy link
Author

rgvy commented Jul 14, 2017

There are hundreds of php frameworks, and perhaps it was a long shot to think there was already a highly-modular and elegant component.

I will keep my eyes open and if I spot something worthy (or make something), will share.

Thx

@ocram
Copy link
Contributor

ocram commented Jul 14, 2017

That layer will definitely depend on the front-end framework that you use, right? For example, outputs for Bootstrap 2, 3 or 4, other frameworks, or plain HTML could be supported. Though plain HTML is a bit difficult because you also need the correct CSS classes that have adequate styles.

In the end, it might not be too hard to build a custom solution for this. But we should discuss this so that we can agree on the overall concept of how to do this, I guess.

@rgvy
Copy link
Author

rgvy commented Jul 14, 2017

Actually, I thought the view could be excluded from this layer. For example, imagine a controller with only these policy requirements:

  1. The name attributes on form variables (or JSON property names for async) [input]
  2. The variable names set in the TWIG context (or JSON again) [output]
  3. These names match column names in some table
    Any view code could obey these policies and wouldn't depend (much) on any given HTML

But maybe the layer could be even thinner. A base controller could have overridable properties for success route, validation error route, validation function, etc...

Although, perhaps I am thinking too eagerly... I will look at a "Hello World" CRUD Rails app and see if there is anything to learn.

@ocram
Copy link
Contributor

ocram commented Jul 14, 2017

That sounds good. I like the idea of excluding the view layer. But indeed, there are many solutions possible, some more efficient, some less.

Looking forward to hear about your experience with Rails!

@rgvy
Copy link
Author

rgvy commented Aug 8, 2017

Hello

I have done some research on Rails, Laravel, and a couple other frameworks and unfortunately don't have too much to recommend.

Many of these "full-service" frameworks use tight-coupling between model classes, database schema, and other components. This is particularly true for Rails which automatically generates consistent database migrations, model class definitions, and more. Clearly, features like these are too much and too complicated...

One thing I love about this framework is how the DB component uses "plain old php arrays" as the basis for its data exchange. It seems natural to use the plain old php arrays as Value Objects (VO) in a Data Access Object (DAO) design pattern. The DB component handles much of the boilerplate DAO pattern. Basically the only work a DAO wrapper needs to do is:

  1. Implement various $dao->get(...) methods which vary on business domain
  2. Implement $dao->save($vo) which switches between $db->insert and $db->update depending on the existence of a primary key in the VO

There isn't much overhead for developer users to implement these. The only possible enhancements to DB I can think of to ease this are:
a) Implementing $db->replace which isn't as efficient as,
b) Implementing a method which uses INSERT ... ON DUPLICATE KEY UPDATE as the query pattern
Of course both are already possible via $db->exec...

Another area I looked at was the role of the controller. It turns out that most useful and reusable controller methods already appear in $app, for example $app->view, $app->redirect. Personally, I have used "composition over inheritance" in my controllers to reuse this functionality. Without creating very tight links between other framework components, it does not seem possible to provide anything useful for a controller base.

One task that controllers do is validation, and this is an interesting area to consider. A Validation component seems like a good candidate for an enhancement. It can exist as a separate and decoupled component. Laravel has an interesting implementation (https://laravel.com/docs/5.4/validation#quick-writing-the-validation-logic).

I am building such a component. It is designed to apply various rules to values received from $app->input methods. It also collects errors and displays them with $app->flash. When I am finished, I will be glad to share it with this framework. Hopefully it will be as well-designed and useful as the rest of the framework.

@ocram
Copy link
Contributor

ocram commented Aug 10, 2017

Thanks for your research!

You say that one would have to switch between insert and update in a save method for a thin wrapper that implements the DAO pattern. Implementing such a replace method, as you suggested, is definitely something that I'd be open to. That is what other database engines or standards call "merge" or "upsert", if I understand you correctly.

For those "merge" or "upsert" operations, there are some slight variations, however, so I could even imagine separate methods insertOrReplace, insertOrMerge and insertOrIgnore, for example.

Most importantly, what the replace method does internally should be of no concern to the user of that method. All you need to know is the API of that method and then you will be able to use it without wondering about the internals, being assured that the method will comply with the contract agreed upon. So that method could very well use INSERT … ON DUPLICATE KEY UPDATE for MySQL and use other syntax for database engines which don't support that feature.

In general, what a "merge" or "upsert" method needs in addition to the data from the insert or update methods is (a) the condition of when to perform the update instead of the insert (e.g. primary key collisions?) and (b) the mappings of values to update.

The validators sound interesting as well. So you want not just the enforcement of specific data types, as provided by the $app->input() helper, but specific checks such as for the minimum or maximum length, right? What I don't like that much about the Laravel solution is the reliance on plain strings without any possible checks from the IDE or compiler. So instead of min:10|max:255 I'd prefer something like ->withMinLength(10)->withMaxLength(255), but perhaps that is not really feasible.

After all, maybe you don't actually have to prevent any tight coupling between the framework and your base controller. If only the base controller is tightly coupled to the framework, you could simply regard it as an extension of the framework. If something changes later, you'll have to adjust calls, but only in one place. Sometimes convenience (of development) and flexibility don't go together that well.

@rgvy
Copy link
Author

rgvy commented Aug 14, 2017

Thanks for your consideration!

For a "merge" or "upsert", the only other insight that I can offer is that REPLACE is non-standard SQL, and that these may only work if a table has a primary key. Enforcing the primary key requirement might be tricky. Many other frameworks couple together DB/ORM with entity model/class definitions. Tight coupling like that would provide for such enforcement.

What I like best about this framework is that it is plain old php! There are no new pseudo-languages to learn. There are no "magical" command line utilities to generate boilerplate code. It is simple and efficient. Perhaps enforcing primary keys can be done the same way. Consider this example:

abstract class DAO
{
    //fields below are set in constructor (not shown)
    private $db; //$app->db
    private $table; //entity table name
    private $pk; //primary key column name

    function get($id)
    {
        $vo = $this->db->selectRow("SELECT * FROM $this->table WHERE $this->pk = ?", [$id]);
        return $vo;
    }

    function save($vo)
    {
        $id = $vo[$this->pk];
        if ($id > 0)
            $this->db->update($this->table, $vo, [$this->pk => $id]);
        else
            $this->db->insert($this->table, $vo);
    }

    //delete method not shown
}

(Reference: http://www.odi.ch/prog/design/php/guide.php - it's old but still valid)
In this case the VOs are plain old php arrays as used by $app->db. The developer can subclass the DAO to include custom get methods (such as getAll, getNewest, getByUser...)

Regarding validation, I totally agree that Laravel's plain string parameters are messy. Also, validation tends to occur within a single method call, which seems to be unnecessary. I like the chained method call approach, ->withMinLength(10)->withMaxLength(255), but I'm curious what you think about this one:

Each rule is its own method call, and each call takes parameters which determine the error severity and message. These errors are collected and could be passed to $app->flash or somewhere else. ($app->flash would need to be modified to support multiple errors per severity level -- In the meantime I delimit them with a pipe character | and split them in the view template). Here's an example validation:

$v = new Validator();
$v->required($name, DANGER_FAIL, 'Please include your name');
$v->maxLength(255, $title, DANGER_FAIL, 'Title should be less than 255 characters');
$v->minLength(10, $title, WARNING_NOFAIL, 'Longer titles are more descriptive');
if ($v->hasErrors)
{
    $v->flashErrors($app->flash());
    $app->redirect('/posterror');
}
else
{
    $app->flash()->success('Your post has been submitted');
    $app->view('/user/posts');
}

I had also considered extending $app->input but think it might be better/simpler to leave these features separate.

Perhaps also the developer user can subclass the validator for reuse:

$successRoute = '/user/posts';
$errorRoute = '/posterror';

$v = new PostValidator($app);
$v->post(); //takes fields from $app->input;
$v->validate($successRoute, $errorRoute); //this could even be included in the base class

Of course each developer is free to implement these solutions independent of this framework (which is exactly what I have done). However, I think these two solutions fit very nicely with this framework and provide useful, reusable features. I am happy to contribute if they're good enough :)

@ocram
Copy link
Contributor

ocram commented Aug 24, 2017

Thank you!

I agree with everything you said and your DAO class looks good.

The Validator class looks fine as well, although there are two things that I don't like that much:

  • I thought that we would be passing the keys from the $_GET or $_POST arrays to the validator only, instead of passing the value itself (which must have been captured in a variable first). Passing the keys only avoids nasty warnings whenever some value does not actually exist. But I see why you did it that way. The only other solution would be having a GetValidator and a PostValidator, which I'd like, but they would be less flexible and wouldn't allow for mixed usage. The only solution that would allow for mixed usage would require you to specify whether you want a value from the GET or from the POST data, which is another parameter.
  • The code is perfectly readable as it is. But I think it could be easier to write if the severity wouldn't have to be specified via a constant but could be included in the method name, e.g. maxLengthOrFail. Not really sure about this, though.

By the way:

What I like best about this framework is that it is plain old php! There are no new pseudo-languages to learn. There are no "magical" command line utilities to generate boilerplate code.

Good point, I should add that to the project description :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants