Refactor DataTables factory

[ElfSundae]

In this PR, I would like to refactor a clean API for the DataTables factory.

ref: #1462 (comment)

• [BC] Exchange key and value for the builders config.
  Builders should be mapped from engines to types because the engines are unique. This change will allow configuring different engines for the same type.

• Keys in the builders config can be an engine alias which defined in the engines config, or an engine class name.
  Engines in the builders config can be a class name now.

  'builders' => [
      Illuminate\Database\Eloquent\Builder::class => 'eloquent',
      App\BillData => App\DataTables\BillDataTable::class,
  ],

• Add support to specify any types in the builders config.

  'builders' => [
      'image' => [
          'resource',
          'array',
          Illuminate\Support\Collection::class,
      ],
  ],

• [BC] Change static methods of() and make() in DataTables to instance methods. Remove Yajra\DataTables\DataTables::of and Yajra\DataTables\DataTables::make usage.
  Since the DataTables has been registered as a singleton in the container, using the ugly (original DataTables) class directly is a bad design in this situation. Think about if the user or an extended package override make() method, the user must replace all DataTables::make to CustomDataTables::make.
  If someone prefers to access DataTables methods using static way, the facade is the best choice.

  \DataTables::make(...)
  \Yajra\DataTables\Facades\DataTables::instanceMethod()

• Add support to make DataTable instances using configured engines alias as DataTables methods.

  'engines' => [
      'eloquent' => ...,
      'foo_bar' => ...,
  ],
  
  \DataTables::eloquent($data);
  datatables()->fooBar($data);

• Remove unnecessary canCreate, create interfaces for DataTable engines.
  To implement a new DataTable, just start with __construct.

• Remove unnecessary Macroable trait for the DataTables.
  For now, the DataTables is just a factory to create DataTable instances, we do not want user to add macros to make it messy. If you want to register a custom engine, just add alias to the engines configuration.

• Remove support of variable length parameters for make().
  See [8.0] Fix datatables() helper #1487 (comment)

• [TODO] Remove needless backward compatibility. @yajra

  $request
  $html
  
  getRequest()
  getConfig()
  queryBuilder()
  getHtmlBuilder()

Extending Notes

• The DataTables factory depends upon the configuration of engines and builders.
• engines config just acts as class aliases, like app()->bind('eloquent', 'Yajra\DataTables\EloquentDataTable'). Adding an engine alias is only needed if you want to use this alias as a method of DataTables to resolve the DataTable instances, like datatables()->eloquent(...).
• make() automatically decides an engine class by comparing the type of given data with types defined in the builders config.
  There may be different engines for the same type, in this case we can not autoselect in anyway, so let the user (application) decide. make() will use the first match.
  If you are developing a public package that contains some DataTable engines, it is not recommended to change/set/merge user's datatables.builders configuration. Just document what you provide, and let user be free to use them.

[pimlie]

Here are my 2cts.

• Fixing the use of the singleton class in DataTables looks like a nice fix!

• Removing dependency on Macroable and using __call in DataTables ourselves is fine with me. I used Macroable as its the go to method for plugin features in Laravel but I have no issue with removing that.

• Relying on is_xxx functions makes custom engines less flexible. Eg what if I want to switch engine based on a class property? Then I need to add two separate / custom is_propery_a and is_property_b helper functions. Imho using canCreate is much cleaner as the DataTable engine has full control and all the logic is in one place.

• Because you removed the factory method create you now cant specify the correct source type in the constructor of Collection. Imho that is bad design, the constructor for the Collection engine should only accept variable types it really can works with. Now I can instantiate a CollectionEngine as follows

$someClass = new EloquentDataTable(); // could be anything, just an example
$engine = new CollectionDataTable($someClass);

Imho above code should give an error like First parameter of CollectionEngine should be an instance of Collection, but EloquentDataTable given and not just fail on the new Collection($source) line as the EloquentDataTable source cannot be converted to a Collection.

• Using 2nd level config parameters is ill-advised, mergeConfigFrom doesnt do a 'deep merge' but only merges the 1st level of config variables. So by adding the array option to engines/builder you now need to make sure to merge default values for them as well in setupAssets.

