Added an "update" function.

[cryophallion]

The issue is that if someone uses the table for any business logic (and therefore those fields aren't reflected in the edit), those changes get wiped out by the recordFromPost method. So, a simple update function is all that is needed here. Also useful for business logic in controllers. Also added a single field array static function.

[jordanlev]

The reason I have the recordFromPost function is because just passing in the entire $_POST array is a security risk: a user could alter the posted data and include a field that you didn't intend to be saved to the database (this is called "parameter tampering"). To avoid this, it's best to utilize a whitelist of fields that you are allowing to be updated. The recordFromPost function does not just take every field in $_POST, but instead loops through the model's $fields array and only grabs those fields from $_POST (this $fields array is the whitelist in this case). By default, the $fields array is populated by every database field in the table except for the primary key id field.

SO, if you want to exclude fields from being considered by the $recordFromPost function, what you should do instead is override the $fields array in your model subclass and list out only the fields that you want considered. For example:

<?php defined('C5_EXECUTE') or die(_("Access Denied."));

Loader::library('crud_model', 'automobiles');
class CarModel extends BasicCRUDModel {

    protected $table = 'automobile_cars';
    protected $order = 'name';
    protected $fields = array('name', 'year'); //the table fields that will be saved to the database when the add/edit form is posted (we are listing them out explicitly here because we do not want every table field included for this model)

    //...

If that solution is not to your liking, you should instead override the save method in your model subclass to handle these exceptional cases (because in my experience they are not the norm).

[cryophallion]

Thank you for your detailed response. I'll absolutely use the workaround, I thought it was pulling field names from the database itself. I knew of the issue, but that is definitely a great solution to the tampering.

Would a pull request for the basic (but often used) function I added for creating an array of a certain field (like an array of ids, most often) be accepted?

Thanks loads for making this crud available, it's definitely a huge help and timesaver for single page projects.

[jordanlev]

You're very welcome!

Could you explain to me a bit more about your use case for the helper function that creates an array of certain fields? What does it give you that you can't get from populating the $fields property of the model itself?

[cryophallion]

I'm not sure I explained it well in the code, it's a slightly different function (and I was going to do it as a separate pull request, but I am still new to git, so it introduced it to this pull).

A common need is for an array of all the items of a certain field. For example, if you wanted an array of the ids of a model, or all the names for something. Your SelectOptions does something similar, but requires a Key field. This is a simple array of just a single fields values. Handy for lots of processing. I also sometimes use it for populating IN queries if I need a subset result.

So calling it with a db result array will return an array of the field values for that field, ie array(row_1_value,row_2_value,...)

[jordanlev]

Ah, I see -- you want a "pluck" function (e.g. http://stackoverflow.com/q/10659875/477513 ).

I agree that that can be very useful, but I do not think it belongs in the crud_model class because it is more of a general utility function and not specific to CRUD operations (retrieving and saving records to/from the database).

I'd recommend just adding your own library of utility functions to your package (or at the top-level of your site) for such helpers. Or if you only need it for one data model, put it in the model subclass itself.

[cryophallion]

Ok, I'm more than happy to do that. As a rebuttal, I point to your SelectOptionsFromArray though. That was what made me think of it, and sometimes, minimal formatting makes sense in the model (it doesn't really belong in the controller, since it's still getting data, and clearly not in the view). Plus, that model will already be loaded, so reduces overhead (minimally, but I'll take what I can get).

Thanks again for taking the time to go back and forth with me. I appreciate your project, and I'll let you know if I find any other little things if I can try and contribute back to you. It's nice to be able to constructively discuss why and how we do things.

[jordanlev]

Hey, Thank you for posting the pull request and the conversation -- I love discussing this stuff too.

You totally got me on the selectOptionsFromArray thing -- yes I see how that is not related to CRUD operations. I guess the difference is that that's something I use a LOT on every single project I build with this boilerplate. Maybe another way to put it is: I only have things in the underlying libraries that are actually used in the demonstration code.

So perhaps if you have a new feature that you think could be useful to other people and that feature relies on the underlying "pluck" functionality, then go ahead and do a pull request with both the pluck function in the crud_model library AND the feature that uses it in the "automibiles" implementation (like a handy new field in one of the add/edit pages or something?).

Happy to bounce some ideas back and forth with you first if you'd like.

Thanks.

[cryophallion]

Well, I have two ideas in reading what you said:

1. I could make a simple change and pull request to make the key parameter for selectOptionsFromArray default to false (or null, I'd have to check which one works properly). That would tweak it enough to do both functions, without being any more out of context than it is.
2. The proper response is for me to post it IN context somewhere. And I am going to do that instead. The place I most often use it is in a library I've made that does query building. I've just uploaded it here and I'd appreciate any thoughts you have on it: https://github.com/cryophallion/c5-QueryBuilder

[cryophallion]

Ok, so I have noticed I still need to add an update function to the Crud Model for several of my sites, for use by the business logic.

I'll explain: A lot of the time I use the controller to set fields in the database that aren't a part of the backend form. I can give examples if you need them, but it happens a lot. The issue comes in when I need to save this info to the db from the controller. Clearly, this falls to the model. Now, if I have restricted the fields in your save function to just the backend form ones (so the business logic ones don't get nulled), there is no way for me to update these. And I don't always want to update all of them, just a subset. This happens in multiple models of mine, so making multiple functions seems very against DRY principles. Even if not used for the forms, but just left as the function itself, it would be useful. Does that make sense?

[jordanlev]

Still having a hard time envisioning the scenario you're describing. If you could provide an example, that would be great. But from what I can gather, I'm tempted to say that if you need to save things that aren't coming from the user's POSTed form data, then you should probably be doing that kind of thing in the model's save method instead of in the controller. (But maybe an example would make it clear to me why that isn't possible).

Regardless, though, this is simply not a situation I've ever run into in my own projects, so I'm not likely to add it into this boilerplate code. I'd recommend you just keep that modification in your own fork of the project. You can still bring in updates I make to this into your fork so it stays up-to-date, but also retains your customization.

[cryophallion]

I'll explain, but let me preface it. I'm more than happy to keep my own concept version (although I'm loathe to do so to some extent - I just wish php did multiple inheritance easier so I could have it extend both my class and your class together). I totally defer to you in all these things (I mean, I have to technically, but I don't hold a grudge or care). This IS your project, and if you don't have a need, then it doesn't need to go in, and your style takes precedence. My sole thoughts were: Hey, I need to do this, someone else may need to, and this awesome library might benefit from them, and as a bonus, I get to give back to a project that saved me boatloads of time. Score! That's how I view my contributions. Thanks for taking the time to review them all, and respond. Honestly, it means a lot.

Now, back to MY situation. I'll use a very specific situation: There is voting by a group of people on images. After a certain date, those votes are compiled, and the vote added to the images. In order to not have the votes recompiled every load after the date, and so that I don't have to pull in an image or image set to determine if the voting has been added to them, I set a flag on the event: VotingSet. So, when the event page is loaded, if the date is greater than the end voting date, it will check that flag to see if it needs to compile the votes. I use that in several locations in this app, and so I find it useful. Again, only when business logic is required.

Anyway, I wanted to explain the logic, but I understand you don't want to add it since you don't have a need for it. Thanks for giving me a chance to try and give back. If I think of anything else minor (doubtful), I'll let you know.

[jordanlev]

I think I understand your situation a little better, and I think the most elegant way (in my opinion, which is of course completely subjective) would be to add a 2nd argument to the CrudModel base class's save method which would tell it to not whitelist the $post fields. This would allow you to populate the data that you send to save any which way you want, and have it be directly INSERTed/UPDATEd to the database. You'd still want to whitelist the fields yourself (so perhaps I should make the recordFromPost method protected so you could call it yourself in the model), but you could then add your own data after the whitelisting. For example, you could override the save method in your model class like so:

public function save($post) {
    $fields = $this->recordFromPost($post);
    $fields['VotingSet'] = true; //or whatever
    return parent::save($fields, false);
}

Would something like that address your situation in a straightforward way?

[cryophallion]

I think the core of save is perfect. Its overriding the recordFromPost that causes the issues. Just making that optional would do it. maybe something like this?

public function save($post, $ignoreWhitelist=false) {
    if(!ignoreWhitelist){
        $record = $this->recordFromPost($post);
    }

    if ($this->isNewRecord($post)) {
        $this->db->AutoExecute($this->table, $record, 'INSERT');
        return $this->db->Insert_ID();
    } else {
        $id = intval($post[$this->pkid]);
        $this->db->AutoExecute($this->table, $record, 'UPDATE', "{$this->pkid}={$id}");
        return $id;
    }
}

[jordanlev]

Yes, I think this is the most elegant solution. I've added this to the repo: https://github.com/jordanlev/c5_boilerplate_crud/commit/9c43084a0fdc7f839f00fd714f90563ebcbeee89

[cryophallion]

Awesome, that will help loads. More clean code in your version as well. Thanks for adding this.