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

Already on GitHub? Sign in to your account

Fix for Issue #276 #307

Merged
merged 14 commits into from Oct 18, 2011

Conversation

Projects
None yet
7 participants
Contributor

kylefarris commented Aug 26, 2011

I've added the following methods to the Active Record class:

  • get_compiled_select()
  • get_compiled_insert()
  • get_compiled_update()
  • get_compiled_delete()

The above methods provide a public API allowing you to get the SQL from an AR query without having to execute them first. They all take two parameters: ($table = '', $reset = TRUE).

I've tested them, somewhat, using the following tests which utilize philsturgeon's cli spark

class Test_ar extends CI_Controller {
    function __construct()
    {
        parent::__construct();
    }

    public function test()
    {
        $this->load->spark('cli/2.0.0');
        $this->load->database();

        /*******************************
         * Test Select
         ******************************/
        // SELECT fee, bar, fo FROM foo WHERE fo='fum'
        $this->cli->write($this->db->where(array('fo'=>'fum'))->select(array('fee','bar','fo'))->from('foo')->get_compiled_select(false,false) . "\n");
        $this->db->reset_query();

        // SELECT * FROM testing WHERE code='awesome'
        $this->cli->write($this->db->where(array('code'=>'awesome'))->get_compiled_select('testing') . "\n");

        /*******************************
         * Test Insert
         ******************************/
        $this->db->set(array('bar'=>'baz'));

        // INSERT INTO foo (bar) VALUES ('baz')
        $this->cli->write($this->db->get_compiled_insert('foo',false) . "\n");

        /*******************************
         * Test Update
         ******************************/
        $this->db->set(array('fee'=>'fi','jack'=>'And the Beanstock'))->where(array('fo'=>'fum'));
        $this->db->from('foobar');

        // UPDATE (foo, foobar) SET bar='baz', fee='fi', jack='And the Beanstock' WHERE fo='fum'
        $this->cli->write($this->db->get_compiled_update() . "\n");

        /*******************************
         * Test Delete
         ******************************/
        $this->db->where(array('fo'=>'fum'));

        // DELETE FROM foo WHERE fo='fum'
        $this->cli->write($this->db->get_compiled_delete('foo'));
    }
}

You can run those tests by installing the cli spark, and putting that controller into one of your projects. Then run it in the command line like:

cd /home/project/path
php index.php test_ar test

Once/if this gets merged, I'll write some real unit tests for these methods.

I've also added the a publicly-visible method that allows you to reset the AR object: reset_query().

Naturally, changelog and user guide has been updated.

Kyle Farris and others added some commits Aug 26, 2011

Kyle Farris Added get_compiled_select(), get_compiled_insert(), get_compiled_upda…
…te(), get_compiled_delete(), and reset_query() methods. to the Active Record class.
0c147b3
@kylefarris kylefarris Small formatting fix. 0a3176b
@kylefarris kylefarris Updated the changelog to reflect new Active Record methods: get_compi…
…led_select(), get_compiled_delete(), get_compiled_insert(), get_compiled_update(), and reset_query().
c1ee04b
@kylefarris kylefarris Updated the Active Record documentation to reflect new publicly visib…
…le Active Record methods: get_compiled_select(), get_compiled_delete(), get_compiled_insert(), get_compiled_update(), and reset_query().
0f35b6e

+1'ing this request.

For core classes, it's best to use protected so these variables can be overridden.

you may be reverting a previous merge here with the int casting.

Owner

kylefarris replied Aug 31, 2011

I'll make the change.

Nitpick: unnecessary newline

Owner

kylefarris replied Aug 31, 2011

Nitpicks are good.

can you break this out to be more lines, less long?

eg:

$sql = $this->_insert(
    $this->_protect_identifiers(
        $this->ar_from[0], TRUE, NULL, FALSE),
         array_keys($this->ar_set), 
         array_values($this->ar_set
    )
);
Owner

