Make every $modules property protected on classes extending BrowserTestBase and KernelTestBase

adding patch.

Updated one.

This patch makes every $modules property protected on tests classes not only in on classes extending BrowserTestBase and KernelTestBase.

I don't really know if this applies only to classes that extend BrowserTestBase and KernelTestBase, so I will submit this patch and work in other that only makes every $modules property protected only on classes extending BrowserTestBase and KernelTestBase.

This one only makes every $modules property protected only on classes extending BrowserTestBase and KernelTestBase.

Remaking patch after failing to test after some changes in the classes. This patch one only makes every $modules property protected only on classes extending BrowserTestBase and KernelTestBase.

Reroll patch in #2822382-7.patch

Hi @dawehner, I'm not sure of the scope of the patch, so I have made two possible patch 2822382-6.patch and 2822382-15.patch.
The first one "2822382-6.patch" makes every $modules property protected on tests classes not only in on classes extending BrowserTestBase and KernelTestBase.
The second patch 2822382-15.patch make every $modules property protected only in classes extending BrowserTestBase and KernelTestBase.

Could you please be more clear about wich patch have you tested and why do you thinks is not completed? Maybe a list of files where the change should be made and the current patch is not making it?

Thanks a lot in advance.

After applying the last patch for example core/modules/views/tests/src/FunctionalJavascript/Plugin/views/Handler/ContextualFilterTest.php is not changed, so we should also convert all JavascriptTestBase tests here.

Reroll patch in #2822382-6.patch, makes every $modules property protected on tests classes.

