[8.0] Support for plugin engine methods. #1462

pimlie · 2017-10-21T13:03:27Z

As discussed in #1294

I have implemented more dynamic/flexibel methods for creating DataTable engines instances, the changes are:

added support for macros in DataTables.php
added boot method in DataTablesServiceProvider to add macro's for all engine names listed in config/datatables.php. Engine names are converted to camelCase, so if you list an engine as mongodb-query the macro will be named mongodbQuery.
added canCreate and create methods in DataTableAbstract.
With the canCreate method a DataTableEngine can indicate whether they can be create with the supplied parameters (see Collection for the most interesting use-case). The create method then returns an instance of the DataTable engine. With these 2 methods DataTable Engine plugins have full control over how they should be used. See the CollectionEngine again which required a special treatment in DataTables.php (with the conversion from array to Collection) but is now fully self sufficient ;)

Due to the canCreate method the config.datatables.builders option has become redundant, see the new implementation of DataTables::make. Therefore I would like to propose to comment out the builder options in config/datatables.php by default and mark that as a 'advanced configuration' option. That way advanced users can still override the builder to engine mapping, but if eg an Engine would split support for one of its supported Builders to a different class the bulk of the users are not affected.

yajra · 2017-10-22T05:09:58Z

src/CollectionDataTable.php

+        return is_array($source) || $source instanceof Collection;
+    }
+
+    public static function create($source)


Please add the missing docs.

yajra · 2017-10-22T05:10:52Z

src/DataTableAbstract.php

@@ -137,6 +137,28 @@
    protected $config;

    /**
+     * Can the DataTable engine be created for these parameters


Please add all the missing period on docs.

yajra · 2017-10-22T05:21:58Z

PR looks great ❤️ . Need some minor changes and I think this is good to go. Will do some actual testing on a project though when I got the chance just to make sure. But I guess the test is sufficient enough to tell that this works :).

pimlie · 2017-10-22T10:23:09Z

I have added the docs. I actually didnt include them in Collection on purpose because phpdoc should inherit the docs from the Abstract class by default anyway, but I guess this is indeed better as now they list the correct param/return types :)

What are your thoughts about the query / queryBuilder inconsistency we currently have? The engine for Query\Builder::class are called just Query but the direct access method in DataTables.php is called queryBuilder. Renaming to eloquent-query would maybe be the best option? E.g I also use mongodb-query in my mongodb plugin. Then we could deprecate the QueryDataTables class and queryBuilder method and remove it in a next major release.

Also, I left in the eloquent and collection methods in DataTables.php but those methods could be removed and replaced by a engine macro. What do you prefer?

yajra · 2017-10-25T01:12:59Z

Actually, queryBuilder is a fluent query builder and not eloquent. Maybe we can add another class QueryBuilderDataTable that just extends QueryDataTable as a work around? Then we can safely remove it on the next major release?

yajra · 2017-10-25T01:14:07Z

src/QueryDataTable.php

@@ -45,6 +45,11 @@ class QueryDataTable extends DataTableAbstract
     */
    protected $limitCallback;

+    public static function canCreate($source)


Missing docs. Maybe we can use @inheritdoc? But I usually copy and paste the comments :)

yajra · 2017-10-25T01:14:29Z

src/EloquentDataTable.php

@@ -15,6 +16,11 @@ class EloquentDataTable extends QueryDataTable
     */
    protected $query;

+    public static function canCreate($source)


Missing doc.

pimlie · 2017-10-25T08:27:13Z

@yajra Sorry for missing those phpdocs in the previous commit, I have added them 😅

I have also added a commit with my proposal for deprecating the QueryDataTable class, hope you dont mind but it was just as easy as first discussing the required changes here. I have also added a test to make sure we still fully support the old datatables config using the deprecated engine. That test can be removed when we really remove the QueryDataTable class.

Btw, if you prefer a second PR for that I can rollback that commit but then I have to wait until you accept this PR due to too many files that are changed in both PR's ;)

yajra · 2017-10-26T03:13:29Z

src/config/datatables.php

