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

get_pair_field() matches incorrectly if multiple pair fields have the same prefix #55

Closed
litzinger opened this Issue Dec 20, 2018 · 8 comments

Comments

Projects
None yet
2 participants
@litzinger
Copy link
Contributor

litzinger commented Dec 20, 2018

Description of the problem
I ran into this using Bloqs, but should be applicable to any module tag that contains var tag pairs. If I have two blocks that start with the same name, e.g. {slideshow}{/slideshow} and {slide}{/slide}, the regex in the method seems to match both, thus returning two items in the array, not one. For example the Bloqs code is making the following call:

$sections = $this->EE->api_channel_fields->get_pair_field($tagdata, $context->getShortname(), '');

In this case the shortname is either slideshow or slide, and when both tag pairs are in the template, it returns a match for both, thus the $sections variable has an array containing 2 keys, each key contains an array of the 4 parts of the regex match. If I add a 3rd block with the name of {slider}{/slider}, $sections will then contain 3 matches. This causes some major rendering issues, mostly duplicating output, but in Bloqs’ nesting feature, it bombs out completely b/c the expected result does not match what get_pair_field is actually returning.

How To Reproduce
Test with Bloqs, or create a module/plugin tag that contains multiple child tag pairs with the same prefix.

Environment Details:

  • Version: 4.3.4
  • PHP Version 7.1
  • OS: Ubuntu
  • Web Server: Apache

Additional context
Originally reported https://expressionengine.com/support/bugs/23706/bug-in-get_pair_field-method

@kevincupp kevincupp self-assigned this Jan 4, 2019

@kevincupp

This comment has been minimized.

Copy link
Contributor

kevincupp commented Jan 4, 2019

Sorry for the delay on that. See if this breaks the world:

--- a/system/ee/legacy/libraries/api/Api_channel_fields.php
+++ b/system/ee/legacy/libraries/api/Api_channel_fields.php
@@ -1537,8 +1537,10 @@ class Api_channel_fields extends Api {
                $pfield_chunk = array();
                $offset = 0;
                $field_name = $prefix.$field_name;
+               $modifier = '';
+               $end = strpos($tagdata, LD.'/'.$field_name, $offset);

-               while (($end = strpos($tagdata, LD.'/'.$field_name, $offset)) !== FALSE)
+               while ($end !== FALSE)
                {
                        // This hurts soo much. Using custom fields as pair and single vars in the same
                        // channel tags could lead to something like this: {field}...{field}inner{/field}
@@ -1582,7 +1584,8 @@ class Api_channel_fields extends Api {
                                $pfield_chunk[] = $chunk_array;
                        }

-                       $offset = $end + 1;
+                       $end = strpos($tagdata, LD.'/'.$field_name.$modifier.RD, $offset);
+                       $offset = (int) $end + 1;
                }

                return $pfield_chunk;
@litzinger

This comment has been minimized.

Copy link
Contributor Author

litzinger commented Jan 22, 2019

I forgot all about this until now. I'll try to investigate this soon.

@litzinger

This comment has been minimized.

Copy link
Contributor Author

litzinger commented Jan 23, 2019

Took a stab and added this to my local copy of a large site and it's eventually running out of memory somewhere. I undo the change and everything returns to normal.

Allowed memory size of 268435456 bytes exhausted (tried to allocate 20480 bytes)

edit: this is on a page that does not have any issues with tag pairs with similar prefixes, but it does have a lot of tag pairs in general (e.g. Bloqs fields)

@kevincupp

This comment has been minimized.

Copy link
Contributor

kevincupp commented Jan 23, 2019

Poo, thanks for testing. I'm having trouble getting that to happen on my end, would you be able to send me that template text?

@kevincupp

This comment has been minimized.

Copy link
Contributor

kevincupp commented Jan 23, 2019

Thanks for the assist, had some colliding variables that were causing an infinite loop. Here's a slightly modified diff:

--- a/system/ee/legacy/libraries/api/Api_channel_fields.php
+++ b/system/ee/legacy/libraries/api/Api_channel_fields.php
@@ -1537,14 +1537,17 @@ class Api_channel_fields extends Api {
                $pfield_chunk = array();
                $offset = 0;
                $field_name = $prefix.$field_name;
+               $end = strpos($tagdata, LD.'/'.$field_name, $offset);

-               while (($end = strpos($tagdata, LD.'/'.$field_name, $offset)) !== FALSE)
+               while ($end !== FALSE)
                {
                        // This hurts soo much. Using custom fields as pair and single vars in the same
                        // channel tags could lead to something like this: {field}...{field}inner{/field}
                        // There's no efficient regex to match this case, so we'll find the last nested
                        // opening tag and re-cut the chunk.

+                       $modifier = '';
+
                        if (preg_match("/".LD."{$field_name}((?::\S+)?)(\s.*?)?".RD."(.*?)".LD.'\/'."{$field_name}\\1".RD."/s", $tagdata, $matches, 0, $offset))
                        {
                                $chunk = $matches[0];
@@ -1563,8 +1566,8 @@ class Api_channel_fields extends Api {
                                        $params = $match[1][$idx];

                                        // Cut the chunk at the last opening tag
-                                       $offset = strrpos($chunk, $tag);
-                                       $chunk = substr($chunk, $offset);
+                                       $chunk_offset = strrpos($chunk, $tag);
+                                       $chunk = substr($chunk, $chunk_offset);
                                        $chunk = strstr($chunk, LD.$field_name);
                                        $content = substr($chunk, strlen($tag), -strlen(LD.'/'.$field_name.RD));
                                }
@@ -1582,7 +1585,8 @@ class Api_channel_fields extends Api {
                                $pfield_chunk[] = $chunk_array;
                        }

-                       $offset = $end + 1;
+                       $end = strpos($tagdata, LD.'/'.$field_name.$modifier.RD, $offset);
+                       $offset = (int) $end + 1;
                }

                return $pfield_chunk;
@litzinger

This comment has been minimized.

Copy link
Contributor Author

litzinger commented Jan 24, 2019

That fix didn't cause an infinite loop and the page loads fine for me, but I don't have a test case setup right now for the exact scenario reported (the similar prefixes), so I can't confirm that at the moment. I'll try to do that tonight.

@litzinger

This comment has been minimized.

Copy link
Contributor Author

litzinger commented Jan 28, 2019

Kevin, a co-worker just ran into this issue where the blocks were being duplicated as noted in the original report, and he just added your fix to EE and the issue is resolved. I think it's safe to say your patch is safe to merge.

If it helps, before your fix is applied, the tag pairs in the following order did work

{blocks_statistic}{/block_statistic}
{blocks_statistic_group}{/blocks_statistic_group}

But this tag pair order did not work

{blocks_statistic_group}{/blocks_statistic_group}
{blocks_statistic}{/block_statistic}

@kevincupp kevincupp closed this in 75aa734 Jan 28, 2019

@kevincupp

This comment has been minimized.

Copy link
Contributor

kevincupp commented Jan 28, 2019

Thanks for trying this out! It's in for the next release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.