search

TheBigBrainsCompany / TbbcMoneyBundle

This bundle is used to integrate the Money library (https://github.com/mathiasverraes/money) into a Symfony project. This library is based on Fowler's Money pattern (https://verraes.net/2011/04/fowler-money-pattern-in-php/)
MIT License
192 stars 74 forks source link

feat: remove symfony/templating #154

Closed Chris53897 closed 7 months ago

Chris53897 commented 8 months ago

https://github.com/TheBigBrainsCompany/TbbcMoneyBundle/issues/152

johanwilfer commented 8 months ago

As this is a BC break we need to release version 7 I think we we merge this, can you update the change log accordingly if you agree?

johanwilfer commented 7 months ago

Can I ask for your help here @Chris53897 ? Seems we have this code in the extension:

        if (in_array('twig', $config['templating']['engines'], true)) {
            $loader->load('twig_extension.xml');
        }

This now fails when I removed the configuration for templating. Not sure if we should load that unconditionally now?

johanwilfer commented 7 months ago

I have tagged a new 5.2 version and made a 5.x branch where we can commit fixes for the 5.x And once this is ready we can merge it to master to release 6.x from master when we are ready for that.

Chris53897 commented 7 months ago

<< Not sure if we should load that unconditionally now? I did ask myself the same question. But i think you should decide this as the maintainer ;) Maybe it is easier to maintain if we just load it unconditionally.

johanwilfer commented 7 months ago

I did ask myself the same question. But i think you should decide this as the maintainer ;) Maybe it is easier to maintain if we just load it unconditionally.

Well, I ended up as the maintainer after submitting a few patches ;) So if you want to aid here I am really interested in what you think.

Lets load it unconditionally then, in all the projects I use it I have twig anyway and can't see how that could harm.

johanwilfer commented 7 months ago

Now this looks good I think, what do you say @Chris53897 - merge this to master (to be 6.x) ?

Chris53897 commented 7 months ago

Sounds good to me. After that the 2 other PRs can be merged, and after that two seperate PR to fix some psalm erros, and add re-introduce support for doctrine/orm 3

johanwilfer commented 7 months ago

Sounds good to me. After that the 2 other PRs can be merged, and after that two seperate PR to fix some psalm erros, and add re-introduce support for doctrine/orm 3

Sounds great, I will merge this then!

Chris53897 commented 7 months ago

Of course please wait with tags for 6.x until this is all stable and tested,