Fix spelling of "resultset" which should be "result set"/"result_set"

This starts with the easy bit, renaming variables, lets see what that breaks.....

The last submitted patch, 2: 3340621-2.patch, failed testing. View results →

Hi,

I am providing a patch for latest code of 11.x branch. Since the patch provided in #2 failed.

Steps performed:
1. Taken pull of latest code from 11.x
2. Search the code for 'resultset'
3. Replaced all appropriate appearances with 'result_set' (Excluded comments)

Providing patch and interdiff. Please review.

Thanks

Don't think the variables need to be updating, especially since some of them are mixing cases.

Also a dictionary rule should be added I think.

@urvashi_vora, thanks for updating the patch and commenting on how you modified the patch!

The wiki has details of the Naming Conventions → used by Drupal. That should help while working on this issue.

1. +++ b/core/lib/Drupal/Core/Database/StatementIteratorTrait.php
   @@ -18,7 +18,7 @@ trait StatementIteratorTrait {
   +  private mixed $result_setRow = NULL;
   

   This should be result_set_row

2. +++ b/core/lib/Drupal/Core/Database/StatementIteratorTrait.php
   @@ -26,7 +26,7 @@ trait StatementIteratorTrait {
   +  private int $result_setKey = -1;
   

   result_set_key

I searched for resultset in core/misc/cspell/dictionary.txt and didn't find it, so that doesn't need to change.

As for the errors they are from ./core/phpstan-baseline.neon. You need to go through that file and delete all the error messages that are being fixed in this issue.

@quiteone, thanks for the information. I will go through the Naming conventions and will try to provide a updated patch soon.

Hi,

I updated the patch as per the suggestions from #7 for mixed case variable. Uploading the updated patch.
Please review.

Also, @quietone, I didn't find any related errors in ./core/phpstan-baseline.neon, so haven't touched it.

Please guide me for the next steps that I have to do in the case we need to modify ./core/phpstan-baseline.neon.

Thanks.

Tried to fix CCF for #9.
Please review.

Thanks for updating the patch @_utsavsharma

Still appears to be instances

Hi, updated the patch, please review.

Thanks

Thanks for addressing those! But for comments I believe it should be result set vs result_set as that's a variable name

Got it, thanks @smustgrave. I will provide a patch soon

Hi, please review.

Thanks

Think this is good now!

Saving credits from original discussion.

1. +++ b/core/lib/Drupal/Core/Database/StatementIteratorTrait.php
   @@ -11,14 +11,14 @@
      private bool $isResultsetIterable = FALSE;
   ...
   -  private mixed $resultsetRow = NULL;
   +  private mixed $result_set_row = NULL;
   
   @@ -26,45 +26,45 @@ trait StatementIteratorTrait {
   -  private int $resultsetKey = -1;
   +  private int $result_set_key = -1;
   

   Coding standards says:

   Be consistent; do not mix camelCase and snake_case variable naming inside a file.

   Therefore I think these should become $resultSetRow and $resultSetKey.

2. +++ b/core/modules/block_content/tests/src/Kernel/Views/RevisionRelationshipsTest.php
   @@ -82,19 +82,19 @@ public function testBlockContentRevisionRelationship() {
   -    $this->assertIdenticalResultset($view, $resultset_id, $column_map);
   +    $this->assertIdenticalResultset($view, $result_set_id, $column_map);
   

   We could rename the method as well; method names are not case sensitive so this is backward compatible.

Hi @longwave,

I have updated the patch with your suggestions, but I have a query, if you could please help me here, it would be great.

I need more elaboration of (2) point from #19. Actually, when I was replacing in I found out that the actual method is rendering from "Drupal\views\Tests\ViewResultAssertionTrait::assertIdenticalResultset" which is also with small "s". And this method is being called at multiple places in multiple files, do we need to replace it everywhere including the "ViewResultAssertionTrait.php" file?

Thanks

I updated the patch provided in #20, because as per (1) of #19, I mistakenly left small "s" in the variable name. Please review the updated patch. And still I have the same query which I mentioned in #20.

Due to custom command fails, I again updated the patch, by changing the instances of updated variable.

Please review.

Thanks

Sending interdiff.txt

Think this is good for back to RTBC

I'm triaging RTBC issues → .

The IS comments that 'resultset' may not be in the Drupal dictionary. Indeed, it is not so 'resultset' is not considered a misspelling. So, what dictionary says it is valid? The results of yarn cspell track 'resultset' show that it is in the php dictionary. Since that is the case should we even be making this change? I very much wish I had done this in #7!

I also see that the question in #20 has not been answered. And related to that there are two instances where assertIdenticalResultset has been changed to assertIdenticalResultSet whereas all the others are unchanged.

But again, are we sure we want to do this since this is a valid spelling according to the php dictionary? I am inclined to say no.

I am setting this back to active for a rethink of this.

RE #25. Personally I would agree that it doesn't really make sense to go against the PHP dictionary. Also not sure this is worth such a large refactoring. I think it's ok to accept both, as even in the official PHP docs, both versions are being used.

I checked with @Lendude in Slack, as the author of this issue, and he replied that he recalls this was opened in response to a comment by xjm in another issue. He continued on to say that "it feels very disruptive for very little gain".

So, given that and that 'resultset' is in the php dictionary let's leave close this.

Thanks to everyone who spent time here to improve Drupal!