• One of the key features I tried to do in [8.0] Support for plugin engine methods. #1462 was to make configuration easier. To me this PR does exactly the opposite by still relying on the builder mapping for default functionality and even introducing a second level of parameters. When you create a package that package should always provide the functionality as (the developer) intended. It is nice if you are able to extend (and in some degree override) that functionality, but the basics should always work. That shouldnt only count for this package but also for any custom engines. This will prevents issues like 'This package s*cks because it doesnt work' just because somebody completely f*cked up their config causing everything you have written carefully down in your documentation not to apply anymore.

• Because you removed the factory methods canCreate and create I introduced, you now indeed have the problem you describe in [8.0] Fix datatables() helper #1487. Your solution to therefore just remove variable parameters exactly proves my third point: this PR makes things less flexible

That said, from a technical point of view I understand that the major difference between my PR and this PR is possibly the memory profile. Probably this PR uses a bit less memory, as using static factory methods on the DataTable engine class itself needs to load all DataTable engine classes into memory until it finds a matching one.
But I think there is a good reason to accept that memory usage, because (as explained above) the trade in is that we otherwise are less flexibel.
And very importantly, it will only use more memory if and only if you use the make method. If you really care about memory usage you shouldnt use generic methods like DataTables::make anyway but write your code in a style so that you already know which DataTable engine is going to be used.

[ElfSundae]

@pimlie
Thanks very much for your kind points.

I think DataTables::make() is just a factory which provides a convenient way to create DataTable instances, and without using it, all DataTable engines can and should work as normal too. There is no relationship between DataTables and engines. We should not force users to only use DataTables factory methods to create engines.

If the user pass unproper source to an engine, in my opinion, failure as error is just OK. I think you wont like the following code, right?

$query = User::query();

if (QueryDataTable::canCreate($query)) {
    $dataTable = QueryDataTable::create($query);
} else if (EloquentDataTable::canCreate($query)) {
    $dataTable = EloquentDataTable::create($query);
} else {
    abort(500);
}

return $dataTable->toJson();

If an engine needs exact type of data to create, type hinting in the constructor will be fine, like QueryDataTable does.

And for another example, let's see the built in CollectionDataTable:

public function __construct(Collection $collection)
{
    $this->collection = $collection;
}

Actually it can handle any type which could be converted to a Collection instance, such as array, Arrayable, Jsonable. For convenience, we can remove the type hinting:

public function __construct($source)
{
    $this->collection = $source instanceof Collection
        ? $source : new Collection($source);
}

Now user can create a CollectionDataTable instance from array, Arrayable or Jsonable directly.
And imagine this, the Collection may add support for other types in the future, such as Traversable, JsonSerializable, our CollectionDataTable can still work well without any upgrade, to the author of CollectionDataTable or to the user. FYI here and here.

Relying on is_xxx functions makes custom engines less flexible. Eg what if I want to switch engine based on a class property?

I think PHP built-in is_xxx functions are sufficient for a DataTable engine. Does anyone want to implement such engine:

class AwesomeDataTable implements DataTable
{
    protected $source;

    public function __construct($source)
    {
        $this->source = $source;
    }

    public function count()
    {
        if (is_array($source)) {
            return count($source);
        } else if (is_string($source)) {
            return strlen($source);
        } else if ($source instanceof Collection) {
            return $source->count();
        } else if ($source instanceof Builder) {
            return $this->countForBuilder();
        } else if ($source instanceof DbQuery) {
            return $this->countForDbQuery();
        } else if ...
    }
}

BTW, if we want to use create method to make DataTable engines, we may should add canCreate and create interfaces to the DataTable contract.

---

For now, I can not think of any case that need variable parameters to create a DataTable engine. If there is, autoselect of make() may not work correctly, even with canCreate and create support.

For example:

class CollectionDataTable extends \Yajra\DataTables\CollectionDataTable
{
}

