Kreyu / data-table-bundle

Streamlines creation process of the data tables in Symfony applications. NOT PRODUCTION READY.
https://data-table-bundle.swroblewski.pl
MIT License
62 stars 12 forks source link

To-do before first stable release #95

Open Kreyu opened 2 months ago

Kreyu commented 2 months ago

Just for reference - we really need more code coverage for critical parts of the bundle.

Additionally:

If you have any problems with the bundle, please, let me know. My contributions slowed down a little this year, but I'll try my best to move forward with the releases.

CMH-Benny commented 2 months ago

Hey, I so much love this bundle - Awesome work!

I found it a while back and kept an eye on it, because I am eager to use it. Sadly it still isn't safe for production according to the docs, but if I look at this Issue it suggests that we are not so far away?

I don't plan to use it with doctrine, we are using DTOs in our project anyway, so if doctrine moves out at ome point, it shouldn't affect it.

The Array source is something that could be implemented via ProxyQuery if it's needed, but I don't see a usecase, i expect to build a lot of ProxyQuery classes myself to make it work in my project, so it also shouldn't affect me much.

The symfony form integration is something I am not sure about. This might be a blocker if that changes at some point. So the question would be, how much effort and time is required to sort this one out?

What worries me most is the theme task, because it's very sure I'll have to create my own theme and if it's subject to get changed soon, this could be a blocker for sure. This seems to be the most important task out of those 4, I guess?

What do you think, how drastically can the changes be? will it void any prior effort to integrate this bundle if those tasks are done? Is there any roadmap ahead?

Kreyu commented 1 month ago

Hey @CMH-Benny, sorry for the late reply and thank you for the kind words!

Sadly it still isn't safe for production according to the docs, but if I look at this Issue it suggests that we are not so far away?

This bundle still requires a lot of work to consider it stable, including reworking existing parts to make them testable (no coverage yet for most crucial parts of the library). It may work fine in some cases, but the amount of possible breaking changes to come may not be worth to install it yet, especially considering that your use case seems to require more than a simple table for a CRUD :) I am really happy that you're interested in the bundle and consider it useful, but I think I am not able to say confidently that in the current state it wouldn't introduce more problems that solutions. Especially considering my limited time developing it in the last months, but I hope that would change soon (side note - I am not abandoning this project or something like that, just things been rough lately).

I don't plan to use it with doctrine, we are using DTOs in our project anyway, so if doctrine moves out at ome point, it shouldn't affect it. The Array source is something that could be implemented via ProxyQuery if it's needed, but I don't see a usecase, i expect to build a lot of ProxyQuery classes myself to make it work in my project, so it also shouldn't affect me much.

Please note, that ProxyQuery classes are meant to be an "adapter" for a data source. For example, if you're retrieving data from API, you can create a ProxyQuery to integrate with it and handle pagination and sorting. Each data source also requires a different set of filters, as built-in filters are strictly bound with Doctrine, and that's by design :) Not sure where your DTO's will come from to comment it further.

The symfony form integration is something I am not sure about. This might be a blocker if that changes at some point. So the question would be, how much effort and time is required to sort this one out?

This will introduce many breaking changes due to some changes in the internal interfaces, such as DataTableInterface, in built-in HttpFoundationRequestHandler (and probably in some other places), but I think this won't affect most applications. That is, unless you're planning to override many core parts of the bundle.

What worries me most is the theme task, because it's very sure I'll have to create my own theme and if it's subject to get changed soon, this could be a blocker for sure. This seems to be the most important task out of those 4, I guess?

There will be changes in block naming, HTML attribute inheritance between blocks, and I think that's it for now. This may be a blocker for sure when using custom themes that override some parts of the built-in ones.

What do you think, how drastically can the changes be? will it void any prior effort to integrate this bundle if those tasks are done? Is there any roadmap ahead?

That is really hard to say. There will be many breaking changes, although I am always trying to write down notes on how to update existing applications (as my co-workers are already using this bundle on daily basis, but mostly on CRUD applications based on Doctrine). Currently, I cannot be confident that new releases does not introduce even most trivial bugs due to the lack of tests.

In summary, I think, in your case (from the description you provided) I would recommend not using this bundle yet. Maybe on some projects with smaller or simpler scope just to test things out, and wait for a stable version. Not sure when that will be though - I feel like this is still something barely more than proof of concept, sometimes unstable, but usable in cases where overriding internal classes is not required. Or maybe I'm exaggerating, as this is my first open source project of such scale.

Cheers!

CMH-Benny commented 1 month ago