@@ -39,19 +39,21 @@
     * This is where you can register your custom dataTables builder.
     */
    'engines'        => [
-        'eloquent'   => \Yajra\DataTables\EloquentDataTable::class,
-        'query'      => \Yajra\DataTables\QueryDataTable::class,


~~I think we can retain query engine to prevent breaking change. This way, users with old config won't break?~~

Also, this would also become an additional feature where we can just use DataTables::query() for a shorter syntax?

I added a Deprecated test as well to make sure we still support the old config's. Maybe I missed a use-case in that? See: https://github.com/pimlie/laravel-datatables/blob/feature-support-plugin-engine-methods/tests/Integration/DeprecatedQueryEngineTest.php

Yeah my mistake, it is not a breaking change but I think it's nice to have query() engine? Thanks!

yajra · 2017-10-26T03:20:17Z

I just realized that DataTables::query() is quite a good shortcut api and I think we can retain it. Although it seems redundant but I think it's quite a good option to have? What do you think?

-- Edit --
Maybe queryBuilder is the one that we should deprecate since query is much more align with Laravel's api like:

$query = User::query();

pimlie · 2017-10-26T07:30:09Z

I dont have a strong preference for either queryBuilder or query, for me it was more about the inconsistency. But in general shorter names are better I think so maybe it would be better to keep query indeed ;)

Hmm, so I guess that means we only have to deprecate the queryBuilder method in DataTables.php

yajra · 2017-10-26T08:00:34Z

In my opinion, Yes, we deprecate queryBuilder in favor of query.

…o feature-support-plugin-engine-methods

To be more consistent in naming, deprecate queryBuilder direct access method in favor of query

pimlie · 2017-10-26T09:25:42Z

Great, I have rolled back the Deprecation commit and added a new one. I also renamed the integration test and added a test for the (now) deprecated queryBuilder method just to be sure :)

yajra

Some minor doc and cs changes and I think this is good to go. Thanks a lot!

yajra · 2017-10-26T11:00:01Z

src/CollectionDataTable.php

+     * Factory method, create and return an instance for the DataTable engine.
+     *
+     * @param array|\Illuminate\Support\Collection $source
+     * @return \Illuminate\Support\Collection


I think the correct return doc here is CollectionDataTable|DataTableAbstract?

I believe not, the | indicates that either of those two types can be returned but in this case it always returns a CollectionDataTable (which just happens to be an instance of DataTableAbstract). I actually think you shouldnt use DataTableAbstract as a type anywhere as an abstract class can never be instantiated and its always the parent class you should list. But correct me if I am wrong..

Not really sure too. Sometimes, scrutinizer complains on things like this. Using PHPStorm, it will suggest to return both so I often go towards it. :)

Thats probably because it doesnt understand that with the parent::create we call a method of the abstract class which uses the static keyword to return an instance of the current class.

The following code will probably trick PHPStorm to get it right:

$datatable = parent::create($source); /* @var $datatable CollectionDataTable */ return $datatable;

Should I use that?

Maybe /* @var $datatable self */ works too btw

I think it is safe to use CollectionDataTable|DataTableAbstract. This way, IDE like phpstorm won't complain since it is compatible on the parent class as it returns DataTableAbstract.

Yes boss, done and done 😄

yajra · 2017-10-26T11:01:23Z

src/DataTables.php

@@ -80,26 +84,35 @@ public function getConfig()
        return app('datatables.config');
    }

+


Excess new line?

yajra · 2017-10-26T12:06:58Z

Released on 🚀 v8.3.0. ❤️ ❤️ ❤️ Thanks!

pimlie · 2017-10-26T12:26:44Z

Thank you for merging 👍

Fyi, for my mongodb plugin I have added another method for easy registration of custom engines by adding a serviceprovider to it. In the serviceprovider I merge the available engines dynamically to the datatables.php config, the performance hit of that should be minimal (compared to listing them hardcoded in the config) and makes it so much easier / cleaner

yajra · 2017-10-26T12:33:40Z

Oh yeah, that's a good idea. Will update the scout plugin too when I got the chance. It's not yet stable :)...

BTW, would you mind if I link any mongodb related issues to your plugin? Doesn't have much knowledge on mongodb as I haven't use it yet personally on any project. TIA.

pimlie · 2017-10-26T12:38:45Z

Sure no problem!

ElfSundae · 2017-10-26T14:09:46Z

Oh my...

ElfSundae · 2017-10-26T14:39:13Z

I am confused about the purpose of DataTables class for a long time.

In my opinion, DataTables singleton may be not needed if there is a static make method. Besides, datatables() helper always returns a new DataTables instance. And DataTables::$request seems unused, and I think if someone wants request, config, html, why not just call app(...) to get instance by using container's factory methods.