class FooDataTable implements DataTable
{
    public static function canCreate($data, $options)
    {
        return $data instanceof Collection && is_array($options);
    }
}

class BarDataTable extends FooDataTable
{
}

Since FooDataTable or BarDataTable needs two parameters to create, and they have the same signature, we can not specify them in the builders config.

// Configuration

'engines' => [
    'collection' => 'App\DataTables\CollectionDataTable',
    'foo' => 'App\DataTables\FooDataTable',
    'bar' => 'App\DataTables\BarDataTable',
],

'builders' => [
    'Illuminate\Support\Collection' => 'collection'
],

// DataTables::make

$args = func_get_args();
foreach ($builders as $class => $engine) {
    if ($source instanceof $class) {
        return call_user_func_array([$engines[$engine], 'create'], $args);
    }
}

foreach ($engines as $engine => $class) {
    if (call_user_func_array([$engines[$engine], 'canCreate'], $args)) {
        return call_user_func_array([$engines[$engine], 'create'], $args);
    }
}

For DataTables::make($collection, $array) :

• It will always create a CollectionDataTable instance because if ($source instanceof $class) only checks the first parameter.
• After removing collection in builders, it will always create a CollectionDataTable instance.
• After removing collection in engines and builders, it will always create a FooDataTable instance.

---

Factory depended upon configuration is a common design. Think about Laravel Auth Guards, Cache, Database, Filesystems, Queue, etc. Users are free to config/override/remove any of built-in engines. So mergeConfigFrom is not an issue, we don't need to merge default values for them.

Everything is limited. I'd like to make it simple, clean, unify, easy to use, in a reasonable range.

[ElfSundae]

should give an error like First parameter of CollectionEngine should be an instance of Collection

public function __construct($source)
{
    if (! ($source instanceof Collection || is_array($source))) {
        throw new InvalidArgumentException(...);
    }

    // ...
}

[pimlie]

I think most of our differences are just because we have a different preference on how to setup our code, I really dont think the constructor of a class should be the place that automagically converts sources.
Again, this is just a personal preference. You could argue that using non type hinted / mixed arguments on a function in php is as close as you can get to function overloading like you can do in strictly typed languages. But the major difference is that with function overloading you still exactly define which types you accept and with accepting a non type hinted argument in php you accept everything and have to check afterwards.

With regards to your comment about the FooDataTable and BarDataTable, this is actually the reason why I proposed in my last comment in #1462 to remove the engines builder configuration as well (and only use it to override default behaviour) 😉 Have a look at the boot method in the service provider I added to the mongodb engine as I exactly have the problem you describe with the Eloquent engine there: https://github.com/pimlie/laravel-datatables-mongodb/blob/master/src/MongodbDataTablesServiceProvider.php

This would imho be the preferred way of registering custom engines. Also read my comments in the readme about the order of adding the mongodb dataservice provider. This way a custom DataTable engine also has control over the order of datatable engines and can even insert itself between two existing datatables.

I am sorry but I just really dont see the advantages of this PR except for the changes to the DataTables singleton. Not sure why you are so set to reverse my changes but my PR is as flexibel as it can get. This PR is just a different way of doing things but I disagree with you that this PR is simpler and/or cleaner.

As I am sure that you and me will probably never get to agree about this, I guess its up to @yajra to decide what to do 😄

[ElfSundae]

Yeah, different stands.

[ChaosPower]

@ElfSundae and @pimlie , thanks for the very informative exchange of comments.

Maybe you could combine this together with #1462 I prefer using both of them.

[ElfSundae]

@ChaosPower I don't think combining is a good idea.
Think about it for the sake of users:

• How can I use the DataTable engines or their factory?
• What things should I know before extending built-in engines or adding a new one? Are they easy to understand?
• If I want to publish a package providing my extended built-in engines or new engines, what other things should I care except implementing the DataTable contract? And which kinds of updating should I follow from yajra/laravel-datatables?

[ChaosPower]

@ElfSundae I like both your PRs and can see combining them benefits more audience.

• How can I use the DataTable engines or their factory?
  I want to be able to use both DataTable engines and custom factories

