Visibility of reset functions in Active Record class

[cs-rhett]

As of 2.0.3, visibility is set to protected on the active record reset functions (_reset_select(), _reset_write()). I realize this conforms cleanly with the previously-unenforced private status of these functions -- but do we actually need/want protected visibility now that we're enforcing visibility in PHP5?

Seems to me that there should be a way to uniformly clear query-related active record bits from controllers/models/libraries and opening up these functions is an easy way to do that.

Reference: system/database/DB_active_rec.php - Lines 1995 and 2027

[rsanchez]

+1

[derekjones]

Can you describe a use case for me where you would call this from your code instead of the result of a public method's action? I'm not necessarily disagreeing with you, I've just never encountered a need to interact that directly with AR data bits. I can keep the ones I want to keep with AR caching, and the others are destroyed after I use them.

[rsanchez]

I sometimes want/need to generate a SQL query without actually running it:

$sql = $this->db->_compile_select();
$this->db->_reset_select();

Unless there's some other way to do this?

[derekjones]

You know, Wes Baker and I were actually talking about that very thing a few days ago - that it would be nice to use AR to create queries that aren't executed, for instance, to write out statements to a file for batch use later. I think there's a way to accomplish that with a new public method rather than making us do a dance (our solution wasn't any prettier).

[rsanchez]

A new public method for this would definitely be a welcome addition!

[kylefarris]

I completely agree. I'm actually using compile_select in several parts of my code (even though I guess I shouldn't be). VERY useful for creating really complicated SQL. In my case, I'm actually using MySQL's match against stuff in a very large query but I still wanted to use AR for all the other parts. For some reason (it might have to do with 2.0.2 bugs...) I couldn't pass my match against code to the where method without it escaping things I didn't want it to escape.

So yeah, +1 for a new public method for getting the compiled SQL before running it.

[derekjones]

Would someone like to take a crack at it? :)

[ruthlessfish]

What about adding another parameter to get() that allows you to get the query string instead of the result? Something like this: https://github.com/bubbafoley/CodeIgniter/commit/e5b58bbdce93ba50d2430568ce6ef35b243bae98

then you can do this:

// normal
$query = $this->db->get('mytable');

// new
$sql = $this->db->get('mytable', null, null, FALSE);

[kylefarris]

I'm working on this right now. I'm added a new method called get_query(). I'm about to commit and request a pull. Really super basic. It takes an optional table parameter and an optional boolean parameter that will call _reset_select().

[derekjones]

I think I prefer the separate method, just so you don't have to fill all of those parameters.

@kylefarris - would get_compiled_query() be a better name (or something similar)? get_query() to me doesn't indicate that it's a method that will not execute the statement.

[NTICompass]

I was using _compile_select() for my Subquery library, so it would be really nice to have a public method to get a query without actually running it.

[bertiful]

@derekjones, good choice for get_compiled_query.

[rsanchez]

get_compiled_query would presumably only work with SELECT statements, right? I don't disagree with @derekjones on the awkwardness of the extra param, but I DO like the idea of being able to also get INSERT and DELETE queries.

[NTICompass]

For INSERT there is always $this->db->insert_string();.

[rsanchez]

DUHR. My bad.

[derekjones]

But insert_string() is not built from AR where(). There should be one AR method that returns the compiled query for any AR-built query.

[NTICompass]

@derekjones: True. I do agree we need a public version of _compile_select().

[kylefarris]

I'll work on getting the uuber "all-powerful" get_compiled_query() public method tonight when I'm not on the clock at work. I'll keep you updated on it. Should be awesome.

[bertiful]

I like the confidence, no homo !

[kylefarris]

I had to use the urban dictionary on the phrase "no homo". Do I get -1 internetz for that?

[bertiful]

Don't worry - it's probably best to stay away from these. Although, I didn't realize it was an internet term. Thanks to Urban Dictionary, I now know:

The phrase that, when uttered in conversation makes the speaker sound gayer than if he or she had not said it.

[NTICompass]

I love you guys, no homo.

[kylefarris]

Github needs a +1 mechanism for comments. I would totally +1 your comment.

[cigoe]

👍

[bertiful]

Looks more like a penis than a thumb, no homo.

[cs-rhett]

LOL. Gotta love the wandering path of brilliant minds - from reset function visibility, to capturing generated SQL, to "no homo". Seriously, though, thanks guys for all the input and help on this.

Personally, I think get_compiled_query() is a brilliant idea, and it's a lot easier to add a new function than refactor three existing ones but I still think there would be a benefit for refactoring to specifically make reset_write(), reset_select() and compile_select() publicly-accessible functions. That would give full flexibility to do just about anything you might need with AR from the public scope.