And, for me, if the engines are configurable, why we define eloquent(), collection(), query() methods in the DataTables? Think about this, I used DataTables::eloquent() before to build datatables for my models, now I need some extending of Yajra\DataTables\EloquentDataTable, I create a subclass CustomEloquentDataTable, then how to let DataTables::eloquent() return CustomEloquentDataTable? There is no elegant way.

ElfSundae · 2017-10-26T14:46:53Z

And I don't think depending upon Macroable is a good design.

pimlie · 2017-10-26T15:24:43Z

I already mentioned in my 2nd post that the eloquent() etc methods are redundant now and I think your case is a good reason why we could / should remove them.

But I dont think depending on Macroable is really that of a problem, can you explain why you think that? E.g. the problem you describe wouldnt even be a problem if the eloquent method was also a macro based on the configured engine key. Then you could have a 'eloquent' => 'mycustomengine' record in your config and you are good to go.

That said I agree with you that those methods arent really that necessary, there are already many other ways of instantiating datatable engines of which make is the most valueable

ElfSundae · 2017-10-26T16:15:56Z

I think the Macroable is only useful for the lazy package/module users.
Inside this package itself, should we really need to store many reduplicate Closures at runtime in ServiceProvider::boot()?

pimlie · 2017-10-26T16:22:48Z

So do you have another solution besides just removing the closures for the lazy users?

Btw, why do you say 'reduplicate'?

yajra · 2017-10-27T01:16:41Z

@ElfSundae

I am confused about the purpose of DataTables class for a long time.

It was originally inherited from the original package and was retained for familiarity of users migrating to this package.

In my opinion, DataTables singleton may be not needed if there is a static make method.

It was added for facade to work. Or I might be wrong?

DataTables::$request seems unused

It was used on older version of the package and by the buttons plugin. It was retained for backward compatibility. But I guess we can deprecate it and be removed on next major version?

ElfSundae · 2017-10-27T12:58:45Z

@pimlie I think this PR made DataTables factory more extendable, but with a complex API. If someone wants to extend an exist engine or add new, he should be careful, as the doc of builders config saying:

Note, only change this if you know what you are doing!

Before extending, the users may need to know exactly what is the meaning of this doc line. Commented builders config, register/replace macros to DataTables, canCreate(), create(), etc.
Maybe we should explain some extending architecture in the documentation?

"reduplicate" means DataTables::macro within foreach, registers the same Closures many times.

ElfSundae · 2017-10-27T13:49:58Z

@yajra
Personally, I prefer instance methods in a singleton, not static methods. Do not mind it, it is all right.

Think about current datatables() helper:

use Yajra\DataTables\DataTables;

function datatables($source = null)
{
    if ($source) {
        return DataTables::make($source);
    }

    return new DataTables;
}

If someone wants to override DataTables, he may replace 'datatables' singleton in a service provider, but datatables() helper always use the Yajra\DataTables\DataTables class. It can be changed to:

function datatables($source = null)
{
    if ($source) {
        return app('datatables')->make($source);
    }

    return app('datatables');
}

Since DataTables is a singleton, letting make() become an instance method seems more reasonable, and in the instance make() we can access instance variables or methods.
And after doing this change, we disable Yajra\DataTables\DataTables::make(), in my opinion, it is a bad way as it is hard coded the certain class.

ElfSundae · 2017-10-27T17:27:47Z

@pimlie @yajra

Does the following make sense?

Configuration

'engines' => [
    'eloquent' => Yajra\DataTables\EloquentDataTable::class,
    'query' => Yajra\DataTables\QueryDataTable::class,
    'collection' => Yajra\DataTables\CollectionDataTable::class,
],

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

I exchange key and value for builders config.

DataTables

namespace Yajra\DataTables;

class DataTables
{
    public function make($source)
    {
        foreach (config('datatables.builders') as $engine => $classes) {
            foreach ((array) $classes as $class) {
                if ($source instanceof $class) {
                    return $this->createDataTable($engine, func_get_args());
                }
            }
        }

        throw new \Exception('No available engine for ' . get_class($source));
    }

    protected function createDataTable($engine, $parameters)
    {
        $class = config('datatables.engines.'.$engine);

        if (! $class) {
            throw new \InvalidArgumentException("DataTable engine {$engine} does not exist.");
        }

        // If we want to support dependency injection for DataTable classes
        // return app()->make($class, $parameters);

        return (new \ReflectionClass($class))->newInstanceArgs($parameters);
    }

