bgultekin / laravel4-datatables-package

Server-side handler of DataTables Jquery Plugin for Laravel 4
267 stars 108 forks source link

Master phazei #151

Closed phazei closed 10 years ago

phazei commented 10 years ago

Added a new dataFullSupport option in the off chance that there' s some use case that new new changes fixed, shouldn't effect anyone now.

Also noticed the composer.json has 5.3 support, so changed the [] array notation to array() in a few spots.

phazei commented 10 years ago

Everything should still be backwards compatible, no breaking changes. Even the snake to camel conversion is backwards compatible.

phazei commented 10 years ago

Hmm, I have a load of notes on many of those commits, but it doesn't always show that when I put a new line.

phazei commented 10 years ago

I took care of the prefixes by checking every column and seeing if began with a table name. So I had to compile the tables from the query and from the joins.

But then I noticed the $query->getGrammar()->wrap($column); method. But I think that might screw up queries that use Expressions. But should those ones even be prefixed? Maybe it should only attempt to prefix if it's not an Expression. Because that's all laravel's query builder will do anyway. Dealing with the prefixes is kind of low level and maybe out of scope?

ktunkiewicz commented 10 years ago

Wow you made lat of work here... It will take some time for me to read and understand all these changes... I can merge that tommorow after I read all this at home (cant do this in office, have lots of other work to do now).

Do you mean Expression is what I call a "DB::raw"? Sorry, my english is not perfect and sometimes I call things differently :)

I think that table names in Expressions doesnt need should not be automatically prefixed, it's hard to do this good and not to break the Expression. User should be aware that using DB::raw means that it is literally "raw", so he should take care about prefixing itself. Do anybody thinks that I'm not right?

phazei commented 10 years ago

Yes, DB::raw is basically a shortcut for Query\Expression

ktunkiewicz commented 10 years ago

I wasn't able to take a deeper look into your changes but it all look good. Are you sure I can merge that and make a new release?

phazei commented 10 years ago

I'm pretty certain it can be merged. Should have all the backwards compatibility, and be easier to understand the code now. Will be around if any issues arise.

After it's merged I'd like to add some TableTools download csv code I have to work with the tables too.

ktunkiewicz commented 10 years ago

I was just going to drafft new release but noticed something and I'm not sure if this will not break something: the default configuration file now has "'dataFullSupport' => true,", so won't that turn on dataFullSupport for everybody who doesn't use that now? Won't that break things for people?

phazei commented 10 years ago

Good catch, I'll update that.