kylefarris replied Aug 31, 2011

Yeah, no problem.

same as above. More lines, not one long line.

Nitpick: adding unnecessary newline.

Contributor

gaker commented Aug 31, 2011

also, if you can get my nitpicky items done, merge in the current develop branch, and we can easily merge it in! :D

Thanks so much Kyle!

Contributor

kylefarris commented Aug 31, 2011

Alright, we should be good.

kutothe commented Aug 31, 2011

Nice! Good job :)

kutothe commented Sep 6, 2011

Is there anything holding up this from getting merged in, or does the review process simply take a little while?

Contributor

kylefarris commented Sep 6, 2011

It probably just takes a while. A lot of code to merge in. Lots of pull-requests recently (which is a good thing!). Usually the easier ones (patches, bug fixes, typos, formatting, etc...) get merged in first because they don't require a lot of scrutinizing. I'd imagine new features are little bit more of a PITA.

kutothe commented Sep 23, 2011

Is there a nice clean way for me to merge this change into my CodeIgniter instance without causing problems when I pull updates from the develop branch?

Contributor

kylefarris commented Sep 23, 2011

Yeah, they merge it in... Wonder what's taking so long?

@kylefarris kylefarris closed this Sep 23, 2011

@kylefarris kylefarris reopened this Sep 23, 2011

Contributor

kylefarris commented Sep 23, 2011

Grr... that "Comment and Close" button is annoying...

Has this been pulled in yet? Someone told me this was accepted. I want to get my Subquery library updated & working.

Contributor

derekjones commented Oct 14, 2011

Kyle I was going to try to review this before EECI, and I see that now the request cannot be automatically merged; any chance of updating the user guide changes for the pull request?

Contributor

kylefarris commented Oct 14, 2011

What do I need to do get it to be able to be automatically-merged?

Contributor

derekjones commented Oct 14, 2011

I believe you'll just need to update and merge the 'isue-276' branch in your fork with the official 'develop' branch.

kutothe commented Oct 14, 2011

Do you also need to update the documentation of the new .rst files instead of the old .html documentation file?

Contributor

derekjones commented Oct 14, 2011

Yes, that would be part of the merge, that would likely need to be resolved manually by incorporating those few changes into the .rst source files.

Contributor

kylefarris commented Oct 14, 2011

Thanks for your help guys! I've made the necessary changes.

Can we please get this merged in quick so I don't have to re-update from development again. Thanks!

@derekjones derekjones commented on the diff Oct 18, 2011

application/config/migration.php
+
+
+/*
+|--------------------------------------------------------------------------
+| Migrations Path
+|--------------------------------------------------------------------------
+|
+| Path to your migrations folder.
+| Typically, it will be within your application path.
+| Also, writing permission is required within the migrations path.
+|
+*/
+$config['migration_path'] = APPPATH . 'migrations/';
+
+
+/* End of file migration.php */
/* Location: ./application/config/migration.php */
@derekjones

derekjones Oct 18, 2011

Contributor

Not really sure what GitHub is showing here. I don't see any diffs in the current develop branch and your isue-276 branch.

@derekjones derekjones added a commit that referenced this pull request Oct 18, 2011

@derekjones derekjones Merge pull request #307 from kylefarris/isue-276
Fix for Issue #276 - Thanks Kyle!
12e93fe

@derekjones derekjones merged commit 12e93fe into bcit-ci:develop Oct 18, 2011

Contributor

atiredmachine commented on 7611601 Jan 27, 2012

damn, there goes compatibility with my MySQL-specific Active Record add-on library.

Is this going to be put into the next stable release? I need to tell users of my Subquery library to use the "dev" version of CodeIgniter. Not that that's a bad thing, I'm just curious.

Any news on this... I'm looking for this functionality to extend https://github.com/IgnitedDatatables/Ignited-Datatables/ for filtering multiple aggregate queries that I want to join on to the main record set.

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