[D7] preg_split in _filter_url breaks for long html tags

D10 issue was committed. Therefore changing the status to NW, so that the D7 patch can be checked if it has all changes from the D10 patch and can be moved to NR. Thanks!

Ready for another round of reviews, I backported any relevant changes from the final D9/10 committed codes.

Again, not D7 specific. You can continue the back port but this needs to be fixed in the latest release.

ooof sorry I see #21 now sorry. Annoyed at the duplicate issue... sorry for the noise.

Great work @stefanos!

@stefanos.petrakis Thanks for working on this! I have reviewed the merge request and added some comments.

Thanks @poker10, this is ready for another round.

Thanks, the changes looks good!

However I am a bit concerned by the testbot on PHP 8.2 / PostgreSQL 13.5, as this combination is ending with a fatal error, see: https://www.drupal.org/pift-ci-job/2626124 → . Not sure if these are some random errors (I have tried 5 runs, see the history), but merge requests on other D7 issues on this PHP 8.2 / PostgreSQL 13.5 combination are green (see: https://www.drupal.org/pift-ci-job/2626080 → ). And also Drupal 7 core is green either. So probably there is some problem related to this patch. It would probably be a good idea to take a look at it.

Other than that, it seems like a straight backport of the D10 patch, so once this testbot issue are solved, I think this could be RTBC. Thanks!

Let's clear this, I am gonna investigate it right now.

Found it, it was the pcre.backtrack_limit messing with preg_replace calls that the pgsql layer was issuing. A little refactoring made lots of sense to keep that effect restricted.

Waiting for the tests to set this back to NR.

Thanks, the PostgreSQL problem seems to be solved. But hmm, actually all tests are green except the PHP 7.2 / MySQL 5.6, see: https://www.drupal.org/pift-ci-job/2626322 →

Let's have a look at this one too.

Unfortunately, this needs to stay at "Needs Work" for the time.

The test introduced here is based on the idea of breaking the preg_* function using pcre.backtrack_limit; this idea as @poker10 observed does not work for any PHP version smaller than 7.3. From PHP7.3 and higher all preg_* functions return bool(FALSE) on failures.

This breaks the new tests due to this part:

    // Split at all tags; ensures that no tags or attributes are processed.
    $chunks = is_null($text) ? array('') : preg_split('/(<.+?>)/is', $text, -1, PREG_SPLIT_DELIM_CAPTURE);

PHP7.2 (and 7,1 and 7,0): preg_split splits the string partially and $chunks is therefore not FALSE; later on the _filter_url() returns a result that differs from the input text.

PHP5.6 (and 5.3 and probably all 5,*): preg_split does not split, but returns the whole string and here $chunks is not FALSE either; later on the _filter_url() returns the whole string as a result by coincidence keeps the tests from breaking.

Ideas for extending the tests to support 5.x and 7,0,1,2 are welcome, I have to think about this some.

FYI: I tried the different PHP versions here => https://3v4l.org/mhmnn#v5.6.40

using this snippet:

<?php
// example code

ini_set('pcre.backtrack_limit', 1);
$text = "<p>www.test.com</p>";
$chunks = is_null($text) ? array('') : preg_split('/(<.+?>)/is', $text, -1, PREG_SPLIT_DELIM_CAPTURE);
var_dump($chunks);

I started to look into this compatibility puzzle. As I was browsing the PHP docs for preg_split() and preg_replace_callback(), I noticed something that looks useful: preg_last_error(). I think it will be more reliable to ask PHP whether there was an error instead of trying to guess based on the result.

Following the example of Comment #35, I tested the following snippet:

$text = '<p>www.test.com</p>';

echo 'pcre.backtrack_limit: ' . ini_get('pcre.backtrack_limit') . PHP_EOL;
$chunks = preg_split('/(&lt;.+?&gt;)/is', $text, -1, PREG_SPLIT_DELIM_CAPTURE);
echo 'error: ' . preg_last_error() . PHP_EOL;
is_array($chunks) ? print_r($chunks) : var_dump($chunks);

ini_set('pcre.backtrack_limit', 1);
echo 'pcre.backtrack_limit: ' . ini_get('pcre.backtrack_limit') . PHP_EOL;
$chunks = preg_split('/(&lt;.+?&gt;)/is', $text, -1, PREG_SPLIT_DELIM_CAPTURE);
echo 'error: ' . preg_last_error() . PHP_EOL;
is_array($chunks) ? print_r($chunks) : var_dump($chunks);

For example, you can see the result using PHP 7.2 at https://3v4l.org/peFdO#v7.2.34. (How do you get results in multiple PHP versions at once?) I tested all the PHP versions tested in #29. (I tested the latest patch version available for each.) As predicted in #34, the last line varied, but I always got 0 and 2 for preg_last_error(): that is, PREG_NO_ERROR and PREG_BACKTRACK_LIMIT_ERROR.

I think it makes sense to replace a line like

    $chunks = is_null($text) ? array('') : preg_split('/(<.+

)/is', $text, -1, PREG_SPLIT_DELIM_CAPTURE);
?>

with

I think it makes sense to replace a line like

    $chunks = !preg_last_error() ? array('') : preg_split('/(<.+

)/is', $text, -1, PREG_SPLIT_DELIM_CAPTURE);
?>

Oops, I pasted my comment into the Issue summary field. Let me try to restore it.

Despite the last few lines of Comment #36, I think it will be clearer to use a few more lines. First, move $saved_text = $text; inside the loop. Then, add something like this after each call to a preg_ function:

    if (preg_last_error()) {
      $text = $saved_text;
      continue;
    }

In particular, use that instead of putting most of the loop inside if ($chunks !== FALSE) { ... }.

I made a similar suggestion in #3239472-56: preg_split in _filter_url breaks for long html tags → and the following comment, and in 📌 Reduce complexity in _filter_url() Active . In Comment #2 on that issue, @alexpott recommends completely refactoring the code in D10, so I think we do not have to worry here about keeping the code consitent between D7 and D10.

I've just done a reroll. Rushed, but sharing in case it helps others. Drupal 7.98

Thanks, pull request looks good to me!

I don't see anything else missing here, so we can go ahead with this?

I think the current MR was causing failures on some PHP/SQL combinations, see: https://git.drupalcode.org/project/drupal/-/pipelines/39895

@benjifisher proposed a solution which could fix this (in #37/#38), but if I see correctly, this was not implemented yet.