• What things should I know before extending built-in engines or adding a new one? Are they easy to understand?
  Should know OOP and they should be easy to understand. Of course well documented. Provided with samples.

• If I want to publish a package providing my extended built-in engines or new engines, what other things should I care except implementing the DataTable contract? And which kinds of updating should I follow from yajra/laravel-datatables?

If I want to publish for Laravel 5.5+ the 3rd party package or plugin should be auto-discovered. I care about performance but of course sometimes when you are in a hurry, a quick way to implement and check things should be available. I do not quite get what you meant by which kinds of updating should be followed. I will leave that for @yajra to answer.

I like how the configs are now merged.

    /*
     * DataTables accepted builder to engine mapping.
     * This is where you can override which engine a builder should use.
     */
    'builders'       => [
        'eloquent'   => [
            Illuminate\Database\Eloquent\Builder::class,
            Illuminate\Database\Eloquent\Relations\Relation::class,
        ],
        'query'      => Illuminate\Database\Query\Builder::class,
        'collection' => [
            'array',
            Illuminate\Support\Collection::class,
        ],
    ],

[pimlie]

@ChaosPower As mentioned before, please note that if a user at first decides to empty the collection builderrs config like this collection' => [] (eg as they think they will not use it and removing codes makes everything run faster /s) he/she will not be able to use the CollectionEngine at all anymore. Unless he/she adds it back again or we add a deep merge in the service provider boot.
That is a breaking change compared to how the config works currently, but with what benefits? Its mostly esthetics and actually it provides the possibility to add the same builder class to two different engines, thus with unexpected results.

I agree with @ElfSundae that combining code (except for the changes to the singleton and macroable) is probably not really useful as we do mostly the same things just in a slightly different way. My PR is really DataTableEngine centric and this PR is more generally centered around DataTables config. Both have advantages and disadvantages and both have even more overlap with the other then we discussed until know.
Actually I was trying to write a list of major differences here but while making that list every time I came to the conclusion that 'yes but then for the other PR you could do this and the result would be the same'...

So eh, I guess its really up to @yajra to decide who is going to get the final 🏆

[ElfSundae]

@pimlie Actually I don't understand clearly some of your thought such as "flexible", "full control using canCreate", and "make configuration easier", even I tried to accept for many times. @yajra merged your PR before, maybe he will be more able to agree with your point of view. So could you please submit a PR with patches such as "the changes to the singleton and macroable" ? Thanks.

And I have several issues with your idea, these may be resolved to improve your PR:

• If the factory uses canCreate and create to make engines instances, should we add these interfaces to the Yajra\DataTables\Contracts\DataTable ?
  If we need to add them for some reasons like you said, "the DataTable engine has full control", "the constructor of the engine should only accept variable types it really can works with", do users need to update their code from new EloquentDataTable($query) to EloquentDataTable::canCreate($query) ? EloquentDataTable::create($query) : what_will_be_here() ?
  If we don't add these interfaces, custom engines may never be created by the factory, as they may implement the contract directly (not a subclass of DataTableAbstract), or they did not override the canCreate (the default value in DataTableAbstract is false).

  I found your PR when I updated composer and my custom engine could not be resolved just because the default canCreate is false.

• If we want to support variable parameters for the engines, should we fix the default implementation of create and the method signature for canCreate create in the DataTableAbstract :

  public static function create($source)
  {
      return new static($source);
  }

  You may try to implement a new engine which accepts variable parameters.
  And please take note of if ($source instanceof $class), it only checks the first argument.

• If we use Macroable for the factory, do we need to bootstrap a service provider to repeatedly register macros like you did in your PR and your mongodb package:

  foreach (static::$engines as $engine => $class) {
      $engine = camel_case($engine);
  
      if (!DataTables::hasMacro($engine)) {
          DataTables::macro($engine, function () use ($class) {
              if (!call_user_func_array(array($class, 'canCreate'), func_get_args())) {
                  throw new \InvalidArgumentException();
              }
              return call_user_func_array(array($class, 'create'), func_get_args());
          });
      }
  }