This going to introduce a lot of merge conflicts between 8.2.x and 8.3.x tests, so postponing it for some point after beta/rc (and probably after the webtestcase conversions on the assumption there'll be some conflicts there too).

I think the same regex will also fix the former WTB tests after they are converted to BTB, so we should easily be able to schedule this patch for 8.3.x RC once the big important WTB conversion has been done.

We are now in RC state so I guess this is the right time to reopen this.

Well spotted, these had slipped through my regex because the order of the visibility and the static modifier were switched.

Oh sorry this was already RTBC.

Yikes, caught myself just before clicking the dreditor button on this one. Massive cleanups like this are ideally rc targets. Tagging for now; haven't reviewed yet.

Okay, unsurprisingly, this thing does not apply. I'm even struggling to find a commit that it does apply to; I thought the CI job would include it (it does for branch tests) but it doesn't here. I finally found 5ce32930 on 8.5.x for the earlier patch in #26.

Untagging 'Novice'; there is no such thing as a 1 MB novice patch.

I strongly recommend putting the commit hash for the thing in the next reroll. I think we should also schedule this change so we can actually get it in. We'll want to have both 8.5.x and 8.4.x versions (they might not be the same) to keep the branches in sync. Ideally we would have done this in beta instead since it's an internal BC break (CR please?) but how about we do it tomorrow or so before the next RC? If we can draft a CR by then and generate a new patch. I'll check back by tomorrow.

Here's a quickly hacked-together porcelain script to check the patch:

git diff --staged --word-diff=porcelain > ~/porcelain.txt
cd ~
php porcelain.php

porcelain.php contains:

$file = file('./porcelain.txt');

foreach ($file as $key => $line) {
  if ((substr($line, 0, 5) == '--- a') || (substr($line, 0, 5) == '+++ b')) {
    // Verify that only test classes and test base classes have the change.
    if (!strpos(trim($line), "Test.php", -8)) {
      if (!strpos(trim($line), "TestBase.php", -12)) {
        print $line;
      }
    }
  }
  // Verify that the only removed words are the word "public", that
  // "protected" is replaced for each one, and that it's for the modules array.
  elseif ($line[0] == '-') {
    if ($line !== "-public\n") {
      print $line . $file[$key+1] . $file [$key+2];
    }
    if ($file[$key+1] !== "+protected\n") {
      print $line . $file[$key+1] . $file [$key+2];
    }
    if (strpos($file[$key+2], "  static \$modules") !== 0) {
      print $line . $file[$key+1] . $file [$key+2];

    }
  }
  // Verify that the only added words are the word "protected".
  elseif (($line[0] == '+') && ($line !== "+protected\n")) {
    print $line;
  }
}

Right now this shows a couple test files with nonstandard-ish names, and (since I used #26) what @dawehner pointed out in #27:

--- a/core/modules/comment/tests/src/Kernel/Migrate/d6/MigrateCommentVariableDisplayBase.php
+++ b/core/modules/comment/tests/src/Kernel/Migrate/d6/MigrateCommentVariableDisplayBase.php
--- a/core/modules/locale/tests/src/Functional/LocaleUpdateBase.php
+++ b/core/modules/locale/tests/src/Functional/LocaleUpdateBase.php
--- a/core/modules/node/tests/src/FunctionalJavascript/TestSettingSummariesContentType.php
+++ b/core/modules/node/tests/src/FunctionalJavascript/TestSettingSummariesContentType.php
-public
  $modules = ['block'];
~
-public
  $modules = ['block'];
~
-public
  $modules = ['system'];
~
-public
  $modules = ['system'];
~

Created draft CR: https://www.drupal.org/node/2909426

Here are the updated patches. The 8.4.x patch is against commit e77fec14, 8.5.x is against aa7c4ab0.

Mind that this is pretty hard work to roll, it's not a matter of simply running a regex like suggested in the issue summary.

I dug in the log files on Jenkins to find the error:

14:32:58 Fatal error: Uncaught Error: Call to protected method Drupal\simpletest\KernelTestBase::containerBuild() from context 'Drupal\simpletest\TestServiceProvider' in /var/www/html/core/modules/simpletest/src/TestServiceProvider.php:22
14:32:58 Stack trace:
14:32:58 #0 /var/www/html/core/lib/Drupal/Core/DrupalKernel.php(1290): Drupal\simpletest\TestServiceProvider->register(Object(Drupal\Core\DependencyInjection\ContainerBuilder))
14:32:58 #1 /var/www/html/core/lib/Drupal/Core/DrupalKernel.php(884): Drupal\Core\DrupalKernel->compileContainer()
14:32:58 #2 /var/www/html/core/lib/Drupal/Core/DrupalKernel.php(466): Drupal\Core\DrupalKernel->initializeContainer()
14:32:58 #3 /var/www/html/core/modules/simpletest/src/KernelTestBase.php(187): Drupal\Core\DrupalKernel->boot()
14:32:58 #4 /var/www/html/core/modules/simpletest/src/Tests/KernelTestBaseTest.php(46): Drupal\simpletest\KernelTestBase->setUp()
14:32:58 #5 /var/www/html/core/modules/simpletest/src/TestBase.php(950): Drupal\simpletest\Tests\KernelTestBaseTest->setUp()
14:32:58 #6 /var/www/html/core/modules/simpletest/src/Tests/KernelTestBaseTest.p in /var/www/html/core/modules/simpletest/src/TestServiceProvider.php on line 22
14:32:58 FATAL Drupal\simpletest\Tests\KernelTestBaseTest: test runner returned a non-zero error code (255).

Unfortunately I don't have time to work on this until Drupalcon.

Screw my lack of time, let's keep momentum on this, here is the updated patch!!

This has slipped trough and wasn't committed on the dates mentioned in the title, should we target this for the next rc phase?

Patch from #39 no longer applies. Copying over (and updating) patch from duplicate issue #2918296: Change all instances of $modules to protected.

Whoops we missed our window. Disruptions like this should happen during beta or RC. The next beta will be released Feb. 7. Thanks!

Shouldn't the date in the title be February 7, 2018?

That works. Note that RC is only two weeks this time around, rather than four. So we should prepare this during the beta and plan to have it done at least a week before the minor release if we want to get it in this time around.

We're now inside the window to be able to do this. I executed the script in the description, on my mac this left a bunch of files *.php-e. I used git clean -fd core to remove all of those.

The result is attached here. No interdiff as this is just the result of that script.

Rerolled

Fatal error: Access level $modules must be public
Fixed access level for base classes and classes that extends that base classes.

#2946419: Entity: Convert system functional tests to phpunit

Needs new time window update.
BTW #2946425: Correcting the tests that lie in BTB but using WTB fixed

Per #45 and #49, it seems this should be done during beta or RC. 8.6.0-alpha1 is nearly upon us, and 8.6.0-beta1 is due the Week of July 29, 2018. Based on previous windows, it seems like this should be done between July 29, 2018 and August 20, 2018 (from beta phase begin to one week after rc phase begins), so I've adjusted the issue title accordingly. If that window is wrong, please feel free to correct it.

@xjm and @dawehner : What needs to happen to get this one done?

@RytoEX it will need to be redone from scratch.

Note that this can best be coordinated with a core committer to minimize the turnover time. There is always a possibility that a new commit to core touches one of the tests being patched, or introduces a new public property, and the patch will need to be rerolled.

Sure, but before that window, we should be sure that tests will pass after applying.
Reroll patch using the script.

I had tested different script for patch generating:
- from Issue Summary:

find . -path './.git' -prune -o -not \( -name '*.js' \) -type f -path '*/tests/*' -exec sed -i -e 's/public static $modules/protected static $modules/g' {} \;

- and little bit changed:

find . -path './.git' -prune -o -name '*.php' -type f -path '*/tests/*' -exec sed -i -e 's/public static $modules/protected static $modules/g' {} \;

So, the only difference between the scripts is that script from IS also update non-related files:
/core/modules/migrate_drupal_ui/tests/src/Functional/d6/files/core/modules/simpletest/files/image-1.png
/core/modules/migrate_drupal_ui/tests/src/Functional/d6/files/core/modules/simpletest/files/image-test.png
/core/modules/system/tests/fixtures/update/drupal-8.0.0-rc1-filled.standard.entity_test_update.php.gz
/core/modules/system/tests/fixtures/update/drupal-8.0.0-rc1-filled.standard.entity_test_update_mul.php.gz
/core/modules/system/tests/fixtures/update/drupal-8.0.0-rc1-filled.standard.entity_test_update_mul_rev.php.gz
/core/modules/system/tests/fixtures/update/drupal-8.2.0.bare.standard_with_entity_test_revlog_enabled.php.gz
/core/modules/system/tests/fixtures/update/drupal-8.4.0.bare.standard.php.gz
/core/modules/system/tests/fixtures/update/drupal-8.bare.standard.php.gz
/core/modules/system/tests/fixtures/update/drupal-8.filled.standard.php.gz
/core/tests/fixtures/config_install/testing_config_install.tar.gz

Here is the regenerated patch

Looks like the updated script works as expected.

Thanks for starting the work on this! I had a quick look. I have two remarks so far:

1. The patch has been rolled against 8.7.x but it should actually be against 8.6.x. The core version for this issue was set to 8.7.x by the issue queue bot. I've corrected it.
2. Doing a quick grep of test files in the core/ folder came up with a large number of occurrences of the public property that were not caught by the script. I've noticed this before when I worked on this issue (ref comment #2822382-37: Make every $modules property protected on classes extending BrowserTestBase and KernelTestBase). Most of these (but not all) are old Simpletest tests that are in a different folder structure and are not found by the `*/tests/*` filter in the command.

Here is the list of files that came up when I did the following grep. This list is manually curated since the grep yields many false positives.

$ grep -nr modules core/ | grep public | grep -i test

List of files that still contain the public property:

core/modules/quickedit/src/Tests/QuickEditLoadingTest.php
core/modules/quickedit/src/Tests/QuickEditAutocompleteTermTest.php
core/modules/migrate_drupal_ui/src/Tests/MigrateUpgradeTestBase.php
core/modules/system/src/Tests/Module/ModuleTestBase.php
core/modules/system/src/Tests/Routing/RouterTest.php
core/modules/system/src/Tests/Routing/DestinationTest.php
core/modules/system/src/Tests/Pager/PagerTest.php
core/modules/system/src/Tests/Image/ToolkitTestBase.php
core/modules/system/src/Tests/Ajax/AjaxTestBase.php
core/modules/system/src/Tests/Ajax/DialogTest.php
core/modules/system/src/Tests/Ajax/MultiFormTest.php
core/modules/system/src/Tests/Form/TriggeringElementTest.php
core/modules/system/src/Tests/Form/RebuildTest.php
core/modules/system/src/Tests/Form/StorageTest.php
core/modules/system/src/Tests/Form/ElementsTableSelectTest.php
core/modules/system/src/Tests/Theme/FunctionsTest.php
core/modules/system/src/Tests/Theme/ThemeSuggestionsAlterTest.php
core/modules/system/src/Tests/Theme/ThemeTest.php
core/modules/system/src/Tests/Theme/TwigTransTest.php
core/modules/system/src/Tests/Theme/TwigDebugMarkupTest.php
core/modules/system/src/Tests/Theme/TwigNamespaceTest.php
core/modules/system/src/Tests/Theme/HtmlAttributesTest.php
core/modules/system/src/Tests/Theme/EngineTwigTest.php
core/modules/system/src/Tests/Theme/TwigRawTest.php
core/modules/system/src/Tests/Theme/TwigFilterTest.php
core/modules/system/src/Tests/System/UncaughtExceptionTest.php
core/modules/system/src/Tests/System/ErrorHandlerTest.php
core/modules/system/src/Tests/Database/DatabaseWebTestBase.php
core/modules/system/src/Tests/Condition/ConditionFormTest.php
core/modules/system/src/Tests/Render/HtmlResponseAttachmentsTest.php
core/modules/system/src/Tests/Render/UrlBubbleableMetadataBubblingTest.php
core/modules/system/src/Tests/Session/SessionAuthenticationTest.php
core/modules/system/src/Tests/Session/StackSessionHandlerIntegrationTest.php
core/modules/system/src/Tests/Session/SessionTest.php
core/modules/system/src/Tests/Session/SessionHttpsTest.php
core/modules/system/src/Tests/Entity/EntityCacheTagsTestBase.php
core/modules/system/src/Tests/Entity/EntityUnitTestBase.php
core/modules/system/tests/src/Functional/Theme/ThemeTokenTest.php
core/modules/system/tests/src/Kernel/PathHooksTest.php
core/modules/link/src/Tests/Views/LinkViewsTokensTest.php
core/modules/user/src/Tests/UserCreateTest.php
core/modules/user/src/Tests/UserBlocksTest.php
core/modules/user/src/Tests/UserAdminLanguageTest.php
core/modules/user/src/Tests/RestRegisterUserTest.php
core/modules/user/src/Tests/UserPasswordResetTest.php
core/modules/user/src/Tests/UserAdminSettingsFormTest.php
core/modules/user/src/Tests/UserRegistrationTest.php
core/modules/block/src/Tests/BlockTestBase.php
core/modules/aggregator/src/Tests/AggregatorTestBase.php
core/modules/rest/src/Tests/RESTTestBase.php
core/modules/tracker/src/Tests/Views/TrackerTestBase.php
core/modules/shortcut/src/Tests/ShortcutTestBase.php
core/modules/field_ui/src/Tests/ManageDisplayTest.php
core/modules/field_ui/src/Tests/ManageFieldsTest.php
core/modules/field_ui/src/Tests/FieldUIDeleteTest.php
core/modules/file/src/Tests/FileFieldWidgetTest.php
core/modules/file/src/Tests/FileManagedTestBase.php
core/modules/file/src/Tests/FileFieldTestBase.php
core/modules/file/tests/src/Kernel/Migrate/d6/MigrateFileTest.php
core/modules/views_ui/src/Tests/UITestBase.php
core/modules/taxonomy/src/Tests/TaxonomyTermIndentationTest.php
core/modules/taxonomy/src/Tests/TermTest.php
core/modules/taxonomy/src/Tests/TermTranslationTest.php
core/modules/taxonomy/src/Tests/TaxonomyTestBase.php
core/modules/taxonomy/src/Tests/RssTest.php
core/modules/datetime/src/Tests/DateTestBase.php
core/modules/datetime/src/Tests/Views/DateTimeHandlerTestBase.php
core/modules/block_content/src/Tests/BlockContentTestBase.php
core/modules/block_content/src/Tests/Views/BlockContentTestBase.php
core/modules/simpletest/src/Tests/KernelTestBaseTest.php
core/modules/simpletest/src/Tests/SimpleTestTest.php
core/modules/simpletest/src/Tests/MissingCheckedRequirementsTest.php
core/modules/simpletest/src/Tests/BrowserTest.php
core/modules/simpletest/src/Tests/UiPhpUnitOutputTest.php
core/modules/simpletest/src/Tests/SimpleTestInstallBatchTest.php
core/modules/simpletest/src/Tests/SimpleTestErrorCollectorTest.php
core/modules/simpletest/src/Tests/BrokenSetUpTest.php
core/modules/simpletest/src/Tests/InstallationProfileModuleTestsTest.php
core/modules/simpletest/src/Tests/SimpleTestBrowserTest.php
core/modules/simpletest/src/KernelTestBase.php
core/modules/menu_ui/src/Tests/MenuWebTestBase.php
core/modules/responsive_image/src/Tests/ResponsiveImageFieldUiTest.php
core/modules/responsive_image/src/Tests/ResponsiveImageFieldDisplayTest.php
core/modules/statistics/src/Tests/StatisticsTestBase.php
core/modules/statistics/src/Tests/Views/IntegrationTest.php
core/modules/content_translation/src/Tests/ContentTranslationTestBase.php
core/modules/node/src/Tests/NodeRevisionsTest.php
core/modules/node/src/Tests/PagePreviewTest.php
core/modules/node/src/Tests/NodeTestBase.php
core/modules/node/src/Tests/NodeTypeTest.php
core/modules/node/src/Tests/Views/NodeTestBase.php
core/modules/node/src/Tests/Views/NodeContextualLinksTest.php
core/modules/image/src/Tests/ImageFieldTestBase.php
core/modules/path/src/Tests/PathTestBase.php
core/modules/comment/src/Tests/CommentTestBase.php
core/modules/comment/src/Tests/Views/CommentTestBase.php
core/modules/field/src/Tests/FormTest.php
core/modules/field/src/Tests/FieldDefaultValueCallbackTest.php
core/modules/field/src/Tests/reEnableModuleFieldTest.php
core/modules/field/src/Tests/EntityReference/EntityReferenceFileUploadTest.php
core/modules/field/src/Tests/EntityReference/EntityReferenceAdminTest.php
core/modules/field/src/Tests/Email/EmailFieldTest.php
core/modules/field/src/Tests/Views/HandlerFieldFieldTest.php
core/modules/field/src/Tests/Views/FieldUITest.php
core/modules/field/src/Tests/Views/FieldTestBase.php
core/modules/field/src/Tests/String/StringFieldTest.php
core/modules/field/src/Tests/FieldImportDeleteUninstallUiTest.php
core/modules/search/src/Tests/SearchTestBase.php
core/modules/views/src/Tests/FieldApiDataTest.php
core/modules/views/src/Tests/Plugin/StyleOpmlTest.php
core/modules/views/src/Tests/Plugin/DisplayFeedTest.php
core/modules/views/src/Tests/ViewTestBase.php
core/modules/views/src/Tests/Wizard/WizardTestBase.php
core/modules/views/src/Tests/ViewKernelTestBase.php

@pfrenssen these remaining files all appear to be unconverted webtests

edit:Ah you pointed that out already :)

It seems the missed occurrences pointed out in #69 are due to the case-sensitivity of the find command used. I've adapted the command to check for paths containing '*/tests/*', '*/Tests/*', and '*/simpletest/*', and to replace both "public static $modules" and "static public $modules" with "protected static $modules". I then ran a series of checks on the resulting patch file to (hopefully) ensure that no unintended changes were introduced and check the resulting codebase to (hopefully) ensure that there were no more instances of "public static $modules" or "static public $modules". I did this for both 8.6.x and 8.7.x Patches and interdiff with #67 (against 8.7.x) are attached.

Per the request in comment #34, here are the commit hashes the patch was based on for each branch:

• 8.6.x: commit 09530d7e
• 8.7.x: commit 045fcb5d

Here's hoping I did this right and all tests come back green.

All tests passed. Please feel free to check that I made all the necessary changes. Although this will probably need re-checked before committing, it seems fairly trivial to script the patch creation (if I got this right). Forgot to set to "Needs review" earlier, so setting that now.

There is the script based on the latest comments

find . -path './.git' -prune -o -name '*.php' -type f \( -path '*/tests/*' -or -path '*/src/Tests/*' -or -path '*/simpletest/*' \) -exec sed -i -e 's/public static $modules/protected static $modules/g' {} \;
find . -path './.git' -prune -o -name '*.php' -type f \( -path '*/tests/*' -or -path '*/src/Tests/*' -or -path '*/simpletest/*' \) -exec sed -i -e 's/static public $modules/protected static $modules/g' {} \;

find . -path './.git' -prune -o -name '*.php' -type f \( -path '*/tests/*' -or -path '*/src/Tests/*' -or -path '*/simpletest/*' \) -exec sed -i -e 's/public static $modules/protected static $modules/g; s/static public $modules/protected static $modules/g' {} \;
Optimized script and regenerated patches.
And still, we need IS update.

interdiff files are incorrect as they were generated on restructured revisions
the manual search showed that there are no one unprocessed "public static $modules" lines.

wrong patch for 8.7.x in #74

@voleger
For reference, the script I used to do the patch generation was nearly identical to yours. I'd initially done multiple passes, but eventually switched to this:

# find PHP files in "tests" or "Tests" or "simpletest" directories
# replace 'public static $modules' with 'protected static $modules'
# replace 'static public $modules' with 'protected static $modules'
find . -path './.git' -prune -o -name '*.php' -type f \( -path '*/tests/*' -o -path '*/Tests/*' -o -path '*/simpletest/*' \) -exec sed -i -e 's/public static $modules/protected static $modules/g' -e 's/static public $modules/protected static $modules/g' {} \;

That takes about 3.5 minutes on my machine on a clean 8.6.x or 8.7.x branch. To check the resulting patch file, I run this script from within the Drupal directory, specifying the patch file as the only argument:

#!/bin/bash

# Check if any lines were unintentionally removed
grep -P -i '^\-  (?!public static \$modules)' "$1" | grep -P -i '^\-  (?!static public \$modules)'

# Check if any lines were unintentionally added
grep -P -i '^\+  (?!protected static \$modules)' "$1"

# Check if any non-test files were modified
grep -P '^\-\-\- a\/' "$1" | grep -P -i '^((?!tests).)*$' | grep -P -i '^((?!simpletest).)*$'
grep -P '^\+\+\+ b\/' "$1" | grep -P -i '^((?!tests).)*$' | grep -P -i '^((?!simpletest).)*$'

# Check that all instances of 'public static $modules' and 'static public $modules' were actually replaced
grep -r -F -i '$modules = ' . | grep -F -i 'public'

If it outputs nothing, then the patch (and code base) are fine. That takes less than a minute.

As an aside, it looks like some commits from earlier today have added new files that need to be processed by these scripts before commit. Rather than continuously reroll, let's get someone's attention on this to make sure our scripts are actually making the right changes, so that the patches can be rerolled closer to when a committer can work on this.

Actually, the goal of this issue is to prepare the script that will correctly generate the changes to create a proper patch before the nearest time window.
So, I agree with @RytoEX that we should focus on the script at the current point.

I can confirm that the current version of the script from IS cover all required replacements.

I thought the goal of this is to produce a patch to be committed? The scripts just facilitate that.

Why does the script in the IS use '*/src/Tests/*' instead of just '*/Tests/*'?

Setting back to Needs Review.

because this is the part of namespace of WTB.

this issue has tag "rc target", so that's means that patch can be applied only during RC window.
please follow #33, #49 to understand why the time window is so important.

Updated IS

Namespaces use backslashes. Unix paths use forward slashes, and the path argument for find uses paths, not file contents. I'm still confused on this point.

I have read the issue, and I do understand why the time window is important (I even proposed the current time window). My point here was that eventually a patch needs to be committed, regardless of how it is produced. A script is just probably the best way of producing a patch for this particular issue.

In any case, as I said before, I think what would help most here is a committer confirming that the patch is on the right track, and confirming the window for action, since I'm not 100% confident in the window that I provided (it is based on previous windows).

It should be done after the first RC release is tagged. According to https://www.drupal.org/core/release-cycle-overview, this will be august 13.

In any case, I think it's a good idea to have a script that produces the correct output, because hopefully a few more test conversions will land before the first RC.

reminder
we are in the required time window

up

Upload correct patch for 8.6.x

Looks good to me:

$ git reset --hard
HEAD is now at cfeae61b70 Issue #2991080 by alexpott: Remove skipped deprecations no longer triggered by tests
$ grep 'public static $modules' -R . | wc -l
    1880
$ curl https://www.drupal.org/files/issues/2018-08-15/2822382-87-8.7.x.patch | git apply --index
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 1122k  100 1122k    0     0   502k      0  0:00:02  0:00:02 --:--:--  502k
$ grep 'public static $modules' -R . | wc -l
       0

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

I had to revert this in order to revert #2957425: Allow the inline creation of non-reusable Custom Blocks in the layout builder for a critical regression. Sorry everyone! That said though this should not have been committed during the RC commit freeze; I think @borisson_ misunderstood the dates in the chart a little. :) This is the week of the commit freeze for the RC. The RC phase starts only after we release the RC and freeze for about 12-24h more to ensure there aren't any hotfixes we need.

We haven't released the RC yet, so this should wait another day before we reroll it. Retitling to the correct timeframe for an RC target. :) Thanks!

Also the commit freeze for release starts before release. :)