Thank you so much for your answer, that is actually not what I wanted to hear :D

I digged into the docs and this package is already really amazing, so I didn't want to write all that myself. However, due to the close relation to doctrine I kinda wrote my own abstraction layer, that fulfils the ProxyQueryInterface, but allows me to provide paging, sorting, filters & search in custom value objects, so I can use it without a QueryBuilder. Theoretically I can use it now with whatever I want, since I only need to fulfil a ProviderInterface and where the data is coming from is up to that provider. I use it for an elastic search index right now in my prototype :D Of course I had to implement my own set of filters, that work with my abstraction query and for now it seems to work just fine.

I think such a kind of abstraction would be nice to have out of the box, so that there is room for different "bridges" not only for doctrine, but maybe also for elastic search, api calls, reading files from storage etc. If that would happen, maybe we could try to get this package into symfony-ux itself? That would be awesome, there are many people with a need for such a powerful datatable package :)

I probably will get in trouble, if there are breaking changes at some point, but hopefully, due to that abstraction I only need to adjust it on that transformation layer. Especially the filters worry me, but at least I translate them into my own set of classes, so from there I am safe. 🤞

In regards of the template/theme I need to see. If block names of the template gets changed, this might cause issues, but since I very likely need to fully rewrite the theme, maybe it won't be too hard to adjust later. Since there is no real base for a tailwind I need to kinda start from scratch and I would guess changing some block names later won't be such a big deal. Maybe I will even come up with my own blocks, didn't spend much time on that yet :D

I would like to contribute, but for now I am playing around and I am not sure if i will have the time, but I would so much love to see a ready to use datatable packge for modern symfony applications :)

Thank you so much

Kreyu commented 1 month ago

I think such a kind of abstraction would be nice to have out of the box, so that there is room for different "bridges" not only for doctrine, but maybe also for elastic search, api calls, reading files from storage etc.

Isn't the abstraction with ProxyQueryInterface enough? We're not tied to any specific data source, and Doctrine is completely optional (and will be extracted to separate bundle soon).

I digged into the docs and this package is already really amazing, so I didn't want to write all that myself. However, due to the close relation to doctrine I kinda wrote my own abstraction layer, that fulfils the ProxyQueryInterface, but allows me to provide paging, sorting, filters & search in custom value objects, so I can use it without a QueryBuilder. Theoretically I can use it now with whatever I want, since I only need to fulfil a ProviderInterface and where the data is coming from is up to that provider. I use it for an elastic search index right now in my prototype :D Of course I had to implement my own set of filters, that work with my abstraction query and for now it seems to work just fine.

Great to hear. If you encounter any problems, or just need with that, let's create a discussion thread, and I'll try my best to help you 😄

In regards of the template/theme I need to see. If block names of the template gets changed, this might cause issues, but since I very likely need to fully rewrite the theme, maybe it won't be too hard to adjust later. Since there is no real base for a tailwind I need to kinda start from scratch and I would guess changing some block names later won't be such a big deal. Maybe I will even come up with my own blocks, didn't spend much time on that yet :D

Regarding to Tailwind, it'll be easier to extend the base.html.twig theme. There will be breaking changes with theming soon. I think most of the work with refactoring has already be done in base theme, now I'm adjusting and ironing out the Bootstrap and Tabler themes. I'll try to provide a comprehensive description what's changed and why. It won't be hard and time consuming though, mostly getting rid of unnecessary blocks and simplifying their hierarchy :)

CMH-Benny commented 1 month ago

Isn't the abstraction with ProxyQueryInterface enough? We're not tied to any specific data source, and Doctrine is completely optional (and will be extracted to separate bundle soon).

As of right now, it seemed to be not enough for me, the ProxQueryInterface only enforces methods for paginate and sort, but not to add any filters, this is only implemented in your DoctrineQuery implementation.

What I did is, I created a DataTableDataProviderQueryInterface, and an additional DataTableDataproviderInterface If I pass a service implementing the latter, a Factory will turn it into an implementation of the former and the implementation of the ProviderQuery maintains a state for the pagination, sorting, filtering and search with my own value objects. Then in the getResult() of the Query it is calling my service that implements DataTableDataProviderInterface, that enforces a getData() method with an instance of the ProviderQuery as an argument, having all that search/filter/etc. state and at this point I can use all that data to ask elastic search, any other api, database or what ever I want to get the data based on all such. My own filters are also built for this query, so in their handler methods, it checks for that interface and calls addFilter() to have them available in my Query without having to manipulate a doctrine query builder directly :D