• Imho above code should give an error like "First parameter of CollectionEngine should be ...

  How to achieve this exception? The factory does not know what checks in canCreate. If we throw it in canCreate or create, I can not see any advantage over throwing it in __construct.

• About the note for the order of engines config, I read the readme of your mongodb package. It seems the service provider is optional and the only purpose of this provider is to help user to add engines (with the author preferred names) which your package provided to the first order, on every request, and it should be registered after Yajra's provider which means users using Laravel 5.5+ need to disable package discovery for yajra/laravel-datatables-oracle. What if another package provides the same provider?
  To me, after reading the readme and exploring the code of the service provider, finally I found that all I need to do is just adding engines which the package provided to my datatables config file, e.g. 'any-name-i-love' => 'Pimlie\DataTables\MongodbDataTable::class',.
  Or I misunderstood something?

[pimlie]

Quite some remarks, see below

• About create methods:

  • Yes, you are right. They should probably be added there as well. Good catch!
  • In this case the factory method is supplemental just for the make method. If you know you need the EloqouentDataTable just instantiate that class directly. Just as before, nothing has changed about that.

• Although custom engines can use multiple parameters, default behaviour is just one. We should probably add a comment in the doc of the DataTables contract about this. Dont think it is wrong that the default behaviour doesnt reflect all the possibilities you can do when you build a custom engine?

• About macroable

  • I already mentioned a couple of times I am ok to use your solution for this
  • What do you mean with 'repeatedly'? If DataTables is a singleton the if hasMacro prevents registering the same macro twice.

• Maybe we have a misunderstanding about the factory methods. But as said above, the create methods were only meant to be used by the magic make method. Also maybe I just don't understand your use-case? How many custom engines do you have for a standard laravel Eloquent model? And what are the differences between them and how/when do you use one or the other?

• About your last point, sorry but I feel like now you are just starting nit-picking a bit and I have to explain the obvious. Yes, if a laravel package depends on another laravel package and one of them uses auto-discovery it seems you cant be assured that the providers will be loaded in the correct order. This is not specific to our use case.
  I also said in the readme that the service provider or config only needs to be added/changed if you want to use the magic make or of method. If you dont use either of those, you dont have to add the service provider. Please remember that a convenience / magic method like make will always come at a price.

Hopefully to finally end this discussion, all the partially pseudo code below are (still) valid ways of getting a datatable:

class eloquentUserModel extends Eloquent\Model {
  use EloquentDataTableTrait;
}

$dataTables = new EloquentDataTable(eloquentUserModel::all());
$dataTables = eloquentUserModel::dataTables();
$dataTables = DataTables::eloquent(eloquentUserModel::all())
$dataTables = DataTables::make(eloquentUserModel::all())
$dataTables = datatables(eloquentUserModel::all())

• If you use the first 2 methods, the create methods and/or datatables configuration are not used. If you care about performance etc you write your code in a way that you use one of these methods!
• If you use the 3rd method, the datatables config should have an engines array to map the called function name to an engine class. As you can't type hint these dynamic functions we could use the create methods here, but we could also just try to instantiate the class. I have a slight preference for the latter as when the user specifically calls eloquent we should be assured that the user will pass a eloquent model. But I have no problem with using a create here. I dont really see the need for these methods actually, maybe we should remove them in a next major? For now we should support them in the best way
• If you use the 4th or 5th method the create methods will be used. You need at least the engines config but if you also provide builders you can override default behaviour of the engines. This is a convenience method which has draw backs. Its nice to have but if you write your code well you'd actually make sure you will never depend on this method.

In my opinion the following behaivour is also correct:

$array = array_to_be_used_in_datatable();

// Throws error as array !== Collection
$dataTable = new CollectionDataTable($array);

// Throws error as array !== Collection or not? Up for discussion
$dataTable = DataTables::collection($array);

// Works as the create method converted the array to Collection. What a _convenience_! ;)
$dataTable = DataTables::make($array);

// Works, duh
$collection = new Collection($array);
$dataTable = new CollectionDataTable($collection);
$dataTable = DataTables::collection($collection);

