Proof of concept for Columns as a Service #903

stephanvierkant · 2019-07-03T14:04:13Z

Yet another *aaS-solution! May I introduce to you: (a very very early stage proof of concept) Columns as a Service! See #902

Symfony Forms can be uses as a service, so you can use Dependency Injection to use components like the Routing Component in your Forms. This PR adds this to Datatable-columns as while. Take this example from my application. I'm using Selectize as a filter and I'm injection the options from a JSON-file.

use App\Datatables\Filter\SelectizeFilter;
use Sg\DatatablesBundle\Datatable\Column\Column;
use Symfony\Component\OptionsResolver\OptionsResolver;
use Symfony\Component\Routing\RouterInterface;

class AjaxColumn extends Column
{
    /** @var \Symfony\Component\Routing\RouterInterface */
    private $router;

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

    public function configureOptions(OptionsResolver $resolver) // phpcs:ignore
    {
        parent::configureOptions($resolver);

        $resolver->setDefaults([
            'title' => 'Column name',
            'width' => '100px',
            'filter' => [
                SelectizeFilter::class,
                [
                    'url' => $this->router->generate('route_to_json'),
                    'valueField' => 'id',
                ],
            ],
        ]);

        return $this;
    }
}

Noticeable changes:

Like Symfony Forms you'll have to use the FQCN as column type, not an object (new VirttualColumn()) or a non-FQCN string (virtual_column)
Services are renamed to the FQCN instead of sg_datatables.*

I'd like to hear your thoughts about this PR, I'm not sure this is the right way to do it. (but it works on my machine ;))

stephanvierkant · 2019-07-12T12:06:52Z

@Seb33300 @mwansinck I'd like to hear your opinion about this PR. I've also changed the service names to FQCN, so we'll have to bump the version to 1.3?

mwansinck · 2019-07-12T14:29:12Z

@Seb33300 @mwansinck I'd like to hear your opinion about this PR. I've also changed the service names to FQCN, so we'll have to bump the version to 1.3?

Code seems OK, but the requirement that a Column must be a services shouldn't be necessary, it will break existing setups (mainly those based on Symfony <= 3.4). It should be possible to first check if a service exists (when written in FCQN notation) and fallback tot the current logic with a E_USER_DEPRECATED notice to warn user this will be removed in a 2.0 release.

You could also add services alias for each changed service name to keep backward compatibility. For example, see below

services:
    sg_datatables.response:
        alias: Sg\DatatablesBundle\Response\DatatableResponse
        public: true

Also, if you are going to use FCQN instead of service keys, you could also replace the service dependencies tot FCQN's, or better use autowiring instead. (will break Symfony < 3.4 setups however). But that could be done in another PR.

Also added some comments on your code. In general good idea and the implementation is cleaner than my earlier PR. We would very like to see this released as soon as possible ;-)

Datatable/AbstractDatatable.php

Datatable/Column/ColumnBuilder.php

Seb33300 · 2019-07-16T07:04:54Z

I'm sorry, I dont really have the time to look into this pull request yet

mwansinck · 2019-07-16T07:32:23Z

I'm sorry, I dont really have the time to look into this pull request yet

Anything we can do to help? We would like to see this functionality being merged.

stephanvierkant · 2019-07-16T13:53:53Z

I came across Factory::create(). Should we put the logic for creating a column in this class? I don't use the Editor so I'm not sure how that'll work.

stwe · 2019-07-16T14:18:12Z

@stephan I have an opinion on this issue. I write after work. Wait. Stephan Vierkant <notifications@github.com> schrieb am Di., 16. Juli 2019 15:53:

…

I came across Factory::create(). Should we put the logic for creating a column in this class? I don't use the Editor so I'm not sure how that'll work. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#903?email_source=notifications&email_token=AAWT2DHR7PPIKEBZVS3VG33P7XHHJA5CNFSM4H5GGGMKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD2A5PMQ#issuecomment-511825842>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAWT2DC7H4ZIBLHOJ6VS2DLP7XHHJANCNFSM4H5GGGMA> .