I know I've used each of these at various times for dynamic AR-based database operations and I've also seen them used in third-party libraries that integrate closely with (but don't directly inherit) the AR class (DataMapper-ORM in particular comes to mind). Capturing the generated statement is important - that's probably the biggest usage scenario - but it's not the only one.

I'd be willing to take a crack at changing the scope and re-factoring, if that solution is appealing to others... ?

[kylefarris]

@cs-rhett, I'll take into account your ideas when working on this tonight. I think a publicly-accessible method reset_query() would make sense. When using get_compiled_query() for inserts and updates, devs would be forced to use the set() method as opposed to passing an array like I think most people do.

Basically, instead of this:

$this->db->insert('foo', array('bar'=>4,'baz'=>true));

We'd have to do:

$query = $this->db->set(array('bar'=>4,'baz'=>true))->from('foo')->get_compiled_query('insert');

It might make sense to allow a second parameter to act as the 'for' since all queries required a table to select from. So, it could be a little less typing:

$query = $this->db->set(array('bar'=>4,'baz'=>true))->get_compiled_query('insert','foo');

Sorry, my mind is meandering. I think that's a bit sloppy as is. It would be nice to have a single method to get a compiled query, regardless of type, but then you have to remember unusual parameter orders. I suggest either this:

$query = $this->db->set(array(
    'bar' => 4,
    'baz' => true
))->get_compiled_query(array(
    'type' => 'insert',
    'table' => 'foo'
));

or:

$query = $this->db->set(array('bar'=>4,'baz'=>true))->get_compiled_insert('foo');

My preference would be the latter. What type would you prefer?

[cs-rhett]

@kylefarris, you rock - that's a perfect solution. And I agree, the latter is best. Nice, clean approach consistent with CI's existing AR conventions.

[kylefarris]

Cool, I'll make:

• get_compiled_select($table_name = ""[, $reset = FALSE])
• get_compiled_insert($table_name = ""[, $reset = FALSE])
• get_compiled_update($table_name = ""[, $reset = FALSE])
• get_compiled_delete($table_name = ""[, $reset = FALSE])
• reset_query()

The reset parameter is optional and would allow the dev to get the query string and reset the AR query in one step without having to explicitly call the public reset_query() method afterward (if desired).

Simple enough, I think.

[NTICompass]

@kylefarris: That's perfect :-)

... no homo

[NTICompass]

Once this gets pushed, I can fix my Subquery library!

[derekjones]

Kyle, you rock, thanks for taking charge here. Don't forget documentation! :)

[kylefarris]

Of course... documentation and changelog. :-)

[kylefarris]

Alas, I'm done with it except for one more piece of documentation. It's 3am, got work tomorrow... I'll get it up tomorrow evening, I guess. Bummer. Tested it thoroughly, though, and seems rock solid.

On a side note, I'm a n00b to gitflow so if any of you can answer this question, it would be awesome...

I currently have a pull request going for another bit of code that I did for the Form_validation library. If I commit my changes on this, the commits will be added to that pull request. Do I need to make another branch in _my_ fork (and if so, should I name it "issue-276"?), make my changes there, commit, and then do a pull request to the official develop branch?

Thanks.

[derekjones]

No rush Kyle, it sounds great.

We like to use Git Flow (video) as a branching model. Essentially you would make a new "feature" (sometimes called "topic") branch in your repository for each major modification. You could either keep them separate or merge them into the develop branch of your repository. The key though, whatever you decide, is that when you submit a pull request, look at the change set preview in GitHub to see what's going to be pulled. At that time you can modify what will be submitted and what won't, so that only those changesets that deal with the specific feature are pulled.

[kylefarris]

Thanks Derek, that helps.

[kylefarris]

:)

#307

[kutothe]

Can't wait for this to get merged in. Thanks!

[derekjones]

Rocking, kyle. There are a number of deletions that look like they don't belong, I think probably the result of not merging your feature branch into develop before the pull request?

[kylefarris]

You'd be right Derek, I was just noticing that. Gah... I'll get git right eventually. Should I fix it and re-commit the file? Should make merging easier...

[derekjones]

I think if you pull and merge develop from here into your local develop branch, and then merge your topic branch into develop and submit the pull request from there that might work best? Either way it will be merged with develop here, so long as we aren't regressing, the method you choose probably doesn't matter a whole bunch.

[kylefarris]

Hmm... okay. I'll see what I can do tonight, thanks.

[kutothe]

Any luck with trying to properly merge this in Kyle? Not trying to be pushy, just excited for this to get merged in :)

[kylefarris]

Yeah dude. Hopefully should get merged in today. We'll see. Check out the pull request for more details:

#307

[NTICompass]

What's the status here, was the pull request accepted?

[derekjones]

Sorry, I saw Greg was working on that and it slipped past me. He's had a full plate this month, but I'll ping him when I get a chance to bring me up to speed and I'll take another look.

[NTICompass]

@derekjones: Thanks. I don't want to be a bother, I was just curious.

[derekjones]

Not a bother at all, I've been eager for this to be in myself. ;)

[kutothe]

Any word on this?

[derekjones]

I'm still intending to review it before EECI, the Reactor crew has been covered.

[kutothe]

Sounds good, thanks Derek!

[kutothe]

Woohoo, thanks guys!

[kylefarris]

:D

[bertiful]

🆒 !

[NTICompass]

:-D

[Relequestual]

I know how old the last comment, but if anyone is looking at this, I went about it a slightly different way.

I needed to get the sql to a batch insert, so called _insert_batch, passing the ar_set. I then reset that ar_set back to a blank array after. Maybe that could help someone. =]