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.

Sign up for GitHub

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

Jump to bottom

Prefix every filter with sonata_ to avoid conflict #512

Merged
merged 4 commits into from
Apr 9, 2022
Merged

Prefix every filter with sonata_ to avoid conflict #512

merged 4 commits into from
Apr 9, 2022

Conversation

VincentLanglet
Copy link
Member

Subject

I am targeting this branch, because minor.

This might be annoying to have a sonata_ prefix, but I don't see an other way to be sure to avoid a conflict with either twigIntl or another library.

Closes #299.

Changelog

### Added
-`sonata_format_date` filter
-`sonata_format_time` filter
-`sonata_format_datetime` filter
-`sonata_number_format_*` filters
-`sonata_country` filter
-`sonata_locale` filter
-`sonata_language` filter

### Deprecated
-`format_date` filter
-`format_time` filter
-`format_datetime` filter
-`number_format_*` filters
-`country` filter
-`locale` filter
-`language` filter

@VincentLanglet VincentLanglet requested a review from a team April 4, 2022 11:27
@VincentLanglet VincentLanglet mentioned this pull request Apr 4, 2022
Closed
@jordisala1991
Copy link
Member

It would be nice to prefix our functions, but also IMO we should deprecate the old methods. To do so, maybe it is easier if you introduce twigRuntime for the new functions, so you dont rely on same implementation, and can add a deprecation message.

jordisala1991
jordisala1991 reviewed Apr 6, 2022
View reviewed changes
src/Twig/Extension/DateTimeExtension.php Outdated
new TwigFilter('format_time', [$this, 'formatTime'], ['is_safe' => ['html']]), // NEXT_MAJOR: Remove this line
new TwigFilter('sonata_format_time', [$this, 'formatTime'], ['is_safe' => ['html']]),
new TwigFilter('format_datetime', [$this, 'formatDatetime'], ['is_safe' => ['html']]), // NEXT_MAJOR: Remove this line
new TwigFilter('sonata_format_datetime', [$this, 'formatDatetime'], ['is_safe' => ['html']]),
Copy link
Member

Choose a reason for hiding this comment

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

Those should use the new runtime directly

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

franmomu
franmomu reviewed Apr 6, 2022
View reviewed changes
src/Twig/Extension/DateTimeExtension.php Outdated
Comment on lines 75 to 81
@trigger_error(sprintf(
'The %s method is deprecated since 2.x and will be removed on 3.0. '.
'Use %s::%s instead.',
__METHOD__,
DateTimeRuntime::class,
__METHOD__,
), \E_USER_DEPRECATED);
Copy link
Member

Choose a reason for hiding this comment

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

IMO these messages should encourage the user to use the new sonata_* filters instead of runtime class.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@franmomu
Copy link
Member

franmomu commented Apr 6, 2022

To avoid conflict for users installing both twig/intl-extra and this bundle we can maybe to add a config parameter to not load the current filters.

@VincentLanglet
Copy link
Member Author

To avoid conflict for users installing both twig/intl-extra and this bundle we can maybe to add a config parameter to not load the current filters.

SonataAdmin is using the format_datetime template of SonataIntl when this bundle is used ; which used the sonata_format_datetime twig method. Also people might want to use sometimes one and sometimes another method.

@franmomu
Copy link
Member

franmomu commented Apr 8, 2022

To avoid conflict for users installing both twig/intl-extra and this bundle we can maybe to add a config parameter to not load the current filters.

SonataAdmin is using the format_datetime template of SonataIntl when this bundle is used ; which used the sonata_format_datetime twig method. Also people might want to use sometimes one and sometimes another method.

Not sure if I understand, but what I meant is that this PR provides alternative sonata_* filters which is fine, but if someone who has twig/intl-extra and this bundle installed wants to use let's say format_date from twig/intl-extra they cannot if this bundle registers filters after twig/intl-extra. Like sonata-project/SonataNewsBundle#591

@VincentLanglet
Copy link
Member Author

Not sure if I understand, but what I meant is that this PR provides alternative sonata_* filters which is fine, but if someone who has twig/intl-extra and this bundle installed wants to use let's say format_date from twig/intl-extra they cannot if this bundle registers filters after twig/intl-extra. Like sonata-project/SonataNewsBundle#591

Currently, if you use both sonata-intl and twig-intl, there is an issue even if you are using none of these methods : If the twig intl is declared after the sonata intl one, the datetime field from sonataAdmin is crashing.
By adding the prefix, I fix the display of the datetime field for SonataAdmin users.

If the twig intl is declared after the sonata intl one, with this PR it will be possible for the user to have no issue

  • format_date will be the twig one
  • sonata_format_date won't conflict.

If the sonata intl is declared after the twig intl one, indeed there will still be an issue so far. It will be fixed in next major.

@jordisala1991
Copy link
Member

What bundles of Sonata make use of this functions? (Only the non deprecated / abandoned)

@VincentLanglet
Copy link
Member Author

VincentLanglet commented Apr 9, 2022 •
edited
Loading

What bundles of Sonata make use of this functions? (Only the non deprecated / abandoned)

Only deprecated/abandoned one are using them.

Can we merge this PR, at least as a first step ?

@jordisala1991
Copy link
Member

if none of the Sonata bundles are using those features. We could even study if this bundle is really needed or we can deprecate it.

But that is another topic, IMO you can merge, yes

@VincentLanglet
Copy link
Member Author

if none of the Sonata bundles are using those features. We could even study if this bundle is really needed or we can deprecate it.

None are using directly those methods. But there are templates used by SonataAdmin (which is the issue I try to solve).

@VincentLanglet VincentLanglet merged commit cde678f into 2.x Apr 9, 2022
@VincentLanglet VincentLanglet deleted the avoidConflict branch April 9, 2022 14:42
@core23
Copy link
Member

core23 commented Apr 10, 2022

The last release is broken, because DateTimeRuntime is not registered as a service

@VincentLanglet
Copy link
Member Author

Can you do the Pr to fix the three runtime ? (Or i'll do later) I m currently on phone ?

@jordisala1991
Copy link
Member

Would. be nice to have a minimal test setup that catches those basic errors

@VincentLanglet
Copy link
Member Author

Found time for #514

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jordisala1991 jordisala1991 jordisala1991 approved these changes

@dmaicher dmaicher Awaiting requested review from dmaicher

@phansys phansys Awaiting requested review from phansys

@core23 core23 Awaiting requested review from core23

@franmomu franmomu Awaiting requested review from franmomu

Assignees
No one assigned
Labels
minor
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Conflict with sonata-project/intl-bundle
4 participants
@VincentLanglet @jordisala1991 @franmomu @core23