Hope I was able to explain it good enough :) But in tl;dr it felt like the ProxyQueryInterface is not sufficient by only asking for sort and paginate, while the whole filtering is kinda hardcoded for Doctrine only :)

Great to hear. If you encounter any problems, or just need with that, let's create a discussion thread, and I'll try my best to help you 😄

Good idea, if we want to continue the talk, this might make sense, don't want to pollute this issue here, I added this answer now, because it also contians some kind of feedbackm but we can move to a discussion if you want :)

Regarding to Tailwind, it'll be easier to extend the base.html.twig theme. There will be breaking changes with theming soon. I think most of the work with refactoring has already be done in base theme ...

That is good to hear and I guess I will do so, still not sure when exactly I will tacle this, but probably soon :D

Thank you again and have a nice day 🙌

bastien70 commented 3 weeks ago

hey, maybe create a demo github repository? :)

Kreyu commented 3 weeks ago

hey, maybe create a demo github repository? :)

Good idea. Similar to EasyAdmin demo application, we could base it off the official Symfony demo.

Kreyu commented 3 weeks ago

Hey @CMH-Benny, coming back to this topic to give a small hint. We can continue here, this issue is mostly for the discussion anyway :)

As of right now, it seemed to be not enough for me, the ProxQueryInterface only enforces methods for paginate and sort, but not to add any filters, this is only implemented in your DoctrineQuery implementation.

If I understand what you mean correctly, in your case, you cannot apply the filters individually, but rather collect the filtration data and apply it only once. I'm thinking about using an external API, that requires simply sending a single request at the end, with specific page, limit, sorting and filtration.

You don't need additional classes for that, just create a single ProxyQuery class for your data source, and implement a custom method (like addFilter), that will be called from your filters. For example (pardon for the "pseudo" code):

class CustomDataSourceFilterHandler implements FilterHandlerInterface
{
    public function handle(ProxyQueryInterface $query, FilterData $data, FilterInterface $filter)
    {
        $query->addFilter($data, $filter);
    }
}

class CustomDataSourceProxyQuery implements ProxyQueryInterface
{
    public function paginate(PaginationData $data) {
        $this->paginationData = $data;
    }

    public function sort(SortingData $data) {
        $this->sortingData = $data;
    }

    public function addFilter(FilterData $data, FilterInterface $filter) {
        // now im just simplifying this as much as possible but you get the idea
        // of collecting filters to apply in the getResult(), instead of doing it in the handler
        $this->filters[$filter->getName()] = $data;
    }

    public function getResult() {
        // retrieve a data with applied pagination, sorting and filtration...
    }
}

I think you described something like this, but I really don't understand what's the reason for the two additional interfaces (DataTableDataProviderQueryInterface and DataTableDataProviderInterface). You should be able to handle even the most complex cases with the built-in classes/interfaces alone.

Hope I was able to explain it good enough :) But in tl;dr it felt like the ProxyQueryInterface is not sufficient by only asking for sort and paginate, while the whole filtering is kinda hardcoded for Doctrine only :)

I don't understand where the "hardcoded" part is coming from ☚ī¸ Both the proxy query and filters are data source agnostic. The fact that filtration applies each filter individually instead of a (for example) single filter method in the proxy query in my opinion, should be an advantage, because you can either use this pattern (similar to the implementation of Doctrine ORM), or do it by applying filters directly in the proxy query class, instead of the filter handlers.

If I missed something, please let me know. I don't fully understand your case, and I would really appreciate any feedback on why you see the current implementation as something tied to Doctrine, or not flexible enough (I plan on adding a comprehensive article about how to integrate with custom data sources to the documentation, so any issues you encounter would be valuable)

CMH-Benny commented 2 weeks ago

If I understand what you mean correctly, in your case, you cannot apply the filters individually, but rather collect the filtration data and apply it only once. I'm thinking about using an external API, that requires simply sending a single request at the end, with specific page, limit, sorting and filtration.

You don't need additional classes for that, just create a single ProxyQuery class for your data source, and implement a custom method (like addFilter), that will be called from your filters. For example (pardon for the "pseudo" code):

class CustomDataSourceFilterHandler implements FilterHandlerInterface
{
    public function handle(ProxyQueryInterface $query, FilterData $data, FilterInterface $filter)
    {
        $query->addFilter($data, $filter);
    }
}

class CustomDataSourceProxyQuery implements ProxyQueryInterface
{
    public function paginate(PaginationData $data) {
        $this->paginationData = $data;
    }

