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

Added twig extension for preg_match and preg_split #2788

Closed

Conversation

thexmanxyz
Copy link
Contributor

I added a twig extension for the PHP methods preg_split and preg_match (including code documentation) to cover those aspects not only within Gantry but also in Grav as well. For more information please also check this FR (gantry/gantry5#2586) and this PR (gantry/gantry5#2590).

@@ -157,6 +157,8 @@ public function getFunctions()
new \Twig_SimpleFunction('repeat', [$this, 'repeatFunc']),
new \Twig_SimpleFunction('regex_replace', [$this, 'regexReplace']),
new \Twig_SimpleFunction('regex_filter', [$this, 'regexFilter']),
new \Twig_SimpleFunction('preg_match', [$this, 'pregMatch']),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cannot we just call the method directly:

new \Twig_SimpleFunction('preg_match', 'preg_match')

Copy link
Contributor Author

@thexmanxyz thexmanxyz Jan 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mahagr The preg_match code is taken directly from Gantry 5 in the form it is currently in this PR (without offset and flags parameter). TBH IDK if incompatibilities might occur if we have different method signatures. Same for the the preg_split but only with the flags parameter omitted. I decided to also omit the flags parameter as it was skipped for preg_match within Gantry5 as well.

Additionally preg_match in Gantry 5 does not act the same way for return values as the default PHP method. Hence we can only update this PR for preg_split to something like this:

new \Twig_SimpleFunction('preg_split', 'preg_split')

otherwise we might break existing code that relies on the Twig preg_match function. Let me know and I will push a commit with the updated code for preg_split.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mahagr you probably know best about the code in G5, but it seems that false return value is there for a reason in G5.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, right. The preg_match() PHP method returns true/false by default and puts the matching strings into the third parameter. It looks like @newkind added this method back in 2015, so I guess it's being used in a few themes.

If this was a filter, I would agree on the returned value, but in the function, not sure. I guess that you cannot update values inside parameters in twig, so it would make sense to remove the third parameter altogether -- it's being returned anyway.

@mahagr
Copy link
Member

mahagr commented Feb 11, 2021

Merged but renamed the methods to match the other two.

@mahagr mahagr closed this Feb 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants