Make all $modules properties protected in tests

#2 Patch works for me on D8.6.15, php-7.3, mysql-5.7.

New patch now that #3061605: Convert simpletest tests to phpunit/browser tests has been committed.

Per #3065121: Menu changes from node form leak into live site when creating draft revision

#5 Updated \Drupal\Tests\token\Functional\TokenMenuUiContentModerationTest as well

I guess this issue is now postponed on #2822382: Make every $modules property protected on classes extending BrowserTestBase and KernelTestBase

Hi,

Thanks for the patch. I reviewed the patch, however, it could not be applied. Rerolling also did not seem to work. It seems something did not go well with the diff.

So I went ahead and made a new patch against the latest branch. I tested this on my local machine and it applied cleanly.

Please let me know If I missed anything.

First of all, I opened a merge request so it's easier to review and commit.

Then, I noticed one more occurrence of public visibility in \Drupal\Tests\token\Functional\TokenMenuUiContentModerationTest and was trying to fix it in the next commit. Turns out, #2822382: Make every $modules property protected on classes extending BrowserTestBase and KernelTestBase wasn't cherry-picked into 8.9.x, so it seems we have to leave TokenMenuUiContentModerationTest as-is until we support Drupal 8. I've reverted my commit, so now the merge request is equal to the last patch...

... which looks good to me, so marking as RTBC.

D8 is end-of-life on 2 November, so at this point in time instead of a partial solution that works for D8 (along with a necessary follow-up later on) I recommend waiting a few weeks then re-rolling this to change *all* instances at once when we don't have to worry about D8 anymore.

Agreed, I'm OK with breaking D8 compatibility now, we do then also need to update the .info.yml to clarify that we require 9.0.

I'll work on changing the left tests with occurrence of public visibility and I will also update the info.yml file as @Berdir suggested.

I hope it fits. I found no other occurrences of public visibility in the module and I changed the core_version_requirement key to require Drupal 9 or newer versions.

All of these protected static $modules declarations should use the same documentation comment:

  /**
   * {@inheritdoc}
   */

Right now, most of them don't, and what they use is a combination of various custom comments.

I found 3 public static $modules in the files:
/tests/src/Kernel/LanguageTest.php
/tests/src/Kernel/ValidateD6MigrationStateTest.php
/tests/src/Kernel/ValidateD7MigrationStateTest.php

I am not sure if those are legacy Simpletest tests?

Besides, no new CS where added, and all the $modules are documented as asked in #19.
But as we are working on these lines, may I ask for splitting these arrays when they go beyond 80 characters?

Thank you

Will work on that

You were right, this was not legacy test, so I made the $modules protected there too. I also followed you suggestion and split the array. Hope it fits.

The patch fixes the visibility for all uses of the $modules property, as well as fixing all the documentation comments for the $modules property, as well as fixing the array wrapping coding standards for this property.

Big +1 for this. I'm working on getting a contrib module of mine ready for D10 and this issue is a blocker. I think this warrants at least Normal priority.

Merged, thanks.