Problem/Motivation

The default value for this is -1. 0 means the same as -1. See https://www.php.net/manual/en/function.preg-split.php

Steps to reproduce

See https://3v4l.org/C81n1

Proposed resolution

Change NULL to -1

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#2 3239294-2.patch1.39 KBalexpott

Comments

alexpott created an issue. See original summary.

alexpott’s picture

Status: Active » Needs review
StatusFileSize
new1.39 KB

We use both 0 and -1 to mean limitless in core code. Going with -1 here since it is the default value and feels better to me to mean without limit.

daffie’s picture

Status: Needs review » Needs work

In #3238941: \Drupal\big_pipe\Render\BigPipe::splitHtmlOnPlaceholders() causes deprecation errors on PHP 8.1 we went with the default value of 0. What shall we use for the default value when 0 and -1 do the same thing? I can live with either one.

alexpott’s picture

Status: Needs work » Needs review

As stated in #2 I believe that choice made in #3238941: \Drupal\big_pipe\Render\BigPipe::splitHtmlOnPlaceholders() causes deprecation errors on PHP 8.1 was wrong - but core already has plenty of examples of both 0 and -1.

alexpott’s picture

Also #3238941: \Drupal\big_pipe\Render\BigPipe::splitHtmlOnPlaceholders() causes deprecation errors on PHP 8.1 should have included these fixes and a check of what default core was already using.

Here is the result of grep -ER "preg_split.*(0|-1)" ./core

./core/scripts/transliteration_data.php.txt:      foreach (preg_split('//u', $result, 0, PREG_SPLIT_NO_EMPTY) as $character) {
./core/lib/Drupal/Core/Mail/MailFormatHelper.php:    $split = preg_split('/<([^>]+?)>/', $string, -1, PREG_SPLIT_DELIM_CAPTURE);
./core/lib/Drupal/Component/Transliteration/PhpTransliteration.php:    foreach (preg_split('//u', $string, 0, PREG_SPLIT_NO_EMPTY) as $character) {
./core/lib/Drupal/Component/Transliteration/PhpTransliteration.php:    foreach (preg_split('//u', $string, 0, PREG_SPLIT_NO_EMPTY) as $character) {
./core/modules/locale/locale.module:  return implode('', preg_split('~(?<!\\\\)[\'"]\s*\+\s*[\'"]~s', substr($string, 1, -1)));
./core/modules/color/color.module:  $style = preg_split('/(#[0-9a-f]{6}|#[0-9a-f]{3})/i', $style, -1, PREG_SPLIT_DELIM_CAPTURE);
./core/modules/aggregator/aggregator.module:  return preg_split('/\s+|<|>/', \Drupal::config('aggregator.settings')->get('items.allowed_html'), -1, PREG_SPLIT_NO_EMPTY);
./core/modules/big_pipe/src/Render/BigPipe.php:      $result = preg_split($pattern, $html_string, 0, $flags);
./core/modules/search/src/SearchIndex.php:    $split = preg_split('/\s*<([^>]+?)>\s*/', $text, -1, PREG_SPLIT_DELIM_CAPTURE);
./core/modules/search/src/SearchQuery.php:      $words = $phrase ? [$words] : preg_split('/ /', $words, -1, PREG_SPLIT_NO_EMPTY);
./core/modules/filter/filter.module:    $chunks = preg_split('/(<.+?>)/is', $text, -1, PREG_SPLIT_DELIM_CAPTURE);
./core/modules/filter/filter.module:  $chunks = preg_split('@(<!--.*?-->|</?(?:pre|script|style|object|iframe|drupal-media|!--)[^>]*>)@i', $text, -1, PREG_SPLIT_DELIM_CAPTURE);
./core/modules/filter/src/Plugin/Filter/FilterHtml.php:        $attribute_values = preg_split('/\s+/', $attribute->value, -1, PREG_SPLIT_NO_EMPTY);
./core/modules/filter/src/Plugin/Filter/FilterHtml.php:          $allowed_attribute_values = preg_split('/\s+/', str_replace($star_protector, '*', $attribute->value), -1, PREG_SPLIT_NO_EMPTY);
./core/modules/views/src/Plugin/views/filter/StringFilter.php:      $words = $phrase ? [$words] : preg_split('/ /', $words, -1, PREG_SPLIT_NO_EMPTY);

There are 4 zeros and 10 instances of -1 ...

andypost’s picture

Status: Needs review » Reviewed & tested by the community

I think the patch fixing current issue so -1 is good common case, not sure it makes sense to unify this calls everywhere (could use follow-up)

  • catch committed b8561b8 on 9.3.x
    Issue #3239294 by alexpott, daffie, andypost: Passing NULL to the limit...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Not sure it's worth trying to unify 0 vs. -1, if we really want to, would probably lean towards -1 given it's the default.

Committed b8561b8 and pushed to 9.3.x. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.