Could we update the IS with why too? Seems strange such a large patch has no reasoning which I assume is just lost offline discussions.

@neclimdul, the change record explains it pretty well: https://www.drupal.org/node/2909426

I've added the CR's text to the IS.

Are you thinking that the patch scope is too disruptive this close to release? Maybe we should consider just making it a beta target instead, since the length of the RC phase has been halved since the issue was originally scheduled last year. E.g. we made #2981652: [Aug. 9 2018] Format core JavaScript using recently add Prettier a beta target instead of an RC target based on this. I'll ask for another committer's opinion.

The CR will need to be updated as it mentions Drupal 8.4.0.

I think patches like this are OK during the beta too, the dates on the issue title made me thing 'end of beta', not 'during rc'.

OK then this is no longer postponed I guess?

I really would like this to get in now that we are so close :)

@xjm which update is needed for the CR? I reviewed it but it still seems OK to me. I can make changes if needed.

Still postponed on the RC being released (it hasn't yet) +24h (so at least Saturday). :)

@pfrenssen, the change record simply needs to have version numbers corrected. It says things like "prior to 8.4.0", but the change is actually going to be in 8.6.0 instead.

@catch and I discussed this and agreed that with the current release cycle these issues should be beta targets (as the prettier JS conversion was); however, since this one is close, we are okay with it going in early during RC. Adjusting the time window based on that. If somehow we don't get to it this cycle it'll be a beta target.

Also, for changes of this magnitude that disrupt many patches, we usually do a g.d.o/core announcement a few days in advance letting contributors know the change will be made. See https://groups.drupal.org/node/534209 for an example. Can someone draft this announcement? We could target Tuesday, August 21 if the above contributors are available then to reroll it. That would give contributors enough lead time to know when their patches need a reroll, and be guaranteed to be after the RC freeze.

In the future I'd suggest tagging for release manager review again just to confirm the details of the cycle, and ping one of us if it's time-sensitive. :)

