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

Drop SonataIntlBundle #5835

Merged
merged 2 commits into from
Mar 6, 2020
Merged

Conversation

franmomu
Copy link
Member

@franmomu franmomu commented Jan 15, 2020

Subject

I was in the middle of dropping support of Symfony < 4.4 and adding support of 5 in master branch. The problem is that SonataIntlBundle doesn't support Symfony 5 and looks like it's not easy because of SonataUserBundle.

So I decided to remove SonataIntlBundle dependency. Right now if the bundle is enabled, it will automatically load its templates to use localized information. I basically copied the templates and then modified them to use twig/intl-extra functions.

This is not finished, I have to try it, but before going further, I would like to see if that's fine.

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

Changelog

### Added
- Import templates from SonataIntlBundle
### Removed
- Dropped SonataIntlBundle

To do

  • Update the tests
  • Update the documentation

core23
core23 previously approved these changes Jan 16, 2020
@@ -514,6 +514,10 @@ public function getConfigTreeBuilder()
->info('Translate group label')
->end()

->booleanNode('use_intl_templates')
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a call to info()

<?xml version="1.0" encoding="UTF-8"?>
<container xmlns="http://symfony.com/schema/dic/services" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://symfony.com/schema/dic/services http://symfony.com/schema/dic/services/services-1.0.xsd">
<services>
<service id="sonata.templates.twig_intl.extension" class="Twig\Extra\Intl\IntlExtension">
Copy link
Member

Choose a reason for hiding this comment

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

Do we need an own instance of this class? I prefer updating the docs and refer to the intl docs instead of registering a new instance.

Copy link
Member Author

Choose a reason for hiding this comment

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

mmm what I can do is to only load this when the SonataIntlBundle is used to maintain BC and remove this in the next major version. And also update the docs to show how to use it.

Copy link
Member

Choose a reason for hiding this comment

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

AFAIK, you should set the default of use_intl_templates to true to keep BC. So we don't need a BC layer here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the intl templates are only loaded when SonataIntlBundle is enabled.

I'll be away some days, I'll continue when I come back or if someone wants to continue, feel free to do it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Here I am again! so I think these are the the scenarios here:

  • The user doesn't have SonataIntlBundle installed:
    • Right now the bundle doesn't load anything.
    • use_intl_templates: true it will load the intl templates and register the service.
    • use_intl_templates: false it won't load anything.
  • The user has SonataIntlBundle installed:
    • Right now the bundle loads the intl templates and the SonataIntlBundle load the Twig extension that registers the twig functions needed.
    • use_intl_templates: true it will load the intl templates and register the service.
    • use_intl_templates: false it will also load the intl templates and register the service.

Copy link
Member

Choose a reason for hiding this comment

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

So there is no change if you are still using SonataIntlBundle?

Copy link
Member Author

Choose a reason for hiding this comment

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

It will load the intl templates but from the AdminBundle. We could throw a warning deprecation in case you have installed SonataIntlBundle but not enabled use_intl_templates.

I removed the definition and added the twig/extra-bundle so there is no need to register the service.

@SonataCI
Copy link
Collaborator

Could you please rebase your PR and fix merge conflicts?

@SonataCI
Copy link
Collaborator

Could you please rebase your PR and fix merge conflicts?

docs/reference/internationalization.rst Outdated Show resolved Hide resolved
docs/reference/internationalization.rst Outdated Show resolved Hide resolved
Copy link
Member

@VincentLanglet VincentLanglet left a comment

Choose a reason for hiding this comment

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

is something missing in this PR to be merged ? @greg0ire @core23 @franmomu :)

@greg0ire greg0ire merged commit 64ecd65 into sonata-project:3.x Mar 6, 2020
@greg0ire
Copy link
Contributor

greg0ire commented Mar 6, 2020

Thanks @franmomu !

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

5 participants