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

Create custom intl twig functions #5975

Closed
wants to merge 1 commit into from

Conversation

franmomu
Copy link
Member

Subject

There are conflicts using the last version of this bundle and SonataIntlBundle because both register twig functions with the same name and different signature.

The only way I could come up with is removing twig/extra-bundle, so the IntlExtension from twig/intl-extra is not automatically registered anymore and I created our IntlExtension which decorates the Twig one and exposes only the methods we're using changing their names to sonata_*.

The problem I see now is people using these functions in their projects and we should remove the in the next major version since this is a patch to make it work with sonata/intl-bundle. Don't know if there is a way to make twig functions as internal or something like that.

I am targeting this branch, because this is BC.

Closes sonata-project/SonataIntlBundle#299

Changelog

### Removed
- `twig/extra-bundle` dependency
### Fixed
- Compatibility with `sonata/intl-bundle`

To test:

composer config repositories.franmomu vcs https://github.com/franmomu/SonataAdminBundle
composer require sonata-project/admin-bundle "dev-allow_intl_extension as 3.62.1"

@franmomu franmomu force-pushed the allow_intl_extension branch from 2d82103 to 99508ab Compare March 19, 2020 11:50
@@ -94,6 +97,14 @@ public function load(array $configs, ContainerBuilder $container)
if ('@SonataAdmin/CRUD/history_revision_timestamp.html.twig' === $config['templates']['history_revision_timestamp']) {
$config['templates']['history_revision_timestamp'] = '@SonataAdmin/CRUD/Intl/history_revision_timestamp.html.twig';
}

$twigIntlExtension = new Definition(TwigIntlExtension::class);
Copy link
Member

Choose a reason for hiding this comment

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

Can you please try to extract some small methods. This will improve readability.

@franmomu franmomu force-pushed the allow_intl_extension branch 2 times, most recently from c1078c2 to 7788c30 Compare March 19, 2020 12:58
@franmomu franmomu force-pushed the allow_intl_extension branch from 7788c30 to 2bb1882 Compare March 19, 2020 13:19
@phansys
Copy link
Member

phansys commented Mar 19, 2020

Great job @franmomu!
IMO, there is a missing part yet. The Twig functions at "sonata-project/intl-bundle" are using helpers to infer the locale and the timezone, in order to provide these values as arguments for the Intl core methods.
If those values are not passed anymore, the results will be different where the values were different than the default.
In that case, the users will be forced to update their codebase in order to provide these values manually each time the functions are called.

@franmomu
Copy link
Member Author

Taking a look at this, we would need to import all the Timezone and Locale classes. Add the timezone configuration to AdminBundle and in case in the SonataIntlBundle is enabled and there is no configuration in SonataAdminBundle, it should use the SonataIntlBundle one to be BC. And also decide what to do with UserBasedTimezoneDetector.

So... at this point I don't know if it's better to just revert #5835 and make all these changes in master or keep patching things.

@phansys
Copy link
Member

phansys commented Mar 21, 2020

So... at this point I don't know if it's better to just revert #5835 and make all these changes in master or keep patching things.

The revert of #5835 has my vote 👍

@SonataCI
Copy link
Collaborator

Could you please rebase your PR and fix merge conflicts?

@franmomu franmomu closed this Mar 21, 2020
@phansys phansys mentioned this pull request Mar 28, 2020
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Conflict with sonata-project/intl-bundle
4 participants