Thanks everyone!

Updated the change record. Here is a proposed announcement:

Module list in tests will become a protected property - many patches will require rerolls

The core team is planning to commit a very large patch to the 8.6.x branch now that it is about to enter RC phase. This patch touches a large number of existing tests, and there is a high probability that it will cause conflicts in patches that are currently being worked on in the issue queue. The affected patches will need to be rerolled.

What is the patch about?

Up to now the visibility of the $modules property in tests was not consistent in core. Tests were marking it either as a public or a protected property. This was causing needless discussion and lost time for both developers and reviewers. The patch now standardizes on making it protected. For more information see the issue and the change record.

Why is this being committed now?

Once a new minor core release enters RC phase this is typically a period of reduced activity since only bug fixes that affect the upcoming release will be accepted. This is the moment that large patches will cause the least possible disruption to the ongoing work in the issue queue.

Thanks @pfrenssen, that looks pretty good! One thing we need to know before posting is, what date this coming week? We should have a day that a patch author, reviewer, and committer are all available to land it.

One other question I have before we go ahead is: How do we plan to keep this from regressing? E.g., is this something we could add a PHPCS rule for? There are probably a lot of patches in the queue with new tests that have the property as public. It's a small difference that is easily missed in patch reviews.

Finally, do we have a count of how many tests have public vs. protected? I'm assuming a lot more are (correctly) protected, but it would be good to document this.

