SonataDoctrineORMAdminBundle icon indicating copy to clipboard operation
SonataDoctrineORMAdminBundle copied to clipboard

Simplier Config Flag to make Filter use DateTimePicker

Open Hanmac opened this issue 2 years ago • 13 comments

Feature Request

If you want to use DateTimePicker for the DataGridFilter then the field_type parameter needs to be set manually to the correct type for the Filter:

    protected function configureDatagridFilters(DatagridMapper $datagridMapper)
    {
        $datagridMapper
        ->add("createTime", 'doctrine_orm_datetime_range', ['field_type' => DateTimeRangePickerType::class])
        ->add("updateTime", 'doctrine_orm_datetime_range', ['field_type' => DateTimeRangePickerType::class]);
    }

i want to make it easier like:

    protected function configureDatagridFilters(DatagridMapper $datagridMapper)
    {
        $datagridMapper
        ->add("createTime", 'doctrine_orm_datetime_range', ['picker' => true])
        ->add("updateTime", 'doctrine_orm_datetime_range', ['picker' => true]);
    }

Maybe even more easy to add a config param in the module config that sets the default picker value for all date(time) filter.

Hanmac avatar May 31 '22 14:05 Hanmac

Currently the default value is set here:

https://github.com/sonata-project/SonataDoctrineORMAdminBundle/blob/1f8791e08852365aeb2849fc10d878a121970e01/src/Filter/DateTimeRangeFilter.php#L36 This could be conditional bases on a config indeed.

The config option would be passed in the construct of https://github.com/sonata-project/SonataDoctrineORMAdminBundle/blob/1f8791e08852365aeb2849fc10d878a121970e01/src/Filter/AbstractDateFilter.php#L26

public function __construct($useDatePicker = false) {}

Do you want to provide the PR ?

VincentLanglet avatar May 31 '22 15:05 VincentLanglet

Some Question about understanding:

The real Filter from this Bundle sonata.admin.orm.filter.type.datetime_range does point to sonata.form.type.datetime_range. This is the part which i currently Alter with my PR.

but the AbstactDateFilter also points to sonata.admin.form.filter.type.datetime_range in the RenderSettings which also does point to sonata.form.type.datetime_range. Shouldn't the PR be in Admin Bundle instead/too? Or are these currently ignored?

Otherwise i don't understand why ORM Bundle is overwriting the field_type when AdminBundle is already setting it to the correct value?

Hanmac avatar May 31 '22 15:05 Hanmac

Shouldn't the PR be in Admin Bundle instead/too? Or are these currently ignored?

Otherwise i don't understand why ORM Bundle is overwriting the field_type when AdminBundle is already setting it to the correct value?

The AbstractDateFilter is in SonataDoctrineORM and not SonataAdmin https://github.com/sonata-project/SonataDoctrineORMAdminBundle/blob/4.x/src/Filter/AbstractDateFilter.php

So I don't see why it should be in the sonata admin.

There are indeed some logic in the AbstractDateFilter https://github.com/sonata-project/SonataDoctrineORMAdminBundle/blob/4.x/src/Filter/AbstractDateFilter.php#L103-L120

We might be able to update the logic here and remove all the override in all the other date filter.

VincentLanglet avatar May 31 '22 16:05 VincentLanglet

i meant it for DateTimeRangeFilter, AbstractDateFilter sets this: https://github.com/sonata-project/SonataDoctrineORMAdminBundle/blob/4.x/src/Filter/AbstractDateFilter.php#L108

which sets the field_type there: https://github.com/sonata-project/SonataAdminBundle/blob/4.x/src/Form/Type/Filter/DateTimeRangeType.php#L41

should in these classes also a "picker" parameter added?

Hanmac avatar May 31 '22 17:05 Hanmac

i meant it for DateTimeRangeFilter, AbstractDateFilter sets this: https://github.com/sonata-project/SonataDoctrineORMAdminBundle/blob/4.x/src/Filter/AbstractDateFilter.php#L108

which sets the field_type there: https://github.com/sonata-project/SonataAdminBundle/blob/4.x/src/Form/Type/Filter/DateTimeRangeType.php#L41

should in these classes also a "picker" parameter added?

Indeed then, it could be a feature for SonataAdmin.

But then we have to remove the override here: https://github.com/sonata-project/SonataDoctrineORMAdminBundle/blob/1f8791e08852365aeb2849fc10d878a121970e01/src/Filter/DateTimeRangeFilter.php#L36.

The first step might be to provide a PR here to add some unit tests in order to check the field type. Then remove the override, and see if the tests still pass.

VincentLanglet avatar May 31 '22 18:05 VincentLanglet