stwe · 2019-07-16T17:23:12Z

I'm always open to new cool ideas, but the following comments:

This software now has good download rates. So let's talk about Maintaining Open Source Software. I think it's good practice not to merge API changes into the current stable branch if these changes are not backwards compatible. It follows that you should first think about whether a 2.0 or a test branch makes sense. So a kind of sandbox. The versions should be designed this way.

MAJOR version when you make incompatible API changes,
MINOR version when you add functionality in a backwards-compatible manner, and
PATCH version when you make backwards-compatible bug fixes.

The idea should also be checked to see if the base is right. By base I mean the implementation of the columns. In my opinion, I built there complete bullshit. Keyword: "diamond problem". That needs to be redone. My (self-critical) opinion. There is too much inheritance in it. Other programmers rebuild the bundle with a new name and do not realize that they are copying crap right now.

We will not be able to fulfill every wish immediately. We can not fix every bug or fix any issues. Everything should be well considered.

@stephan: the idea is good. keep it up. But please not in version 1, which is stable. But hey ... you're the boss.

mwansinck · 2019-07-16T17:56:43Z

@stwe I agree breaking changes should be backwards compatible. However I do also think that the concept "Columns as a service" can be written in such way it is backwards compatible.

Also, the base isn't complete bullshit if you ask me, the concept of columns is very similar to that of Symfony forms. Of course there is always room for improvements, but it still works.

That this software has good download rates is good. You should however keep up changes and improvements to prevent other bundles (included forks) will eventually catch up.

stephanvierkant · 2019-07-17T07:22:12Z

I agree, this should be backwards compatible.

@mwansinck How about putting this logic in the Factory? Can we inject the columns into that class in a backwards compatible way?

…rnal

mwansinck · 2019-07-18T09:54:43Z

Datatable/Column/ColumnBuilder.php

 use Twig_Environment;
 use Exception;
+use function array_key_exists;


In my opinion, remove the use function ....; lines because they don't have any advantage

I'll remove this. Let's discuss this in #912.

mwansinck · 2019-07-18T09:55:26Z

Datatable/Column/ColumnBuilder.php

+use function array_key_exists;
+use function sprintf;
+use function trigger_error;
+use const E_USER_DEPRECATED;


Can also be removed, because it's a global constant

mwansinck · 2019-07-18T09:55:50Z

Datatable/Column/ColumnBuilder.php