Updated announcement text:

Many core patches will require rerolls following [date] -- Module list in tests will become a protected property

A large patch will be committed to the 8.6.x branch on [date] now that it is in its release candidate phase. This patch touches a large number of existing tests, and it will cause conflicts in patches that are currently being worked on in the issue queue. The affected patches will need to be rerolled.

What is the patch about?

Up to now the visibility of the $modules property in tests was not consistent in core. It was intended to be a protected property, but [N]% of core tests mark it as a public property. Confusion about the standard extended review process discussion and took extra time for both developers and reviewers. The patch now standardizes on making it protected. For more information see the issue and the change record.

Why is this being committed now?

Once a new minor core release enters its beta and release candidate phases, only bug fixes that affect the upcoming release will be accepted. This means there is the least divergence between the upcoming release branch and the new development branch, no interference with key deadlines for important changes, and reduced issue activity. This is the phase that large coding standards patches will cause the least possible disruption to the ongoing work in the issue queue. In the future, all such patches will be targeted for the beta phase.

Thanks!

Wasn't a big deal and don't have feelings about when its committed. Just came looking for more explanation than was in the commit but the IS and discussion didn't provide it. Thanks!

How do we plan to keep this from regressing? E.g., is this something we could add a PHPCS rule for? There are probably a lot of patches in the queue with new tests that have the property as public. It's a small difference that is easily missed in patch reviews.

I don't think a PHPCS rule is the right approach for this. This is an OO convention, not a coding standards violation. It would also set a dangerous precedent that would open the floodgates to request rules for every public / protected property in core.

We _could_ add some lines in the test base classes to check the visibility of the property using reflection. But I'm conflicted on this, since for consistency we should then also do the same for all other properties used in test classes. I would be fine with making an exception for this case though. We don't often change visibility of a property.

I think the best approach would maybe be to just keep an eye out for it during reviews. I think this counts for all inherited properties, checking if the visibility matches the parent hierarchy is a part of the due diligence when reviewing patches. The main problem was that most tests were using the (incorrect) public visibility and this was being copy/pasted into new tests. Also a number of popular base classes had set the visibility to public, and then it becomes illegal in PHP to make it protected again in classes that inherit it. I think this was the main cause of the wide proliferation of the public property.

Finally, do we have a count of how many tests have public vs. protected? I'm assuming a lot more are (correctly) protected, but it would be good to document this.

I did a quick rough check and the result is pretty surprising. It seems that the large majority of tests have deviated from the protected visibility that is defined in the test base classes:

$ grep -r 'protected static $modules' . | wc -l
71
$ grep -r 'public static $modules' . | wc -l
1957

Setting this back to active since we are now in the approved time window.