    public function sort(SortingData $data) {
        $this->sortingData = $data;
    }

    public function addFilter(FilterData $data, FilterInterface $filter) {
        // now im just simplifying this as much as possible but you get the idea
        // of collecting filters to apply in the getResult(), instead of doing it in the handler
        $this->filters[$filter->getName()] = $data;
    }

    public function getResult() {
        // retrieve a data with applied pagination, sorting and filtration...
    }
}

I think you described something like this, but I really don't understand what's the reason for the two additional interfaces (DataTableDataProviderQueryInterface and DataTableDataProviderInterface). You should be able to handle even the most complex cases with the built-in classes/interfaces alone.

The DataProviderQueryInterface I was talking about is basically the ProxyQueryInterface, just extending it to also add the addFilter() and addSearchTerm() methods so I can check for an instance of DataProviderQueryInterface in my Filter Handlers, in your example above you do this blindly, so for me it just looks similar like this:

class CustomDataSourceFilterHandler implements FilterHandlerInterface
{
    public function handle(ProxyQueryInterface $query, FilterData $data, FilterInterface $filter)
    {
        if (!$query instanceof DataProviderQueryInterface) {
            throw new RuntimeException('Query must be an instance of DataProviderQueryInterface');
        }
        $query->addFilter($data, $filter); // addFilter is now 100% existing, because handle expects a different type of interface
    }
}

And because of that interface, I can resuse filters for every DataProviderQuery

And the DataTableDataProviderInterface is kinda looking like this:

interface DataTableDataProviderInterface
{
    public function getData(DataTableQueryInterface $query): DataTablePageResultSetInterface;
}

I can now implement my custom provider:

readonly class CustomProvider implements DataTableDataProviderInterface
{
    public function __construct(
        private SomeService $someService,
    ) {
    }

    public function getData(DataTableQueryInterface $query): DataTablePageResultSetInterface
    {
        $options = [];
        if ($query->getPagination()) {
            $options['pagination'] = [
                'page' => $query->getPagination()->getPage(),
                'limit' => $query->getPagination()->getPageSize(),
            ];
        }

        if ($query->getSearchTerm()) {
            $options['search'] = $query->getSearchTerm();
        }

        if ($query->getSorting()) {
            $sorting = $query->getSorting()[0];
            $options['sort'] = [$sorting->getPropertyPath() . ':' . ($sorting->getDirection()?->value ?? 'asc')];
        }

        if ($query->getFilters()) {
            foreach ($query->getFilters() as $filter) {
                // this still needs to go to somewhere more generic to not have to reimplement it all the time
                match (get_class($filter)) {
                    NumberFilter::class => $filter->getNumber() ? $options[$filter->getPropertyPath()] = $filter->getNumber() : null,
                    TextFilter::class => $filter->getText() ? $options[$filter->getPropertyPath()] = $filter->getText() : null,
                    DateFilter::class => $filter->getDate() ? $options[$filter->getPropertyPath()] = $filter->getDate()->format('Y-m-d') : null,
                    Filter::class => $options[$filter->getPropertyPath()] = $filter->getValues(),
                    default => null,
                };
            }
        }

        # shape of this query is set inside Finder
        $result = $this->someservice->doSomething($options);

        return new DataTablePageResultSet(
            $result->getData(),
            $result->getPage(),
            $result->getLimit(),
            $result->getResultCount(),
            $result->getTotalCount(),
        );
    }
}

So with my provider, I can use any service, a doctrine repo, an elatic search index, opening a file handle, call to an API or whatever and all I need to do is map it back to the proper result class and it will work. That way I don't need to write a new ProxyQueryInterface implementation for each new table, I only need to implement a DataTableDataProvider

And the best thing is, when I pass such an provider in the controller as second argument, it checks for this interface and automatically turns it into my DataProviderQuery (through a ProxyQueryFactoryInterface implementation)

I don't understand where the "hardcoded" part is coming from ☚ī¸ Both the proxy query and filters are data source agnostic. The fact that filtration applies each filter individually instead of a (for example) single filter method in the proxy query in my opinion, should be an advantage, because you can either use this pattern (similar to the implementation of Doctrine ORM), or do it by applying filters directly in the proxy query class, instead of the filter handlers.

I mean hardcoded, because it only works with a Doctrine Query Builder and this handling has no interface so it just offers additional methods is magicaly uses and if you just use doctrine directly in your controller this probably is fine, but we never use doctrine directly, we have custom repository classes and hide away the query builder of it etc. so for me it was important to get the data into a service that can do the transformation/translation between datatable and some other datasource