kenjis / codeigniter-ss-twig

A Simple and Secure Twig integration for CodeIgniter 3.x and 4.x
MIT License
168 stars 46 forks source link

Adding filters #61

Closed atlza closed 1 year ago

atlza commented 1 year ago

Hi, I'm using your library in a non-CI app (native php) since a while now and it works great.

As I'm moving the app to php 8.0 I've updated your library to 4.1 and saw that Filters are no longer supported. Is there any particular reason ?

I have a very large number of views and can't switch all filters to functions.
As Twig 3.3 still use filters, I tried to implements addFilter as you did for addFunction and it works like a charm.

So if there is no particular reason for removing addFilter in your lib I can submit a pull Request whit AddFilter functionality.

If this a duplicate of #20 sorry I was not sure.

Regards.

kenjis commented 1 year ago

As I'm moving the app to php 8.0 I've updated your library to 4.1 and saw that Filters are no longer supported.

What do you mean? 4.x has the exactly same functionality with v1.0. I did not remove anything.

So if there is no particular reason for removing addFilter in your lib I can submit a pull Request whit AddFilter functionality.

addFilter() was not implemented in v1.0. https://github.com/kenjis/codeigniter-ss-twig/blob/master/libraries/Twig.php So it is not in 4.x.

atlza commented 1 year ago

My bad, you're right, i was adding filter this way : $this->twig->addFilter('getRelation', new Twig_Filter_Function('getRelation'));

As it does not work anymore, I've been trying to add filters the way it's done for functions. And as I said, it's easy and works great.

So I was asking if there is a particular reason to not implement it natively in your lib. If not, I'd be glad to make a pull request if it can help. Else I can just work with a fork.

Anyway, thank for your work I've been using it both with CI projects and non CI-projects and I would even recommend to remove CI 4.2 from the requirements as the library works just fine in a php Native app.

kenjis commented 1 year ago

So I was asking if there is a particular reason to not implement it natively in your lib.

No particular reason. Just I did not need it, and nobody sent a PR to implement it.

20 is for it, so if you send a PR, I would merge it if it works fine!

atlza commented 1 year ago

Ok, I'll submit a PR in the nexts hours.
Regards.

atlza commented 1 year ago

It took me some time but I've submited a pull request with this feature, and also the ability for non CI users to add the lib to their projet as I do. Hope it helps.

kenjis commented 1 year ago

Closed by #65