the problem i currently have is this: https://github.com/sonata-project/SonataAdminBundle/blob/4.x/src/Filter/Filter.php#L176

i wanted to make

public function getDefaultOptions(): array
    {
        return ['field_type' => DateTimeRangeType::class];
    }

turn into this:

public function getDefaultOptions(): array
    {
        return ['field_type' => $this->isPicker() ? DateTimeRangePickerType::class : DateTimeRangeType::class];
    }

with isPicker just be getOption('picker', false) but it doesn't work because the DefaultOptions are called before the options are set.

i will think about something, but for this i probably would need to alter admin-bundle first

or we would need something like options resolver that handles callbacks like in Form Types

Hanmac avatar Jun 01 '22 07:06 Hanmac

the problem i currently have is this: https://github.com/sonata-project/SonataAdminBundle/blob/4.x/src/Filter/Filter.php#L176

i wanted to make

public function getDefaultOptions(): array
    {
        return ['field_type' => DateTimeRangeType::class];
    }

turn into this:

public function getDefaultOptions(): array
    {
        return ['field_type' => $this->isPicker() ? DateTimeRangePickerType::class : DateTimeRangeType::class];
    }

with isPicker just be getOption('picker', false) but it doesn't work because the DefaultOptions are called before the options are set.

i will think about something, but for this i probably would need to alter admin-bundle first

or we would need something like options resolver that handles callbacks like in Form Types

I don't like the picker option.

asking to put picker => true instead of field_type => DateTimeRangePickerType::class doesn't help at all:

  • It hide the logic
  • It still requiert to pass an option

I will be more ok to a sonataAdmin config option. Then you pass this option to all those type https://github.com/sonata-project/SonataAdminBundle/blob/4.x/src/Form/Type/Filter/DateTimeRangeType.php#L41 in the constructor of those forms, and the picker or not based on the option.

IMHO, this code https://github.com/sonata-project/SonataDoctrineORMAdminBundle/blob/1f8791e08852365aeb2849fc10d878a121970e01/src/Filter/DateTimeRangeFilter.php#L34-L37 should be removed. If like you said it's already set in SonataAdmin, there is no need to set it again here.

VincentLanglet avatar Jun 01 '22 07:06 VincentLanglet

i added an MR for SonataAdminBundle to add picker as option to the Form Filter

Hanmac avatar Jun 01 '22 09:06 Hanmac

i added an MR for SonataAdminBundle to add picker as option to the Form Filter

I added a comment. I don't think you understood me because I was talking about a config option and not a form option.

And I think you'll need first a PR here to remove all the defaultoptions https://github.com/sonata-project/SonataDoctrineORMAdminBundle/blob/1f8791e08852365aeb2849fc10d878a121970e01/src/Filter/DateTimeRangeFilter.php#L34-L37 in order to not override the one provided by SonataAdmin.

But we need to check this with unit tests.

VincentLanglet avatar Jun 01 '22 09:06 VincentLanglet

When i remove the Default Options, then the problem is this part: https://github.com/sonata-project/SonataDoctrineORMAdminBundle/blob/9ccf050648625de7055a28251f04ab487afe3fea/src/Filter/AbstractDateFilter.php#L103-L120

with setting the field_type from the options. So the users can overwrite it. i'm currently strugging to make it fall back correctly without removing the option for the User.

Hanmac avatar Jun 01 '22 09:06 Hanmac

This should be easier now https://github.com/sonata-project/SonataDoctrineORMAdminBundle/pull/1669 is merged if you wanna try again @Hanmac

VincentLanglet avatar Jul 21 '22 19:07 VincentLanglet

This should be easier now #1669 is merged if you wanna try again @Hanmac

the only thing that makes it a bit tricky would be that now the default values are removed from SonataAdminBundle (or that part is getting deprecated) it wouldn't be possible for SonataAdminBundle config to set stuff in SonataOrmAdminBundle

so if you want to do it via config for all date time filters, it can only be done nicely as config for SonataOrmAdminBundle

Hanmac avatar Jul 22 '22 11:07 Hanmac

the only thing that makes it a bit tricky would be that now the default values are removed from SonataAdminBundle (or that part is getting deprecated) it wouldn't be possible for SonataAdminBundle config to set stuff in SonataOrmAdminBundle

That's normal. Things related to SonataOrmAdminBundle should be configured in the SonataOrmAdminBundle config.

Filters are persistenceBundle specific, so you may want different behavior for different persistence bundles.

VincentLanglet avatar Jul 22 '22 12:07 VincentLanglet

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

github-actions[bot] avatar Jan 18 '23 12:01 github-actions[bot]