Same review as in #90 yields the same result as in #90. Also skimming the patch reveals nothing unexpected. -> RTBC

Thanks!

The patches from #103 also pass my tests from #78 which attempts to validate the changes in the patch files and checks the Drupal source for 'public static $modules' and 'static public $modules'. LGTM

rerolled
6 days remaining

2 days remaining

The last day of the available window

I hope we still have the time to get it in.

Looks like we're going to have to do this for the RC-window of 8.7.x.

A very very big thank you to @voleger for all the work to try to get done in time!! Too bad we didn't make the window.

We'll give it another shot in January.

What's the next date/time to make this land?

Basing on #98, I would say this is the next available time.

The release date of 8.7.0 is May 1st according to https://www.drupal.org/core/release-cycle-overview

I take this is still a 8.7.x task, so far.

Updated title and issue summary with the next available window of opportunity.

As for 8.9 beta

Maybe we are able to perform changes in the different time window? See #3107732: Add return typehints to setUp/tearDown methods in concrete test classes

Given the new dates, isn't this a task for the 9.0.x branch?

Agree that what we need is the patch, not just the script helping generate the patch.

First time to use the script in IS, it generated me .e files. Instead of using the script. I made the patch with the find and replace feature of PHPStorm

1. Searched static public $modules with file mask *.*, 0 match.
2. Searched public static $modules = [ with file mask *Test.php, and replaced with protected static $modules = [
3. Searched public static $modules = [ with file mask *TestBase.php, and replaced with protected static $modules = [
4. Searched public static $modules = [ with file mask *.php, and replaced with protected static $modules = [ (6 files. They do not follow the naming conversation)
5. Searched public static $modules = with file mask *.php, and replaced with protected static $modules = (Due to \Drupal\Tests\field\Kernel\Migrate\d6\MigrateFieldInstanceOptionTranslationTest::$modules, its opening [ is at the next line)
6. Searched public static $modules with file mask *.*, 0 match.

To verify changes only made to Kernel tests, Functional tests and FunctionalJavascript tests.

1. Get the list of changed files saving as foo.txt by using git status.
2. Replaced /Functional/, /Kernel/, /FunctionalJavascript/, /FunctionalTests/, /KernelTests/ and /FunctionalJavascriptTests/ with /***/ (or any other string unique) one by one
3. Counting the total of /***/ (by searching again, it should show the results found, used Atom), it should match the total lines of changed files - 1.
4. -1 because of core/modules/system/src/Tests/Ajax/AjaxTestBase.php does not match any of the above.

core/modules/jsonapi/tests/src/Functional/MenuLinkContentTest.php is recognized as a binary file, that's why the patch failed to apply in #126, a new issue filed.

Used git diff --text generated the patch this time

A patch for 9.0.x

@kiamlaluno you are correct. But changes has to go into the 9.0.x and 9.1.x as well.

@jungle thanks for reroll and patches for both branches. Looks good for me. But I'll keep needs review till expected time window. Potentially new test may appear at that period of time.

April 13 - April 17 is the closest available time window to commit this patch?

Correct the title change if I'm wrong, but the issue #3107732: Add return typehints to setUp/tearDown methods in concrete test classes set the time window for the similar reasons as I understand.

Both patches needs a reroll.

Rerolled, thanks @daffie for reviewing!

@jungle the patches can be applied to either branch so there's no real difference between them. In such cases a single patch is preferred because there is less for reviewers to think about. You can queue a patch to test on other branches. I'll queue the 9.1.x against 9.0.x.

Actually, I was thinking the same, yesterday. but after checking by.

$ git diff 2822382-9.1.x-134.patch 2822382-9.0.x-134.patch

Found that besides the follow one:

 --- a/core/tests/Drupal/KernelTests/Core/Entity/EntityCrudHookTest.php
 +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityCrudHookTest.php
-@@ -40,7 +40,7 @@ class EntityCrudHookTest extends EntityKernelTestBase {
+@@ -41,7 +41,7 @@ class EntityCrudHookTest extends EntityKernelTestBase {

the rest are all similar to the below.

-index 354aae4174..e71f7ff40b 100644
+index c6d8d97f74..7d07ec1989 100644

Not sure whether the difference between the two patches of EntityCrudHookTest matters. so uploaded two.

@jungle git is able to resolve that and still apply the patches... it's patch cruft.

All the code changes looks good.
All the static $modules occurrences are now set to protected. For both 9.0 and 9.1. Used a full projects search.
Both patches get a RTBC from me.
Please commit this fast as I do not like to do this again. It is not fun to do.

As we've already done #3107732: Add return typehints to setUp/tearDown methods in concrete test classes today let's get all the massive test disruption out of the way in one go.

Committed and pushed 6bf02d966d to 9.1.x and d1512f59db to 9.0.x. Thanks!

Very nice to see this in finally! Thanks all for the concerted effort!!

Well, this change makes it impossible to have tests that work for 8.x and 9.x when specifying $modules. This is not good.

Update: I guess this should be backported to 8.9, no?

Update #2: I can set visibility to public of course, but the whole point of this issue is to stop doing this

@Taran2L
I don't know if things have changed since your comment three months ago, but at the moment tests which extend KernelTestBase or BrowserTestBase do run OK at 8.8, 8.9 and 9.0+ with protected $modules.

There is a problem with tests that extend EntityKernelTestBase as these fail at 8.8, 8.9 with protected $modules and have to remain public, which then fails with deprecations at 9.1 only (ok at 9.0)

@jonathan1055: There are still over 2000 tests in core D8 that use public instead of protected. Of these, more than 100 are test base classes. Any contributed module that extends one of these test bases will have a problem because the change from public to protected is required for D9.1+ but will fail in D8. Affected contributed modules can't support D8 and D9.1+ at the same time.

Note there are still NEW tests/test bases being added to D8 that have this error, for example: core/tests/Drupal/KernelTests/Core/Field/MapBaseFieldTest.php which was created just four months ago.

This started out as a D8 issue and was moved to D9 at some point then D8 was thrown aside and instead of being backported to address the original issue, this issue was closed when it was "fixed" in D9. And it still took more issues to fix more instances of public that were snuck into D9 over the years (for example #3164721: More uses of public static $modules).

In D9.1.x, we have finally enforced this at the CI level (#3165588: Add a check to ensure $modules property is always protected), but now the workaround of using public in contrib no longer works. So contrib can be broken in 9.1 or can be broken in 8.9, but can't work in both.

This has been a five-year long effort, and we keep going backwards in D8. Already so much work has been wasted. In order to fix this we would need to start over again.

Just have to wait until D8 EOL I guess, then it won't be a problem anymore right?