Proof of concept for Columns as a Service

[stephanvierkant]

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]

@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]

@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 ;-)

[Seb33300]

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

[mwansinck]

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]

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]

@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 https://github.com/stwe/DatatablesBundle/pull/903?email_source=notifications&email_token=AAWT2DHR7PPIKEBZVS3VG33P7XHHJA5CNFSM4H5GGGMKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD2A5PMQ#issuecomment-511825842, or mute the thread https://github.com/notifications/unsubscribe-auth/AAWT2DC7H4ZIBLHOJ6VS2DLP7XHHJANCNFSM4H5GGGMA .

[stwe]

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]

@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]

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?

[mwansinck]

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.

[stephanvierkant]

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]

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?)

[Seb33300]

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

[mwansinck]

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

[stephanvierkant]

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

[stwe]

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