Text item sample generation fails if max length < 3

Thanks for the issue and patch. I found a few minor things.

1. +++ b/core/modules/text/src/Plugin/Field/FieldType/TextItemBase.php
   @@ -79,13 +79,14 @@ public static function generateSampleValue(FieldDefinitionInterface $field_defin
   +      'format' => \Drupal::config('filter.settings')->get('fallback_format'),
   

   Isn't this out of scope for this issue?

2. +++ b/core/modules/text/tests/src/Unit/TextItemTest.php
   @@ -0,0 +1,60 @@
   +    // Set up a configfactory for the filter settings fallback format.
   

   Nitpick: change "configfactory" to "config factory".

3. +++ b/core/modules/text/tests/src/Unit/TextItemTest.php
   @@ -0,0 +1,60 @@
   +    $configs = array(
   +      'filter.settings' => array(
   +        'fallback_format' => 'plain_text',
   +      ),
   +    );
   

   Switch from array() to [] syntax.

4. +++ b/core/modules/text/tests/src/Unit/TextItemTest.php
   @@ -0,0 +1,60 @@
   +    $configFactoryStub = $this->getConfigFactoryStub($configs);
   

   Since there is only one config, it could be passed in directly like many tests do, e.g.

   $config_factory = $this->getConfigFactoryStub([
         'filter.settings' => ['fallback_format' => 'plain_text'],
       ]);
   

5. +++ b/core/modules/text/tests/src/Unit/TextItemTest.php
   @@ -0,0 +1,60 @@
   +    $container->set('config.factory', $configFactoryStub);
   

   Other tests use $config_factory.

6. +++ b/core/modules/text/tests/src/Unit/TextItemTest.php
   @@ -0,0 +1,60 @@
   +    $this->assertEquals(1, Unicode::strlen($sample_value['value']), 'Sample value is correct');
   

   I don't see Unicode::strlen used anywhere in core code.

Rerolled the patch in #1. Also addressed all the points from #13 13.1, 13.2, 13.3, 13.4, 13.5, 13.6.

Thank you!

@Kristen Pol Rerolled the patch for drupal 9.2.x.
#13 Has been addressed with some additional changes use getMockBuilder function

Ignore #15 @Kristen Pol Rerolled the patch for drupal 9.2.x.
#13 Has been addressed with some additional changes use getMockBuilder function

There were some linting issues in #16 Please Ignore #16. I have fixed linting issues and Rerolled the patch for drupal 9.2.x.
#13 Has been addressed with some additional changes use getMockBuilder function

@pavnish I don't understand why you created your own new patch rather than starting with #14.

Tried to remove the errors from #14 patch. Please review.

fixed the test case form #22.

Updated the Tests and Made some changes to generateSampleValue() method.

Missed the Patch from previous commits , adding that.

I have provided a patch, please verify if it is working or not.

Updated coding standards in the tests.

Hard to follow all the patches but I looked at the last one.

1. The issues noted in #13 seem addressed

2. In #2, it says:

   The change of filter_fallback_format() is to decouple from a hard dependency on the filter module and improve testability.

   but that is out of scope of this issue so would need to be separated out.

   +++ b/core/modules/text/src/Plugin/Field/FieldType/TextItemBase.php
   @@ -70,7 +70,7 @@ public function onChange($property_name, $notify = TRUE) {
   +  public static function generateSampleValue(FieldDefinitionInterface $field_definition, $format = NULL) {
   

   +++ b/core/modules/text/src/Plugin/Field/FieldType/TextItemBase.php
   @@ -80,13 +80,14 @@ public static function generateSampleValue(FieldDefinitionInterface $field_defin
   +      'format' => isset($format) ? $format : filter_fallback_format(),
   

@Kristen pol, so you are telling us we need to remove #2 from this issue and create a new one for that?

As per #33's 2nd point, this patch removes the code which was requested to be added in #2

@Meenakshi_j. Welcome to Drupal. Thanks for adding an interdiff. When adding a patch it helps the next person working on the issue (whether reviewer or developer) if you add a comment explaining why you made the changes you made.

1. +++ b/core/modules/text/src/Plugin/Field/FieldType/TextItemBase.php
   @@ -80,13 +80,13 @@ public static function generateSampleValue(FieldDefinitionInterface $field_defin
   -      'format' => filter_fallback_format(),
   

   ? No, we can just remove the filter.

2. +++ b/core/modules/text/tests/src/Unit/TextItemTest.php
   @@ -0,0 +1,61 @@
   +  protected static $modules = ['filter'];
   

   Unit tests do not have a database. Therefor this does nothing.

3. +++ b/core/modules/text/tests/src/Unit/TextItemTest.php
   @@ -0,0 +1,61 @@
   +  public function testTextFieldSampleValue() {
   

   This should test more than a length of 1.

I started to review and then I realized that this needs will need a Kernel test so that the function, filter_fallback_format, is available. As pointed out in #33 changing that is out of scope.

Then, even thought it is late I decided to make a patch myself.

It may look like there is work to do in #30 but there isn't. I abandoned the review and wrote a new patch.

The patch in #40 is ready for review.

Thanks @quietone - can we get a test-only patch?

Thanks

Oops, you are right, I forgot.

Here it is.

Looks great, thanks

+++ b/core/modules/text/tests/src/Kernel/TextItemBaseTest.php
@@ -0,0 +1,39 @@
+
+      $this->assertEquals($string_length, strlen($sample_value['value']), "Sample value failed for string_length '$string_length'");
+    }

We don't normally use the fail condition for custom phpunit assertion messages, I think we'd be better of without this.

We don't normally use the fail condition for custom phpunit assertion messages, I think we'd be better of without this.

Without it we just get the line number, which is in the loop, so we don't know what failed

Tentatively putting back to rtbc

Actually no we don't we get the size

My 2c - make it a dataProvider instead of an array with a loop for more context :)

I must not be understanding the problem here.

Actually no we don't we get the size

Sorry, size of what?

For example, an assertion inside a loop needs a custom message so you know which loop iteration you were on when the assertion failed.

That is from #3131946: [policy] Remove PHPUnit assertion messages when possible, and standardize remaining messages . And I do not think that this needs to a new test environment for each source value.

Finally, lets see what happens with a failure.

When using a string length that fails, such as 0, the error message is

1) Drupal\Tests\text\Kernel\TextItemBaseTest::testTextFieldSampleValue
Sample value failed for string_length '0'
Failed asserting that 3414 matches expected 0.

That tells me exactly which of the input $string_lengths is causing the problem.

Setting to NR.

I think @larowlan is pointing that when the test only patch is applied, the error message is not the error of the PHPUnit. It is the PHP error of the currently bug. For me is okay to let it as it is because the assertion is in a foreach and if the $this->assertEquals fails, it will be displayed a good help message.
See:

1) Drupal\Tests\text\Kernel\TextItemBaseTest::testTextFieldSampleValue
ValueError: mt_rand(): Argument #2 ($max) must be greater than or equal to argument #1 ($min)

/app/drupal/core/modules/text/src/Plugin/Field/FieldType/TextItemBase.php:83
/app/drupal/core/modules/text/tests/src/Kernel/TextItemBaseTest.php:33
/app/drupal/vendor/phpunit/phpunit/src/Framework/TestResult.php:703

I agree that no data provider is required as it will need a new test environment for each source value so #49 is not necessary.

Just one thing:

// Test requesting strings of various lengths, ensuring that lengths 1, 2,
    // and 3 are tested.
    $string_lengths = [1, 2, 3, 4];

Do we need to explain why we are specifically testing 1, 2, and 3? Maybe adding a @see tag to the comment.

Just to be clear, I was agreeing with catch, the output will say 'failed asserting that 3414 matches expected 0' where 0 is the length, so you can discern which case failed.

But I do agree with Adam, that this would make sense as a data provider, however we're paying a setup cost for the kernel test each time, so the added test execution time might be a factor there

And I agree with Paulo, some context of why those lengths are significant would be great too

@paulocs, thanks for the explanation.

I didn't find anything in the issue that added TestItemBase to explain why the max_length is divided by three. #2320157: Generate placeholder content for Field types.

This patch adds a dataprovider and a comment in the test to explain the values chosen.

Oh bother, I did fix that but neglected to make a new patch.

Looks good.
Patch is working as expected and it addresses #52.

Committed 12aec14 and pushed to 9.3.x. Thanks!

I'll backport to 8.2.x once the freeze is over.

Cherry-picked to 9.2.x