8000 Replace twig extensions with twig extra bundle by franmomu · Pull Request #5865 · sonata-project/SonataAdminBundle · GitHub
[go: up one dir, main page]
Skip to content

Replace twig extensions with twig extra bundle #5865

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
Closed
wants to merge 1 commit into from
Closed

Replace twig extensions with twig extra bundle #5865

wants to merge 1 commit into from

Conversation

franmomu
Copy link
Member
@franmomu franmomu commented Feb 2, 2020 •
edited by greg0ire
Loading

Subject

Twig/extensions is deprecated. I have only found that we use truncate and trans filters. trans is in symfony/twig-bridge that we already use and for the truncate filter I added twig/string-extra.

To make it work we can or define a service with the StringExtension or tell the user to define that service (but it would be BC break as u.truncate won't work without it) or I decided to use twig/extra-bundle that will load the extensions used (right now it is only the StringExtension, but we would probably use IntlExtension as well).

I am targeting this branch, because this is BC.

Changelog

### Added
- Added `twig/string-extra` to use `u.truncate` filter
### Removed
- Removed deprecated `twig/extensions` library

@franmomu franmomu added the minor label Feb 2, 2020
phansys
phansys previously approved these changes Feb 2, 2020
View reviewed changes
@franmomu franmomu force-pushed the remove_twig_extensions branch from 31f0090 to 3342157 Compare February 2, 2020 14:55
@franmomu
Copy link
Member Author
franmomu commented Feb 2, 2020

There is a problem I didn't realised, it looks like the truncate function from String doesn't have the preserve parameter that TextExtension has.

And we are using it in list_html.html.twig and show_html.html.twig. Looking at the symfony issues, I haven't found anything about this, maybe there is another way to specify this in the String Component, I'm going to take a look.

@greg0ire
Copy link
Contributor
greg0ire commented Feb 2, 2020

I removed the double backticks from your changelog, since this is markdown.

@jorrit
Copy link
Contributor
jorrit commented Feb 3, 2020

The String component is still experimental. Perhaps it is not too late to try to make the |u.truncate() signature identical to the old |truncate() signature? Another solution to be able to remove twig-extensions is perhaps to copy twig_truncate_filter() / twig_wordwrap_filter() to let's say sonata-project / twig-extensions?

@franmomu
Copy link
Member Author
franmomu commented Feb 3, 2020

The String component is still experimental. Perhaps it is not too late to try to make the |u.truncate() signature identical to the old |truncate() signature? Another solution to be able to remove twig-extensions is perhaps to copy twig_truncate_filter() / twig_wordwrap_filter() to let's say sonata-project / twig-extensions?

Yes, I created this morning an issue in Symfony, let's see how it goes and if not, we can do as you say and copy/create a function for that.

@OskarStark
Copy link
Member

For the record symfony/symfony#35567

@VincentLanglet
Copy link
Member
VincentLanglet commented Feb 23, 2020 •
edited
Loading

It's merged symfony/symfony#35649 but we need symfony 5.1 to use it.

Does it mean we have to write a sonata_truncate filter with a comment Remove when we drop support of Symfony 4 and use the u.truncate function instead ?

@VincentLanglet VincentLanglet mentioned this pull request Feb 24, 2020
Closed
@OskarStark
Copy link
Member

IMO we just need to require symfony/string:^5.1 which is fine, as its standalone usable, isn't it?

@VincentLanglet VincentLanglet mentioned this pull request Mar 5, 2020
Closed
@SonataCI
Copy link
Collaborator
SonataCI commented Mar 6, 2020

Could you please rebase your PR and fix merge conflicts?

@franmomu
Copy link
Member Author

Closed in favor of #5973

@franmomu franmomu closed this Mar 21, 2020
@franmomu franmomu deleted the remove_twig_extensions branch March 21, 2020 19:32
@franmomu franmomu mentioned this pull request Jun 7, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@core23 core23 core23 approved these changes

@phansys phansys phansys left review comments

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

Successfully merging this pull request may close these issues.
8 participants
@franmomu @greg0ire @jorrit @OskarStark @VincentLanglet @SonataCI @phansys @core23
0