Make Templating optional #520

Hanmac · 2022-05-25T11:32:49Z

Subject

This makes the Symfony/Templating Dependency Optional.
The Twig Components don't need it, so they use the new base classes.
The Old Services for the Helper still exist, but they are only loaded when the templating component is used.

i don't know if it is 100% BC or not.

I am targeting this branch, because it should be BC.

Closes #518.

Changelog

### Deprecated
- Deprecated the Templating\Helper classes
- Deprecated the use of the Templating\Helper classes inside the Twig Runtime and Extensions
- Deprecated uses of LocaleDetectorInterface in favor of Symfony\Contracts\Translation\LocaleAwareInterface

### Changed
- Make `Symfony/Templating` Component optional

VincentLanglet

Currently this PR is made of multiple BC-break.

I think you'll have to duplicate the BaseHelper so far (one in the Helper folder and one in the Helper/Templating).

Also you'll have to keep the old alias/service declaration. But you can add some TODO for the next major in order to remove the deprecated code.

src/Helper/BaseHelper.php

src/Resources/config/autowire.php

Hanmac · 2022-05-25T13:46:54Z

the problem isn't a possible duplicated base helper, but if we really need duplicated DateTimeHelper code (if i can't have the Templating ones extend the Twig ones)

the question is more if it is really a BC break if the Templating\Helper\DateTimeHelper doesn't extend Symfony\Component\Templating\Helper anymore but implements the Symfony\Component\Templating\Helper\HelperInterface

or is it an BC break if Templating\Helper\DateTimeHelper doesn't extend from Templating\Helper\BaseHelper but from Helper\BaseHelper instead over Helper\DateTimeHelper ?

VincentLanglet · 2022-05-25T14:13:12Z

the question is more if it is really a BC break if the Templating\Helper\DateTimeHelper doesn't extend Symfony\Component\Templating\Helper anymore but implements the Symfony\Component\Templating\Helper\HelperInterface

If someone is doing if ($foo instanceof Symfony\Component\Templating\Helper) the condition was true and after this pr it will be false ; so it's a bc break.

Hanmac · 2022-05-25T14:28:51Z

so the only way without BC break would be to duplicate all the helper classes,

because i probably can't change the constructor to give them the Twig Helper. right?

VincentLanglet · 2022-05-28T14:29:36Z

so the only way without BC break would be to duplicate all the helper classes,
because i probably can't change the constructor to give them the Twig Helper. right?

I'm not sure about the purpose of this PR and the strategy you have in mind to do it so I currently cannot say what is the best way to do this without BC-break.

A constructor can be change if you do it this way:

public construct(Foo $foo) {}

to

public construct(Foo $foo, ?Bar $bar = null) {}

or

public construct(Foo|Bar $fooOrBar) {}

but it has to still work if you pass the old elements.

Hanmac · 2022-05-30T07:58:14Z

so the only way without BC break would be to duplicate all the helper classes,
because i probably can't change the constructor to give them the Twig Helper. right?

I'm not sure about the purpose of this PR and the strategy you have in mind to do it so I currently cannot say what is the best way to do this without BC-break.

the main reason was to make the templating dependency optional

for this i moved/created helpers that doesn't extend on templating Helper (no BC)
than can be used for the Twig Extension/Runtime. This should also be no BC, right?
Until this point that should be no BC, just duplicated code.

because of duplicated code, i wanted the templating helper use the twig helper, but i can't do that because it would change the templating Helper in a way that would be a BC.
The only way that would work where i can remove duplicated code:

