Check use of "it's" in core to ensure proper use

The patch applies cleanly in all cases and even I guess all the correction for its and it's were made in the above patch. So changing the status to RTBC .

@prash_98, thank you for reviewing this issue!

The automated testing infrastructure tells us whether the patch applies, so we do not need people to review that. It is also not sufficient criteria for the issue to be marked "Reviewed and Tested by the Community".

When you do post a review, be sure to describe what you reviewed and how. This helps other reviewers understand why you considered the issue RTBC (and is considered for issue credit).

For this sort of issue, what would be helpful would be to follow the steps @vegantriathlete went through to create the patch, look at each instance of it's in core (for which @vegantriathlete helpfully provided a text file), and double-check that in each instance changing it's to its is correct based on the grammatical difference between them.

To review this patch myself, I did a few things:

1. Applied the patch locally and reviewed the diff with git diff --color-words, to ensure the only changes being made were replacing gramatically incorrect uses of "it's", that the replacement with "its" was correct in context, and that the change was being made in a comment only.
    
2. Looked over the list of files changed with git diff --stat to ensure that all were files it's allowable to change. (Certain files in core, like test fixtures and a couple of third-party libraries, aren't Drupal's to change.) The one file I hesitated about was web.config, because when we make changes to this file, it's a disruptive update for sites, because sites customize the file. So we generally announce changes to the file in the release notes, etc. In this case I decided that the disruption of having a comment be grammatically improved was not worth that release notes mention and that it would not be too problematic for this comment fix to cause divergence between their custom files and core's version. I did have to think about it, though, and I'm still a little hesitant, but I think we should go ahead with it.
    
3. Grepped core for all the remaining it's with the patch applied, to confirm for myself that these were grammatically correct ones that did not need to be fixed. I used:
   

   grep -rh -A 1 --color=auto --exclude-dir=vendor "\bit\'s" *
   

   And I've found at least one that was missed. :) Edit: Fixed the grep and the results that still were in vendor.

   // Create another field for testing with a long name. So it's storage name

   I don't have the filenames handy (the -h on the grep specifically excludes them so there's less text to read) but they can be found easily enough with grep. There might also be more, once I found one that was missed I skimmed a bit.

Thanks @vegantriathlete! Overall this is a great patch.

Also not such a quick fix. :)

Will keep in mind @xjm

Thanks @prash_98, good luck with your future contributions!

Looks like my --exclude-dir failed! Those misuses were in vendor. Checking again.

Okay, confirmed that the first one is still in our code and not vendor:

core/modules/field/tests/src/Kernel/EntityReference/Views/EntityReferenceRelationshipTest.php:    // Create another field for testing with a long name. So it's storage name

I'll fix my comment above. Edit: The trailing slash was causing it to not match the directory. The updated command above fixes it.

One more incorrect one from the updated grep:

      // If #access is an AccessResultInterface object, we must apply it's
      // cacheability metadata to the render array.

The patch can not be applied due to changes in the classes that extend Drupal\views\Plugin\views\style\StylePluginBase.

Rss.php is now inheriting documentation from StylePluginBase instead of duplicating it.

Re-rolled.

Patch applied clean using simplytest.me

I reviewed the patch for use of it's versus its and all instances are appropriate.

Screen shot provided. Marking RTBC.

Thanks for improving user facing copy.

Thanks for reviewing this.

The automated testing infrastructure tells us whether the patch applies, so we do not need people to review that. It is also not sufficient criteria for the issue to be marked "Reviewed and Tested by the Community". The frontpage screenshot is also not needed since it is not relevant to this issue, which is about fixing typos in codebase documentation.

To review this, we should also check the grep to make sure we catch all of them. See #9. This time I just removed the vendor and node_modules directories and then grepped what's left:

[ibnsina:core | Wed 04:20:10] $ rm -rf vendor
[ibnsina:core | Wed 04:20:44] $ rm -rf node_modules

There's also this one which may have crept in since the initial patch:

/* Prevent list item from expanding it's container. */
#drupal-off-canvas td ul.dropbutton li.edit {

And this one:

      // If #access is an AccessResultInterface object, we must apply it's
      // cacheability metadata to the render array.

I think those are the only two remaining that are incorrect, so if we can add this to the patch and re-review it fairly quickly, then this can probably finally land. :) Thanks everyone!

1. Corrected the first error (in two places).

2. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -218,7 +218,7 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
   -      // If #access is an AccessResultInterface object, we must apply it's
   +      // If #access is an AccessResultInterface object, we must apply its
   

   The second error you highlighted has already been corrected in the previous patch (and I can't find that text anywhere else).

@Jo Fitzgerald thanks your patch applied cleanly

I went through the whole list following @xjm's instructions once again and I couldn't spot any other wrong occurrence. Attaching the grep output so if we need to do this again, we just need to review the diff :)

I'd like to commit this soonish, but I'll wait some time to give @xjm a chance to review the latest patch.

@Venkatesh Rajan.J:

Thanks for testing the patch but, as pointed out by @xjm a couple of times, we don't need proof that the patch can be applied: testbots will mark the issue as needing work if it doesn't.

This way of "reviewing" patches is not seen as a good practice, as adding a screenshot of the patch applied provides no useful contribution, but tricks the credit system into believing a contribution was made. I'm sure you did this in good faith but, since it seems this is becoming a common practice, you should stop doing that and tell everyone you know in the community that is doing the same to stop as well. Feel free to point them to this comment.

@xjm spent quite some time documenting in #9 and #24 how to contribute to this issue in a constructive way. Please read those comments carefully and take mental notes for the next time! You are welcome to ask help in the IRC and Slack support channels, when in doubt about how to meaningfully contribute in an issue :)

The attachment for reals...

Last try

bot fluke

Bot, this patch is changing only comments :)

Thanks @plach for the grep results. Also uncrediting @Venkatesh Rajan.J as per the comment above.

1.     // Save the recipient handler and it's settings.
   

   Looks like we missed this one. :) All of the other "it's" besides that one are correct.

2. +++ b/web.config
   @@ -34,7 +34,7 @@
   -      uncomment the following rule to mitigate it's impact. To make this
   +      uncomment the following rule to mitigate its impact. To make this
   

   This one is correct, but this change will also mean that people's custom web.config files will have a conflict. So let's remove this one change from the patch (I don't think it's worth the disruption in this one case.)

covering point number 2.

Not sure about point number 1 like in which file this need to be done.

@xjm, could you specify which file needs to be addressed for the change in 1. to take place in #37 ?

I'm tempted to say that
// Save the recipient handler and it's settings.
does not appear in core, perhaps @plach has an additional theme or something in /vendor? Would you be able to confirm/deny that, @plach?

I have rolled back to the codebase as it was when @plach obtained the grep results and none of the occurances of "it's" are in the codebase after
/* Make li text transparent above icon so it's clickable. */
which is in themes/stable/css/core/dialog/off-canvas.dropbutton.css, hence why I suspect an additional theme (or perhaps something in vendor).

As a result I think this can be considered RTBC.

Yes, sorry, I forgot to remove one contrib module from my sandbox. That string comes from Simplenews.

Updating credits.

Bot fluke

Committed/pushed to 8.6.x and cherry-picked to 8.5.x. Thanks!