+    /**
+     * Column services
+     *
+     * @var \Symfony\Component\DependencyInjection\Argument\RewindableGenerator


change to iterable

mwansinck · 2019-07-18T09:57:32Z

Datatable/Column/ColumnBuilder.php

-        $column = Factory::create($class, ColumnInterface::class);
+        if (is_object($class)) {
+            $column = Factory::create($class, ColumnInterface::class);
+            @trigger_error(sprintf('Using an object as column type is deprecated since 1.3 and will be removed in 2.0. Use a class name (FQCN) instead.'), E_USER_DEPRECATED);


Is this going to be releases as a 1.3 version or could we add this to the 1.2 release

mwansinck · 2019-07-18T09:59:23Z

Resources/config/services.yml

+    Sg\DatatablesBundle\Datatable\Column\:
+        resource: '../../Datatable/Column/*'
+
+    Sg\DatatablesBundle\Datatable\AbstractDatatable:
        abstract: true
        arguments:
            - '@security.authorization_checker'


No need to specify service arguments because they can be autowired (or is this needed because of the latest argument for the colums, if so, we could reverse order of arguments).

mwansinck · 2019-07-18T09:59:35Z

Resources/config/services.yml

        public: true
        arguments:
            - '@request_stack'

-    sg_datatables.factory:
-        class: Sg\DatatablesBundle\Datatable\DatatableFactory
+    Sg\DatatablesBundle\Datatable\DatatableFactory:
        public: true
        arguments:
            - '@security.authorization_checker'


No need to specify service arguments because they can be autowired

mwansinck · 2019-07-18T10:07:47Z

I agree, this should be backwards compatible.

@mwansinck How about putting this logic in the Factory? Can we inject the columns into that class in a backwards compatible way?

I think its possible, by adding a else-if at line 133 to check for the ColumnInterface and implementing the logic within the else-if block. Other classes created by the factory could be handled in the same way (in ClassName::class notation) as well but maybe thats out-of-scope for now.

# Conflicts: # Datatable/AbstractDatatable.php # Datatable/Column/ColumnBuilder.php

stephanvierkant · 2019-08-22T13:40:33Z

I've changed the LinkColumn (introduced in #839, ping @philippemilink) into a column as a service.

Tests are failing, haven't looked into that yet.

stephanvierkant · 2019-08-24T12:08:02Z

Any idea how to fix the tests? I have really no idea how to mock the iterable $columns argument. If I mock Column::class (and put in in an array) it doesn't work because the class name is something like Mock_Column_4f1375ff, not the real FCQN.

Or can we use Symfony\Bundle\FrameworkBundle\Test\WebTestCase; instead? That makes it easier, but it changes the unit test in a functional test. (and then we need an AppKernel?)

# Conflicts: # Datatable/AbstractDatatable.php # Datatable/Column/AbstractColumn.php # Datatable/Column/ColumnBuilder.php # Datatable/Factory.php # DependencyInjection/SgDatatablesExtension.php

Seb33300 · 2019-09-02T12:55:00Z

You should rebase your columns-as-a-service branch instead merging master into your pull request.

# Conflicts: # Datatable/AbstractDatatable.php # Datatable/Column/AbstractColumn.php # Datatable/Column/ColumnBuilder.php

mwansinck · 2021-03-19T13:49:38Z

@stephanvierkant May I suggest closing this PR ? Latest commits are almost a year old, or do you still plan on continue on this?

stephanvierkant · 2021-03-19T15:19:35Z

I'm using this fork in my own application, but maybe it's better to create a new PR instead.

stwe · 2021-03-30T19:50:02Z

@stephanvierkant: Your last comment got me coding my thoughts on Column after a long time. See 2.0 branch.

…s-as-a-service

…onse\DatatableQueryBuilder::$columnNames is deprecated

…ns-as-a-service

Very very early stage proof of concept of Columns as a Service

aef5b93

stephanvierkant requested review from stwe and Seb33300 July 3, 2019 14:39

stephanvierkant mentioned this pull request Jul 3, 2019

Column as a service? #902

Open

stephanvierkant mentioned this pull request Jul 11, 2019

Pass the container to the Factory so datatables and columns can be a service #784

Closed

mwansinck reviewed Jul 12, 2019

View reviewed changes

Datatable/AbstractDatatable.php Outdated Show resolved Hide resolved

Datatable/Column/ColumnBuilder.php Outdated Show resolved Hide resolved

iterable

7f18b5a

stephanvierkant added 3 commits July 17, 2019 09:34

wrong use statement

2d095fa

Support objects as column types to maintain backwards compatibility

85e3c4b

remove RewindableGenerator from typehinting since it's marked as inte…

3a1ffbe

…rnal

mwansinck suggested changes Jul 18, 2019

View reviewed changes

stephanvierkant added 4 commits August 22, 2019 15:28

Merge remote-tracking branch 'upstream/master' into columns-as-a-service

fd1421a

# Conflicts: # Datatable/AbstractDatatable.php # Datatable/Column/ColumnBuilder.php

columnbuilder update

b11f84f

DatatableFactory can be autowired

fb2b2bd

LinkColumn fixed

547528d

stephanvierkant added 3 commits August 26, 2019 15:51

Merge branch 'master' into columns-as-a-service

e01a06d

# Conflicts: # Datatable/AbstractDatatable.php # Datatable/Column/AbstractColumn.php # Datatable/Column/ColumnBuilder.php # Datatable/Factory.php # DependencyInjection/SgDatatablesExtension.php

php-cs-fixer

ab5220a

Merge remote-tracking branch 'upstream/master' into columns-as-a-service

7eb9547

stephanvierkant added 3 commits April 6, 2020 14:54

Merge branch 'master' into columns-as-a-service

2f3a00a

# Conflicts: # Datatable/AbstractDatatable.php # Datatable/Column/AbstractColumn.php # Datatable/Column/ColumnBuilder.php

fix

1d66575

Merge remote-tracking branch 'upstream/master' into columns-as-a-service

eba9e2d

stephanvierkant added 9 commits November 26, 2021 12:42

Merge branch 'master' of github.com:stwe/DatatablesBundle into column…

9189cab

…s-as-a-service

php 8.1 fix

44cfe0a

Merge branch 'stwe:master' into columns-as-a-service

b2450a5

Update composer.json

b9df98e

PHP Deprecated: Creation of dynamic property Sg\DatatablesBundle\Resp…

8f16674

…onse\DatatableQueryBuilder::$columnNames is deprecated

Merge remote-tracking branch 'origin/columns-as-a-service' into colum…

2077465

…ns-as-a-service

update

3835505

allow symfony 7

bba15cd

TreeBuilder as return type

661d4fe

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Proof of concept for Columns as a Service #903

Proof of concept for Columns as a Service #903

stephanvierkant commented Jul 3, 2019

stephanvierkant commented Jul 12, 2019 •

edited

mwansinck commented Jul 12, 2019

Seb33300 commented Jul 16, 2019

mwansinck commented Jul 16, 2019

stephanvierkant commented Jul 16, 2019

stwe commented Jul 16, 2019 via email

stwe commented Jul 16, 2019

mwansinck commented Jul 16, 2019

stephanvierkant commented Jul 17, 2019

mwansinck Jul 18, 2019

stephanvierkant Aug 22, 2019

mwansinck Jul 18, 2019

mwansinck Jul 18, 2019

mwansinck Jul 18, 2019

mwansinck Jul 18, 2019

mwansinck Jul 18, 2019

mwansinck commented Jul 18, 2019

stephanvierkant commented Aug 22, 2019 •

edited

stephanvierkant commented Aug 24, 2019 •

edited

Seb33300 commented Sep 2, 2019 •

edited

mwansinck commented Mar 19, 2021

stephanvierkant commented Mar 19, 2021

stwe commented Mar 30, 2021

Proof of concept for Columns as a Service #903

Are you sure you want to change the base?

Proof of concept for Columns as a Service #903

Conversation

stephanvierkant commented Jul 3, 2019

stephanvierkant commented Jul 12, 2019 • edited

mwansinck commented Jul 12, 2019

Seb33300 commented Jul 16, 2019

mwansinck commented Jul 16, 2019

stephanvierkant commented Jul 16, 2019

stwe commented Jul 16, 2019 via email

stwe commented Jul 16, 2019

mwansinck commented Jul 16, 2019

stephanvierkant commented Jul 17, 2019

mwansinck Jul 18, 2019

Choose a reason for hiding this comment

stephanvierkant Aug 22, 2019

Choose a reason for hiding this comment

mwansinck Jul 18, 2019

Choose a reason for hiding this comment

mwansinck Jul 18, 2019

Choose a reason for hiding this comment

mwansinck Jul 18, 2019

Choose a reason for hiding this comment

mwansinck Jul 18, 2019

Choose a reason for hiding this comment

mwansinck Jul 18, 2019

Choose a reason for hiding this comment

mwansinck commented Jul 18, 2019

stephanvierkant commented Aug 22, 2019 • edited

stephanvierkant commented Aug 24, 2019 • edited

Seb33300 commented Sep 2, 2019 • edited

mwansinck commented Mar 19, 2021

stephanvierkant commented Mar 19, 2021

stwe commented Mar 30, 2021

stephanvierkant commented Jul 12, 2019 •

edited

stephanvierkant commented Aug 22, 2019 •

edited

stephanvierkant commented Aug 24, 2019 •

edited

Seb33300 commented Sep 2, 2019 •

edited