either inherit from them, (not BC)
or use the twig helper as constructor param (would also not be BC because it can't be null)

VincentLanglet · 2022-05-30T08:21:59Z

for this i moved/created helpers that doesn't extend on templating Helper (no BC)
than can be used for the Twig Extension/Runtime. This should also be no BC, right?
Until this point that should be no BC, just duplicated code.

You can have a Helper and a HelperWithoutTemplating and then changing the constructor of the twig Extension to accepts

Helper|HelperWithoutTemplating

This is BC.

Hanmac · 2022-05-30T08:41:47Z

for this i moved/created helpers that doesn't extend on templating Helper (no BC)
than can be used for the Twig Extension/Runtime. This should also be no BC, right?
Until this point that should be no BC, just duplicated code.

You can have a Helper and a HelperWithoutTemplating and then changing the constructor of the twig Extension to accepts
Helper|HelperWithoutTemplating
This is BC.

This wouldn't work with the current php version defined in composer.json
currently that is ^7.3 || ^8.0, your suggestion would drop the 7.3 and make 8.0 required.

because my projects aren't for 8.0 yet i would like to avoid that.

VincentLanglet · 2022-05-30T09:44:23Z

This wouldn't work with the current php version defined in composer.json
currently that is ^7.3 || ^8.0, your suggestion would drop the 7.3 and make 8.0 required.
because my projects aren't for 8.0 yet i would like to avoid that.

You dont have to use the typehint, just the phpdoc.

Changing

    public function __construct(DateTimeHelper $helper)
    {
        $this->helper = $helper;
    }

to

/** @param DateTimeHelper|DateTimeHelperWithoutTemplating $helper */
    public function __construct(object $helper)
    {
        $this->helper = $helper;
    }

SonataCI · 2022-05-31T12:30:48Z

Could you please rebase your PR and fix merge conflicts?

SonataCI · 2022-06-25T17:20:50Z

Could you please rebase your PR and fix merge conflicts?

derrabus · 2022-07-12T14:08:45Z

Hello. I'm currently working on a codebase that uses this package and this PR would allow us to remove the templating component from our dependencies. I see that progress on the PR is stalling somehow. Is there anything I can do to help?

Hanmac · 2022-07-12T14:11:52Z

@VincentLanglet should i mark the Templating Helpers as Deprecated to be removed in a later PR?

src/DependencyInjection/SonataIntlExtension.php

src/Twig/DateTimeRuntime.php

VincentLanglet · 2022-07-12T14:42:42Z

@VincentLanglet should i mark the Templating Helpers as Deprecated to be removed in a later PR?

Yes please :)

And I assume you can add a NEXT_MAJOR: Remove this code above

if (interface_exists(HelperInterface::class)) {
            $loader->load('templating.php');
        }

then.

VincentLanglet · 2022-07-12T14:43:34Z

Hello. I'm currently working on a codebase that uses this package and this PR would allow us to remove the templating component from our dependencies. I see that progress on the PR is stalling somehow. Is there anything I can do to help?

I made some reviews. I think the PR seems ready, it just require some deprecation message I think @Hanmac will add.
Any extra review is welcome.

VincentLanglet

Same for others and it will be perfect

src/Twig/DateTimeRuntime.php

VincentLanglet · 2022-07-12T19:41:20Z

And take a look to the linter ;)

src/Helper/DateTimeHelper.php

src/Helper/NumberHelper.php

jordisala1991 · 2022-07-16T21:18:28Z

src/Helper/NumberHelper.php

+     * @param array            $symbols        The symbols used by \NumberFormatter
+     * @param string|null      $locale         The locale used to format the number
+     */
+    public function format($number, int $style, array $attributes = [], array $textAttributes = [], array $symbols = [], ?string $locale = null): string


do we need to make all format methods public? or the class should be used with the other methods? is this used outside this class?

src/Helper/BaseHelper.php

VincentLanglet

Looking at the interface methods, I wonder if it shouldn't be better to rename the class/interface to
DateTimeFormatter
NumberFormatter
LocaleSomething

It could be better than Helper

VincentLanglet · 2022-07-18T17:04:54Z

(and tests are failing)

Hanmac · 2022-07-19T09:14:43Z

Looking at the interface methods, I wonder if it shouldn't be better to rename the class/interface to DateTimeFormatter NumberFormatter LocaleSomething

It could be better than Helper

i don't know about different helper names, can you decide before i change the code again?

VincentLanglet · 2022-07-19T09:22:09Z

i don't know about different helper names, can you decide before i change the code again?

To me, DateTimeFormatter and NumberFormatter are much better names since all the methods are "formatFoo".
For the LocaleHelper, there is better to find, not sure what.

