Updating test case and a few fixes

[shama]

No description provided.

[tigrang]

I do have a request object within the DataTableComponent - unless this is to say its unnecessary?

[shama]

I don't think the extra param is necessary; especially being public.

But this change is actually a mistake thinking about it now. I should be calling initialize() in the test. I'll update the test and revert this. Your call if you think a request property should be available. If you do, I'd recommend making it protected.

[shama]

Actually don't merge this yet. I'll write some more extensive tests later and update this pull.

[shama]

I came across an interesting issue with using the paginator component. When the Controller->paginate property is set it overwrites the DataTableComponent settings. So if you do $this->paginate = array('limit' => 5); in your controller action, the component crashes. Got a test and fix for it coming up.

EDIT: Yes pointless to do limit => 5... but still an issue if adding conditions in an action.

[tigrang]

For component branch or master, or both?

edit: I can't reproduce

[tigrang]

I fixed the App::uses and removed constructor along with other stuff in my last commit.

[tigrang]

Also, the reason for extending PaginatorComponent was to keep both functionality? To do normal paginating and js paginating? If that's the case then I need to modify paginate() method a bit.

[shama]

Only with the component branch. Pushing the progress I have so far...

[tigrang]

I hope it merges without problems.

I think we do need the constructor, so we can merge DataTableCompoennt::settings with PaginatorComponent::settings, then merge user passed settings - or copy/paste PaginatorComponent::settings default to DataTableComponent.

[tigrang]

I can't reproduce it and there's a debug statement in your commit.

I have

 public function index() {
        $this->paginate = array('limit' => 5);
}

but DataTableComponent::settings looks fine. It gets merged, not overwritten.

Edit: well not really, it doesnt even get merged because of

if (isset($this->Controller->paginate[$object->alias])) {

If it was $this->paginate['Model'] = array(....); it would merge. How can I set it up to reproduce the error?

[shama]

Sorry just coding and thinking out loud. To reproduce do:

public function index() {
    $this->paginate = array('limit' => 5);
    $results = $this->paginate();
}

The reason is $this->paginate = array() in a controller is the exact same as doing $this->Paginator->settings = array().
Also $settings gets merged into $this->settings up the Component chain, just an extra merge call if also done in the construct(), no biggie.

This is just a problem for those who want to use DataTable and normal Paginator on the same controller (CakePackages has this).

Although... maybe I was wrong and you are right about extending paginator... diving into it; maybe it's more trouble then it's worth? (Please dont kill me :)

[tigrang]

Still can't reproduce it but to accomplish what you want DataTableComponent::paginate needs to be changed to something like:

function paginte(...) {
   if (!$this->isdDataTableRequest()) {
       return parent::paginate(....);
   }
  .... rest of datatable for jquery stuff ...
}

or move have separate methods for them.

I'm done for the day though.

[tigrang]

I still can't reproduce. Running that code just outputs json for me since it sets the view class to DataTableResponseView but no errors and settings are all there. I can't see how it would get overwritten.

Calling Controller::paginate() just passes Controller::$paginate to the Component which merges only. Are you sure its not something specific to your app?

Check the comments I left in the code.

[tigrang]

So _settings and settings are identical now. See comment below

[shama]

I used a fresh clone of CakePHP 2.1. Cloned the DataTable component branch to app/Plugin/DataTable. Then in the PagesController added an index action with the code mentioned above (removed the default route to PagesController:display() as well). Then I get this error message:

Notice (8): Undefined index: columns [ROOT2plugins/DataTable/Controller/Component/DataTableComponent.php, line 158]
Warning (2): Invalid argument supplied for foreach() [ROOT2plugins/DataTable/Controller/Component/DataTableComponent.php, line 158]
Notice (8): Undefined index: scope [ROOT2plugins/DataTable/Controller/Component/DataTableComponent.php, line 115]
Warning (2): array_merge() [http://php.net/function.array-merge]: Argument #1 is not an array [ROOT2plugins/DataTable/Controller/Component/DataTableComponent.php, line 115]
Notice (8): Undefined index: viewVar [ROOT2plugins/DataTable/Controller/Component/DataTableComponent.php, line 132]

ROOT2plugins = /var/www/cake2plugins/ btw... cake is mistakenly screwing that up in error messages (on my list things to fix in cake ;)

It is because this line in the core Controller: https://github.com/cakephp/cakephp/blob/master/lib/Cake/Controller/Controller.php#L430

Doing $this->paginate = array() sets the Paginator settings property directly thus overwriting any previous settings.

Setting $this->_settings in the construct prevents the user from overwriting $this->settings. Later we merge it back in case $this->paginate = array() was set on the controller (since it overwrites $this->settings, that will have the data from $this->Controller->paginate).

I didn't know the Paginator component was this tied to Controllers until now. Otherwise I wouldn't have thought this would be a good idea. Your method on master is superior, imho.

[tigrang]

Ah! I set public $paginate = array(); in my AppController in so that's why I wasn't able to run into it, never calls __set. Well, easy fix is to either rename public $settings to public $_settings or move that array to the constructor, just so we only need to merge once.

I like extending the Paginator because it fixes the 2.1 fields issue too without any messy unsetting and then merging and it has a better _getObject() method to get the model.

I'm almost done with the helper, I'll add the fix for the component in a bit.

[tigrang]

And there's another boog. In beforeRender() when it sets the view var, $columns is null because I took the _parseSettings() out of constructor so I can pass $object from paginate() to it. So gotta fix that as well.

[shama]

Awesome man! Just let me know if/when your ready for me to write some tests. :)

[tigrang]

Actually, I'm not going to fix that.

Two simpler solutions are a) don't alias Paginator (see note below) or b) add public $paginate = array(); to your AppController. I think that's a better solution then merging everywhere.

I think I'm going to keep DataTableComponent (while still extending PaginatorComponent) only for jquery server-side. If you need normal paginating include PaginatorComponent and use each accordingly.

[tigrang]

Helper done. see temporary wiki https://github.com/tigrang/cakephp-datatable/wiki/Helper