    public function __call($engine, $parameters)
    {
        return $this->createDataTable($engine, $parameters);
    }
}

If we would like to add Macroable, just handle macros in __call.

I am not sure whether we need to support variable parameters for make()...

helper.php

if (! function_exists('datatables')) {
    function datatables($source = null)
    {
        return is_null($source)
            ? app('datatables')
            : call_user_func_array([app('datatables'), 'make'], func_get_args());
    }
}

In this PR, func_get_args() is used, maybe variable parameters are useful for custom datatables, then fix datatables() helper.

Usage

datatables($query);
datatables()->eloquent($query);
\DataTables::eloquent($query);

Example of handling mixed source

class CollectionDataTable extends DataTableAbstract
{
    public function __construct($source)
    {
        $this->collection = $source instanceof Collection
            ? $source : new Collection($source);

        // ...
    }
}

Extending:

To override built-in engines or add new, just change the config file:

'engines'        => [
    'eloquent'   => App\DataTables\EloquentDataTable::class,
    'mongodb'    => App\DataTables\MongodbDataTable::class,
    // ...
],

If you have different engines for the sources extended the same base class, just take note of the order in builders config:

'builders' => [
    'mongodb' => [
        Subclass\Of\Illuminate\Database\Eloquent\Relations\Relation::class,
        Illuminate\Database\Eloquent\Builder::class,
        // ...
    ],
    'eloquent' => [
        Illuminate\Database\Eloquent\Relations\Relation::class,
        Illuminate\Database\Eloquent\Builder::class,
    ],
    // ...
],

In brief, the DataTables factory only depends on the configuration, no matter this package, user application, or other plugin packages.

yajra · 2017-10-28T02:29:32Z

@ElfSundae maybe you can submit another PR and we can discuss this there further. Your proposal makes sense too and I agree with you with regard to datatables() helper. The idea of depending the factory on config is also in sync with #1462 (comment).

ElfSundae · 2017-10-28T03:19:21Z

OK

pimlie · 2017-10-28T07:51:19Z

Although I have no issue with most of your suggestions, using ReflectionClass should imho always be avoided if possible. If you talk about Macroable as a bad design due to closures then we should avoid using ReflectionClass at all cost.

But what I wanted to do with this PR is to make the configuration easier. Why do we always need to configure a mapping between builder classes and engines when probably for most of the users the default builders provided by an engine are ok? Its only adding another point where you can introduce bugs/mistakes.

Also I am not a big fan of deliberate type switching if you can avoid it. Although php is loosely typed I think it can be confusing when the builders config can have both array as a string/class reference elements.

With the implementation in this PR you can already do what you want with regards to registering custom builders to engines. The only issue what this PR didnt address where those engine specific methods in DataTables. I think your suggestion for using __call in DataTables for that is a good solution and that it removes the dependency on Macrorable is a nice side effect . The rest is ~~just~~mostly implementation preferences I guess ;)

-- edit --
btw, using Macroable was also a deliberate decision. The name of this package starts with laravel- and Macroable is the 'go to' method in Laravel for introducing plugin like features like we need here. But I have no objection to using __call ourselves, just explaining why I used it.

pimlie · 2017-10-28T07:55:34Z

Oh, and I indeed used func_get_args() to provide a really future ready interface for all engines you can think of. Currently all engines only use one $source, but who knows what somebody thinks of as an engine and what needs it has.

So if you really want we could remove the func_get_args/call_user_func_array but it was a deliberate decision to future proof the custom engine interface 😄

pimlie · 2017-10-28T08:23:00Z

Thinking about it, actually I would even prefer to go a step further and also remove the default engine list in the configuration as well. Just put them hard coded in DataTables or the ServiceProvider and only use the configuration to override or extend default functionality.

--edit--
to explain why, I think that if a package provides certain functionality that functionality should always be available. Its the choice of the package developer to support certain features or not, not the user. If the user wants to override functionality thats ok with me, he can do that with a custom engine or custom builder mapping. But I dont think its good design that when I remove everything within the engines and builders configuration options I loose all functionality as mergeConfigFrom only merges the first config level

ElfSundae · 2017-10-28T20:12:40Z

Let's discuss in #1488