Looking at https://github.com/sonata-project/SonataIntlBundle/blob/2.x/src/Twig/LocaleRuntime.php, all three method are returning a localized name. I don't know if something like a Localizer or NameLocalizer or ... would be a thing.

Let's wait for @jordisala1991 opinion.

jordisala1991 · 2022-07-20T09:12:16Z

i don't know about different helper names, can you decide before i change the code again?

To me, DateTimeFormatter and NumberFormatter are much better names since all the methods are "formatFoo". For the LocaleHelper, there is better to find, not sure what.

Looking at https://github.com/sonata-project/SonataIntlBundle/blob/2.x/src/Twig/LocaleRuntime.php, all three method are returning a localized name. I don't know if something like a Localizer or NameLocalizer or ... would be a thing.

Let's wait for @jordisala1991 opinion.

Agree! To me if we are introducing new code, let's try to be as good as possible, because we are going to maintain it.

Hanmac · 2022-07-20T09:38:28Z

@jordisala1991 your opinion for better names?

VincentLanglet · 2022-07-20T11:56:32Z

@jordisala1991 your opinion for better names?

I talked with him.

DateTimeFormatter
NumberFormatter
Localizer

are ok.

Hanmac · 2022-07-20T12:13:17Z

i did better names, also i made twig use the interfaces instead.

and i added the interfaces to autowire

VincentLanglet · 2022-07-20T12:28:38Z

Some static analysis build are failing

core23 · 2022-09-15T22:19:57Z

src/Resources/config/intl.php

            ])

-        ->set('sonata.intl.templating.helper.datetime', '%sonata.intl.templating.helper.datetime.class%')


Removing a service alias is a BC break (or this kind of change was handled as a BC break a few years ago...)

VincentLanglet requested changes May 25, 2022

View reviewed changes

src/Helper/BaseHelper.php Show resolved Hide resolved

src/Helper/BaseHelper.php Outdated Show resolved Hide resolved

src/Resources/config/autowire.php Outdated Show resolved Hide resolved

Hanmac force-pushed the templatingOpt branch from 2794fd4 to 8a4f3b8 Compare May 30, 2022 08:55

VincentLanglet requested a review from a team May 30, 2022 19:38

SonataCI added pending author and removed pending author labels May 31, 2022

SonataCI added the pending author label Jun 25, 2022

Hanmac force-pushed the templatingOpt branch from 484639d to b1f6ca4 Compare June 28, 2022 07:53

SonataCI removed the pending author label Jun 28, 2022

Hanmac marked this pull request as ready for review June 28, 2022 08:03

VincentLanglet requested changes Jul 12, 2022

View reviewed changes

src/DependencyInjection/SonataIntlExtension.php Outdated Show resolved Hide resolved

src/Twig/DateTimeRuntime.php Show resolved Hide resolved

VincentLanglet requested review from jordisala1991 and a team July 12, 2022 14:43

VincentLanglet added the minor label Jul 12, 2022

VincentLanglet requested changes Jul 12, 2022

View reviewed changes

src/Twig/DateTimeRuntime.php Outdated Show resolved Hide resolved

src/Twig/DateTimeRuntime.php Outdated Show resolved Hide resolved

make Helper classes without Templating

71f238e

Hanmac added 2 commits July 15, 2022 11:23

~ without psalm suppress

b2bd5c9

~ phpstan fix

17b500b

VincentLanglet previously approved these changes Jul 16, 2022

View reviewed changes

jordisala1991 reviewed Jul 16, 2022

View reviewed changes

~ remove getName and make properties private

1f953d9

Hanmac dismissed VincentLanglet’s stale review via 1f953d9 July 17, 2022 07:58

Hanmac added 2 commits July 17, 2022 12:18

~ use interfaces for the helper

f0d6cf0

~ use LocaleAwareInterface

72a201e

VincentLanglet reviewed Jul 18, 2022

View reviewed changes

Hanmac added 2 commits July 18, 2022 20:33

~ fix tests

7e6e499

~ add more deprecations for LocaleDetector

