Undeclared static property

I am afraid it is more complicated than that.

I am updating the issue summary, including steps to reproduce the problem. When I follow those steps with an unpatched version of the module, I get a WSOD and the error you describe:

Error: Access to undeclared static property: Drupal\typogrify\SmartyPants::$method in Drupal\typogrify\SmartyPants::smartNumbers() (line 684 of /app/modules/contrib/typogrify/src/SmartyPants.php)

When I apply the patch from Comment #2, I no longer get the WSOD, but the filter is not wrapping the numbers and I see this in the log:

Warning: preg_replace_callback(): Requires argument 2, 'numberNarrownbsp', to be a valid callback in Drupal\typogrify\SmartyPants::smartNumbers() (line 684 of /app/modules/contrib/typogrify/src/SmartyPants.php)

So we need some variant of self::$method to identify the method. One option is to use [__CLASS__, $method]. When I do that, this is what I see in the log:

Warning: preg_replace_callback(): Compilation failed: invalid range in character class at offset 92 in Drupal\typogrify\SmartyPants::smartNumbers() (line 684 of /app/modules/contrib/typogrify/src/SmartyPants.php)

That means there is something wrong with this regular expression:

          $number_finder = '@(?:(&#\d{2,4};|&(#x[0-9a-fA-F]{2,4}|frac\d\d);)|' .
            '(\d{4}-\d\d-\d\d)|(\d\d\.\d\d\.\d{4})|' .
            '(0[ \d-/]+)|([+-]?\d+)([.,]\d+|))@';

I think the invalid range refers to this part: [ \d-/]. Do we know what that regular expression is supposed to do? Is it the same as what we have in the D7 version?

It may be that we are running into differences between PHP 5 and PHP 7, both in the use of self and in regular expressions. Or maybe we just garbled things when updating the code for Drupal 8.

I will start by adding some failing tests and correctly calling the methods. Maybe someone else can help with the regular expression.

I am leaving the status at NW in order to trigger the testbot, but I expect both the test-only patch and the "real" one to fail. The "real" one should generate the last warning quoted above.

A quick search on the PHP warning led me to preg_match(): Compilation failed: invalid range in character class at offset on StackOverflow. Based on the answer there, the invalid range warning may be due to a change in PHP 7.3. If so, then the patch in #4 may pass. I queued a second test using PHP 7.3, which is the version I am using locally.

If the patch in #4 does pass with PHP 7.1, then I may decide to commit it, since the problem with the regular expression is a separate issue.

That was quick! The testbot must be stuck at home and bored, just like everyone else.

Let me see if I can fix the regular expression. The automated test was not quite right, so I am attaching an updated test-only patch as well as one that I hope to pass and an interdiff.

Yay, the testbot approves!

I would like to get some real-world testing, and maybe even a code review, before committing this patch, so I will leave the status at NR. In particular, that change to the regular expression could use a second look.

I forgot to un-assign myself.

1. +++ b/src/SmartyPants.php
   @@ -680,8 +680,8 @@ class SmartyPants {
   -            '(0[ \d-/]+)|([+-]?\d+)([.,]\d+|))@';
   -          $t = preg_replace_callback($number_finder, self::$method, $t);
   +            '(0[ \d\-/]+)|([+-]?\d+)([.,]\d+|))@';
   +          $t = preg_replace_callback($number_finder, [__CLASS__, $method], $t);
            }
            $result .= $t;
          }
   @@ -831,7 +831,7 @@ class SmartyPants {
   
   @@ -831,7 +831,7 @@ class SmartyPants {
            $t = $cur_token[1];
            if (!$in_pre) {
              $abbr_finder = '/(?<=\s|)(\p{L}+\.)(\p{L}+\.)+(?=\s|)/u';
   -          $t = preg_replace_callback($abbr_finder, self::$replace_method, $t);
   +          $t = preg_replace_callback($abbr_finder, [__CLASS__, $replace_method], $t);
            }
   

   +1 for this syntax.

2. +++ b/tests/src/Kernel/TypogrifySmartyPantsTest.php
   @@ -18,10 +18,44 @@ class TypogrifySmartyPantsTest extends KernelTestBase {
   +  public function testTypogrify(array $extra_settings, $original, $processed) {
   

   Nit: you could typehint $original and $processed too.

3. +++ b/tests/src/Kernel/TypogrifySmartyPantsTest.php
   @@ -18,10 +18,44 @@ class TypogrifySmartyPantsTest extends KernelTestBase {
   +    $configuration = ['settings' => $extra_settings + $this->baseSettings];
   

   👍 for $extra_settings as the first operand for the + operator; that means that settings in there always win of those in $this→baseSettings (the second operand).

4. +++ b/tests/src/Kernel/TypogrifySmartyPantsTest.php
   @@ -18,10 +18,44 @@ class TypogrifySmartyPantsTest extends KernelTestBase {
   +    $this->assertEqual($result, $processed);
   

   🤔 Why assertEqual() and not assertSame()?

5. +++ b/tests/src/Kernel/TypogrifySmartyPantsTest.php
   @@ -31,29 +65,25 @@ HTML;
   +    // Original example compatibility-test.
   

   Übernit: s/compatibility-test/compatibility test/

6. +++ b/tests/src/Kernel/TypogrifySmartyPantsTest.php
   @@ -31,29 +65,25 @@ HTML;
   +      'original_typogrify_example' => [
   ...
   +      'test_wrap_abbreviations' => [
   ...
   +      'test_wrap_numbers' => [
   

   FYI @dataProvider test case labels are free form — they can contain spaces and even emoji.

Thanks for the review!

1. Thanks.

2. TIL that type hints are now called Type declarations in PHP 7. Checking that page, I see that string is an allowable declaration since PHP 7.0.0. I am a little worried that people might still be running an old version of Drupal 8 that allows PHP 5.6, but since this is in the test I think I will take the suggestion. Fixed.

3. Yes, I see this pattern a lot in Drupal.

4. No good reason: it was like that when I got here. I wish the API docs for KernelTestBase had links to the methods in PHPUnit\Framework\Assert, such as assertSame() ans assertEquals(). As in (2) I will not worry too much about BC for the tests. Fixed.

5. Fixed.

6. Thanks for the tip. I am not a fan of emojis, but some spaces will be an improvement. While I am at it, I will use the actual option name: wrap_abbr instead of wrap_abbreviations.

It turns out there was a reason to use assertEqual() instead of assertSame().

Since assertEqual() is deprecated (use assertEquals() instead) I will not go back. Instead, cast FilterProcessResult to string.

The attached patch makes two changes to the test:

1. Use assertEquals() instead of casting to string and then using assertSame().
2. Test the wrap_numbers option more thoroughly.

You can see the two changes separately in the issue fork.

The first change reverses the decision I made in #15.

Since the problem is caused by an un-escaped - character in a regular expression, I decided that the wrap_numbers test should include that character.

I did my best to review my own patch with "fresh eyes" before making the additional changes in #17. Fixed.