[ElfSundae]

I think most of our differences are just because we have a different preference on how to setup our code

You are right, different personal preferences on coding.

My preference is going to decouple the DataTables factory from others, by removing unnecessary canCreate create methods which being called only from the factory, and removing other currently unneeded legacies like getHtmlBuilder() getRequest() which the html or buttons plugin (depended on v8.* of this package) does not use them any more.

Please forgive my bad eyes, I can not see any helpful or strong reasons for adding canCreate or create to the DataTable engines, except throwing an exception of "No available engine for xxx" instead of failure on error when improper data source being given to the sole DataTables::make() method.
Besides, in my opinion using them will bring confusion somewhere, such as I need to pay attention to data type in three places: canCreate, create, and __construct; or new CollectionDataTable($array) failed while datatables($array) working fine.

To me, less is more.

BTW, I was not nit-picking in my last point. Using the factory to create DataTable engines is the recommended way in the readme of this package and the datatables:make command, then most of users will use the factory way. My point was just the truth that I found out: the service provider which ships with one-time & one-situation task is unnecessary to bootstrap on every request, even without thinking of another same provider.

Let's end this kind of discussion. Please submit your patched PR, let @yajra decide 👻

[pimlie]

Thanks for your insights, this really is/was an interesting discussion! Maybe you look more to it from a developers point of view, someone who is actively working on custom engines? And my view is more from a users point of view, where the engines have already been created and its just about using them?

I fully agree with you that using the service provider to register the engine is a bit more costly then including it in the datables config, but again convenience comes at a price. The reason I thought you were nitpicking was mostly because of what you said about the order of loading the service providers. In the mean time I looked into this, and it seems this will not be a problem due to PR 19646 in @laravel /framework.
--edit--
my bad, this statement is false. First I was thinking about laravel-datables comes before laravel-datatables-mongodb but this package is named laravel-databales-oracle. And the order is also defined by the namespace it seems. So probably it would be best if we fix this in the boot method of DataTables, so that engines could have already been loaded.
--/edit--

Although I am happy to submit a PR, I will wait until @yajra speaks his mind about this 😄

One final note, could you maybe share your config that failed due to the missing canCreate method? Maybe this is just a bug? Because my impression was that my PR would be fully backwards compatible but if you got an error due to the default canCreate returning false I guess its not?

[ElfSundae]

Yeah, as I said, different stands. But our users are also developers. 😄
To general users which just use the built-in engines, it's all okay to use out of the box before/after your/my PR.

FYI: The order of package discovery may be from vendor/composer/installed.json, see PackageManifest::build.

Sorry I was messed, I wanted to say create, not canCreate. I have a custom engine that just implements the DataTable contract, it has no create method.

[ElfSundae]

Merged 8.0 branch.

@yajra StyleCI analysis seems strange, see https://styleci.io/analyses/qv5b5V and https://styleci.io/analyses/zRb21j
I need to fix it to :

'builders' => [
    'eloquent' => [
        Illuminate\Database\Eloquent\Builder::class,
        Illuminate\Database\Eloquent\Relations\Relation::class,
    ],
    'query'      => Illuminate\Database\Query\Builder::class,
    'collection' => [
        'array',
        Illuminate\Support\Collection::class,
    ],
],

Is there something wrong with our StyleCI config?

[yajra]

Going to hold-off this atm since there are some breaking changes. Please target to master branch since this may need a major bump? Thanks!

[ElfSundae]

Updated base branch to master.

And I am thinking restore the builders format...

[ElfSundae]

Updated:

• Reverted builders configuration to the original format, no BC now.
• Improved engine auto choosing

[ElfSundae]

Updated PR description.

[ElfSundae]

It does not make sense to support number/bool/string for a DataTable engine, so I removed is_xxx() condition.

Now the DataTables::make can automatically choose the optimum engine for the data source, there is no need to care about the engines order in the builders configuration.
@pimlie You may try it for your Mongodb engines.

[yajra]

Outdated and too many conflicts, may not be needed anymore on latest version. Thanks!