e9ee683

~ better names, also make twig use the interfaces instead

1bdeb91

VincentLanglet previously approved these changes Jul 20, 2022

View reviewed changes

~ fixed checks

99dac1c

Hanmac dismissed VincentLanglet’s stale review via 99dac1c July 20, 2022 12:34

VincentLanglet approved these changes Jul 20, 2022

View reviewed changes

VincentLanglet requested a review from jordisala1991 July 20, 2022 16:15

jordisala1991 approved these changes Jul 20, 2022

View reviewed changes

VincentLanglet merged commit 5f3469b into sonata-project:2.x Jul 20, 2022

Hanmac deleted the templatingOpt branch July 20, 2022 17:29

AirBair mentioned this pull request Aug 16, 2022

Allow symfony/translation-contracts 3 #547

Merged

core23 reviewed Sep 15, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make Templating optional #520

Make Templating optional #520

Hanmac commented May 25, 2022 •

edited

Loading

VincentLanglet left a comment

Hanmac commented May 25, 2022 •

edited

Loading

VincentLanglet commented May 25, 2022

Hanmac commented May 25, 2022

VincentLanglet commented May 28, 2022

Hanmac commented May 30, 2022 •

edited

Loading

VincentLanglet commented May 30, 2022

Hanmac commented May 30, 2022

VincentLanglet commented May 30, 2022 •

edited

Loading

SonataCI commented May 31, 2022

SonataCI commented Jun 25, 2022

derrabus commented Jul 12, 2022

Hanmac commented Jul 12, 2022

VincentLanglet commented Jul 12, 2022

VincentLanglet commented Jul 12, 2022

VincentLanglet left a comment

VincentLanglet commented Jul 12, 2022

jordisala1991 Jul 16, 2022

VincentLanglet left a comment

VincentLanglet commented Jul 18, 2022

Hanmac commented Jul 19, 2022

VincentLanglet commented Jul 19, 2022

jordisala1991 commented Jul 20, 2022

Hanmac commented Jul 20, 2022

VincentLanglet commented Jul 20, 2022

Hanmac commented Jul 20, 2022

VincentLanglet commented Jul 20, 2022

core23 Sep 15, 2022

		])

		->set('sonata.intl.templating.helper.datetime', '%sonata.intl.templating.helper.datetime.class%')

Make Templating optional #520

Make Templating optional #520

Conversation

Hanmac commented May 25, 2022 • edited Loading

Subject

Changelog

VincentLanglet left a comment

Choose a reason for hiding this comment

Hanmac commented May 25, 2022 • edited Loading

VincentLanglet commented May 25, 2022

Hanmac commented May 25, 2022

VincentLanglet commented May 28, 2022

Hanmac commented May 30, 2022 • edited Loading

VincentLanglet commented May 30, 2022

Hanmac commented May 30, 2022

VincentLanglet commented May 30, 2022 • edited Loading

SonataCI commented May 31, 2022

SonataCI commented Jun 25, 2022

derrabus commented Jul 12, 2022

Hanmac commented Jul 12, 2022

VincentLanglet commented Jul 12, 2022

VincentLanglet commented Jul 12, 2022

VincentLanglet left a comment

Choose a reason for hiding this comment

VincentLanglet commented Jul 12, 2022

jordisala1991 Jul 16, 2022

Choose a reason for hiding this comment

VincentLanglet left a comment

Choose a reason for hiding this comment

VincentLanglet commented Jul 18, 2022

Hanmac commented Jul 19, 2022

VincentLanglet commented Jul 19, 2022

jordisala1991 commented Jul 20, 2022

Hanmac commented Jul 20, 2022

VincentLanglet commented Jul 20, 2022

Hanmac commented Jul 20, 2022

VincentLanglet commented Jul 20, 2022

core23 Sep 15, 2022

Choose a reason for hiding this comment

Hanmac commented May 25, 2022 •

edited

Loading

Hanmac commented May 25, 2022 •

edited

Loading

Hanmac commented May 30, 2022 •

edited

Loading

VincentLanglet commented May 30, 2022 •

edited

Loading