pimlie added 3 commits October 21, 2017 14:13

Apply cs fix

894088c

Rebuild engine creation methods for better plugin support

497dd62

Mark datatables.builder an advanced option to override default behaviour

afdbe6b

yajra changed the title ~~Support for plugin engine methods~~ [8.0] Support for plugin engine methods. Oct 22, 2017

yajra added the Hacktoberfest label Oct 22, 2017

yajra requested changes Oct 22, 2017

View reviewed changes

add phpdocs

34a538c

yajra requested changes Oct 25, 2017

View reviewed changes

add more missing phpdocs

032afe5

yajra reviewed Oct 26, 2017

View reviewed changes

yajra approved these changes Oct 26, 2017

View reviewed changes

yajra requested a review from ChaosPower October 26, 2017 08:00

Merge branch '8.0' of https://github.com/yajra/laravel-datatables int…

245fe47

…o feature-support-plugin-engine-methods

pimlie force-pushed the feature-support-plugin-engine-methods branch from 4d50ac2 to 9563010 Compare October 26, 2017 09:20

deprecate queryBuilder()

3352114

To be more consistent in naming, deprecate queryBuilder direct access method in favor of query

pimlie force-pushed the feature-support-plugin-engine-methods branch from 9563010 to 3352114 Compare October 26, 2017 09:24

yajra requested changes Oct 26, 2017

View reviewed changes

pimlie added 2 commits October 26, 2017 13:12

apply cs fix

6f021b8

fix phpdoc

0dabc60

yajra approved these changes Oct 26, 2017

View reviewed changes

yajra merged commit ffb11f5 into yajra:8.0 Oct 26, 2017

pimlie deleted the feature-support-plugin-engine-methods branch October 26, 2017 12:27

ElfSundae mentioned this pull request Oct 28, 2017

[8.0] Fix datatables() helper #1487

Merged

ElfSundae mentioned this pull request Oct 28, 2017

Refactor DataTables factory #1488

Closed

		@@ -80,26 +84,35 @@ public function getConfig()
		return app('datatables.config');
		}

[8.0] Support for plugin engine methods. #1462

[8.0] Support for plugin engine methods. #1462

Conversation

pimlie commented Oct 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yajra commented Oct 22, 2017

pimlie commented Oct 22, 2017 • edited Loading

yajra commented Oct 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pimlie commented Oct 25, 2017

yajra Oct 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yajra Oct 26, 2017 • edited Loading

Choose a reason for hiding this comment

yajra commented Oct 26, 2017 • edited Loading

pimlie commented Oct 26, 2017 • edited Loading

yajra commented Oct 26, 2017

pimlie commented Oct 26, 2017

yajra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pimlie Oct 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pimlie Oct 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yajra commented Oct 26, 2017

pimlie commented Oct 26, 2017

yajra commented Oct 26, 2017

pimlie commented Oct 26, 2017

ElfSundae commented Oct 26, 2017

ElfSundae commented Oct 26, 2017

ElfSundae commented Oct 26, 2017

pimlie commented Oct 26, 2017 • edited Loading

ElfSundae commented Oct 26, 2017

pimlie commented Oct 26, 2017

yajra commented Oct 27, 2017

ElfSundae commented Oct 27, 2017

ElfSundae commented Oct 27, 2017

ElfSundae commented Oct 27, 2017

yajra commented Oct 28, 2017

ElfSundae commented Oct 28, 2017

pimlie commented Oct 28, 2017 • edited Loading

pimlie commented Oct 28, 2017 • edited Loading

pimlie commented Oct 28, 2017 • edited Loading

ElfSundae commented Oct 28, 2017

pimlie commented Oct 21, 2017 •

edited

Loading

pimlie commented Oct 22, 2017 •

edited

Loading

yajra Oct 26, 2017 •

edited

Loading

yajra Oct 26, 2017 •

edited

Loading

yajra commented Oct 26, 2017 •

edited

Loading

pimlie commented Oct 26, 2017 •

edited

Loading

pimlie Oct 26, 2017 •

edited

Loading

pimlie Oct 26, 2017 •

edited

Loading

pimlie commented Oct 26, 2017 •

edited

Loading

pimlie commented Oct 28, 2017 •

edited

Loading

pimlie commented Oct 28, 2017 •

edited

Loading

pimlie commented Oct 28, 2017 •

edited

Loading