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

[WIP] Allow Twig 3 #5973

Closed
wants to merge 1 commit into from
Closed

[WIP] Allow Twig 3 #5973

wants to merge 1 commit into from

Conversation

phansys
Copy link
Member

@phansys phansys commented Mar 18, 2020

Subject

Allow Twig 3.

I am targeting this branch, because these changes can not be done at the stable branch.

Related to #5788, symfony/symfony#35649.

Changelog

### Added
- Added support for "twig/twig:^3.0".

To do

  • Use stable versions of "sonata-project/form-extensions" and "sonata-project/twig-extensions" when released;
  • Ensure results from StringExtension is the same as TextExtension;
  • Wait for the removal of "sonata-project/intl-bundle".

When symfony/symfony#35649 is available in a stable release, we need to remove some of the stuff added in this PR and bump "symfony/string" to the version which ships them.
Regarding the Twig truncate filter, I had to make some slight modifications at test assertions since the new behavior differs a little bit from the previous one. Plus, I renamed "preserve" option to "cut", in order to be consistent with the term used at the String component (which also has the opposite behavior than "preserve").
In this way, I think we should deprecate the "preserve" option at the stable branch.

@phansys
Copy link
Member Author

phansys commented Mar 21, 2020

We could try to move StringExension (and UnicodeString) added here to the stable branch, and then, merge them later here.
The only concern I have about this is the small difference between the results from truncate() and u.truncate(), which I guess can be accepted in a major release, but the case could be not the same in a minor.
WDYT?

@phansys phansys marked this pull request as ready for review March 21, 2020 18:51
@SonataCI
Copy link
Collaborator

Could you please rebase your PR and fix merge conflicts?

@phansys phansys force-pushed the twig_3 branch 2 times, most recently from ad4c1ab to 36aeaa5 Compare March 22, 2020 03:45
@greg0ire
Copy link
Contributor

I think if using the new behavior is opt-in, then it's ok to have it in the stable branch. It's ok to deprecate the old behavior in favor of a new one if you don't force it on people. They will be more happy to see it clearly in a minor than have it non-obvious, in the middle of other things in a big major.

@phansys
Copy link
Member Author

phansys commented Mar 22, 2020

Great proposal. I'll be working on that path.
Thank you.

@SonataCI
Copy link
Collaborator

Could you please rebase your PR and fix merge conflicts?

@greg0ire
Copy link
Contributor

What's the status of this? Should it be reviewed or is it too soon? I see there is an unchecked checkbox in the todo list

@phansys phansys changed the title Allow Twig 3 [WIP] Allow Twig 3 Mar 28, 2020
@phansys
Copy link
Member Author

phansys commented Mar 28, 2020

What's the status of this? Should it be reviewed or is it too soon? I see there is an unchecked checkbox in the todo list

We need to decide what to do with "sonata-project/intl-bundle", since it provides some helper functions over the basic Intl features (#5975 (comment)).

/cc @franmomu.

@franmomu
Copy link
Member

franmomu commented Mar 28, 2020

What's the status of this? Should it be reviewed or is it too soon? I see there is an unchecked checkbox in the todo list

We need to decide what to do with "sonata-project/intl-bundle", since it provides some helper functions over the basic Intl features (#5975 (comment)).

/cc @franmomu.

I was thinking... what about to not deprecate the bundle?

It could be updated using twig/intl-extra package (and make it compatible with Twig 3 and Symfony 5) and remove the UserBasedTimezoneDetector to remove the dependency with user-bundle? We could add a recipe showing how to add it.

To me, it looks more like it should belong to a separated package instead of the AdminBundle one.

@phansys
Copy link
Member Author

phansys commented Mar 28, 2020

I agree @franmomu.
Even, maybe we could leave this class and do something like this:

public function getTimezone()
{
    if (!($token = $this->securityContext->getToken()) || !is_object($user = $token->getUser())) {
        return null;
    }

    if ($user instanceof 'Sonata\UserBundle\Model\User') {
        return $user->getTimezone();
    }

    $ro = new \ReflectionObject($user);
    if ($ro->hasMethod('getTimezone')) {
        $rm = $ro->getMethod('getTimezone');

        if ($rm->isPublic() && !$rm->isStatic() && 0 === $rm->getNumberOfRequiredParameters()) {
            return $user->getTimezone();
        }
    }

    return null;
}

But I'd prefer to declare a TimezoneAwareInterface in "sonata-project/intl-bundle", implement it at "sonata-project/user-bundle", and check against that interface instead.

WDYT?

@franmomu
Copy link
Member

But I'd prefer to declare a TimezoneAwareInterface in "sonata-project/intl-bundle", implement it at "sonata-project/user-bundle", and check against that interface instead.

WDYT?

Exactly, I think this is the best way to deal with sonata/intl-bundle right now.

@VincentLanglet
Copy link
Member

What's the next step @franmomu @phansys ?

@phansys
Copy link
Member Author

phansys commented May 17, 2020

We need to make the required changes at "sonata-project/intl-bundle". I had no time these weeks.
If someone can, help is welcomed.

@VincentLanglet
Copy link
Member

Seems like a discussion about the status of the SonataIntlBundle is needed then, I thought it was deprecated sonata-project/SonataIntlBundle#280...

@sonata-project/contributors ?

If it's not deprecated anymore, we should start by updating the bundle description.

@SonataCI
Copy link
Collaborator

Could you please rebase your PR and fix merge conflicts?

@VincentLanglet
Copy link
Member

I think you can close this PR @phansys, isn't it ?

@phansys
Copy link
Member Author

phansys commented Jun 19, 2020

I think you can close this PR @phansys, isn't it ?

Yes, I guess. Closing.

@phansys phansys closed this Jun 19, 2020
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.

None yet

6 participants