Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add tests to filter types and fix options #7007

Merged
merged 1 commit into from
Apr 9, 2021

Conversation

franmomu
Copy link
Member

@franmomu franmomu commented Apr 2, 2021

Subject

I wanted to work a bit with filters, but before starting, better to have some tests.

Adding them showed that:

  • format is not valid for DateTimeRangeType because this Type adds two DateTimeType that have that configuration (not DateTimeRangeType itself), so I've added the format option in the field_options option so is passed to those DateTimeType and changed from DateType::HTML5_FORMAT to DateTimeType::HTML5_FORMAT.
  • date_format is not valid for DateType, so I've changed it to format.

Another interesting part I was thinking was... why has it worked so far? Because having:

public function configureOptions(OptionsResolver $resolver)
{
    $resolver->setDefaults([
        'field_type' => FormDateType::class,
        'field_options' => ['date_format' => FormDateType::HTML5_FORMAT],
    ]);
}

The field_options option is an array and we always override this array (here for example), and by doing this, it does not merge the values, so this date_format always gets lost.

If we want to allow nested options we should use a closure instead (useful also for #6078), but that's another issue (that I think we should consider).

I am targeting this branch, because these changes are BC.

Changelog

### Fixed
- Fixed using invalid values in `field_options` of filters.

@franmomu franmomu added the patch label Apr 2, 2021
@VincentLanglet VincentLanglet requested a review from a team April 2, 2021 17:25
Copy link
Member

@phansys phansys left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll try to check these changes in a real project, but at first sight, they seem right.

@franmomu franmomu merged commit 0587fcc into sonata-project:3.x Apr 9, 2021
@franmomu franmomu deleted the test_filters branch April 9, 2021 07:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants