Problem/Motivation

Follow-up to #2926068-25: Deprecate system_rebuild_module_data() and remove usages in core, @alexpott's comment:

There is likely to be a few other KernelTests where this is the case. Let's file a followup to review all tests where system is the only dependency. There are 52 tests where this is the case. For example the same code can be removed from \Drupal\Tests\field\Kernel\FieldDefinitionIntegrityTest but not from \Drupal\KernelTests\Core\Theme\ThemeInstallerTest - so working through that in a follow-up makes sense to me.

This issue is to remove system module dependencies that were created in Kernel and Browser tests just to call system_rebuild_module_data() or some other deprecated method.

Steps to reproduce

N/A

Proposed resolution

Create an issue fork by clicking the button.
Follow the instructions to pull down the fork.
Search core for protected static $modules = ['system'];
If it is in a Kernel test delete the line.
Run the test locally
If it passes commit the change
If it fails undo the deletion
Note the test in the remaining tasks section for further investigation
Push up changes
Create MR
Set issue status to needs review

Remaining tasks

Fixes for the following tests as they don't work after removing the dependency:

  • AddFeedTest
  • BaseThemeRequiredTest
  • ClaroVerticalTabsTest
  • ConfigExistsConstraintValidatorTest
  • ConfigImportThemeInstallTest
  • DbDumpCommandTest
  • DrupalFlushAllCachesTest
  • ElementsFieldsetTest
  • FileSystemRequirementsTest
  • FloodTest
  • FormattableMarkupKernelTest
  • FormElementLabelTest
  • FormElementMaxlengthTest
  • ImageTest
  • InstallerDependenciesResolutionTest
  • MailerDsnConfigValidationTest
  • MenuStorageTest
  • MessageTest
  • ModuleHandlerTest
  • PendingUpdatesValidatorTest
  • RouteNoneTest
  • RouteProcessorCurrentIntegrationTest
  • RunTimeRequirementsTest
  • SimpleConfigValidationTest
  • ThemeInstallerTest
  • ThemeSettingsTest
  • ToolkitGdTest
  • TwigEnvironmentTest

The following one is an abstract class but probably needs to take the dependency out as well

  • FileTestBase

User interface changes

N/A

Introduced terminology

N/A

API changes

N/A

Data model changes

N/A

Release notes snippet

CommentFileSizeAuthor
#33 2943436-nr-bot_lh2z8sfh.txt91 bytesneeds-review-queue-bot

Issue fork drupal-2943436

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

almaudoh created an issue. See original summary.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

nicxvan’s picture

nicxvan commented
Issue summary: View changes
Issue tags: +Novice

This should be a pretty good novice task.

luismagr made their first commit to this issue’s fork.

quietone’s picture

quietone commented
Issue summary: View changes
luismagr’s picture

luismagr commented

Hi,

I'm going to go through the tests and add all the failed ones on the remaining task section when I finish so I only have to update the section once if that's ok.

Thanks

luismagr’s picture

luismagr commented
Issue summary: View changes
luismagr’s picture

luismagr commented

Hi all,

I've pushed to the Issue fork individual commits per test and updated the list of remaining tests to remove the dependency. From here, I'm not sure if we want to fix them under this issue or if we want to create individual issues for each one.

Thanks

luismagr’s picture

luismagr commented
Status: Active » Needs work
quietone’s picture

quietone commented

@luismagr, Can you make an MR so the tests run?

nicxvan opened merge request !13161

nicxvan’s picture

nicxvan commented

Whoops forgot this was novice and created the MR.

@luismagr great work so far! I created the MR in the future you can do that after pushing by clicking compare next to the branch on the issue then clicking create Merge request on the next page.

nicxvan’s picture

nicxvan commented

This is green, I think a good final step world be to remove it from FileTestBase and see if any tests fail.

luismagr’s picture

luismagr commented

Thanks @quietone and @nicxvan

Understanding the workflow was the idea behind working on this issue. Will create it next time for sure. Thanks for creating it this time.

Thanks

luismagr’s picture

luismagr commented
Issue summary: View changes
luismagr’s picture

luismagr commented

It doesn't like the removal from FileTestBase. Shall we revert the commit or do we need to look at that change? Before the commit pipeline was green.

luismagr’s picture

luismagr commented

An example of running the test locally. I got this 

1) Drupal\KernelTests\Core\File\FileMoveTest::testNormal
Drupal\Core\Config\Schema\SchemaIncompleteException: No schema for system.file

/var/www/html/core/lib/Drupal/Core/Config/Development/ConfigSchemaChecker.php:91
/var/www/html/vendor/symfony/event-dispatcher/EventDispatcher.php:246
/var/www/html/vendor/symfony/event-dispatcher/EventDispatcher.php:206
/var/www/html/vendor/symfony/event-dispatcher/EventDispatcher.php:56
/var/www/html/core/lib/Drupal/Core/Config/Config.php:232
/var/www/html/core/tests/Drupal/KernelTests/Core/File/FileTestBase.php:43

I guess the dependency needs to be injected at some other point

nicxvan’s picture

nicxvan commented

Yes let's revert that thank you.

luismagr’s picture

luismagr commented
Issue summary: View changes
luismagr’s picture

luismagr commented

Reverted the one failing and added it to the remaining tasks section. Now is green again

luismagr’s picture

luismagr commented

Btw @nicxvan, is this issue now in "needs review" or is it panned to fix the remaining tests in this one?

nicxvan’s picture

nicxvan commented

I think this is almost ready.

I'm just considering if it's worth a follow up to track the remaining tests or of we need to verify the failing ones.

Honestly I think we are good as is, but I want to check.

It might be worth a followup for file test base.

It might be a week or so before I can dig in.

nicxvan’s picture

nicxvan commented
Status: Needs work » Reviewed & tested by the community

I think this is a good cleanup.

If someone wants a follow up we can create that, but shouldn't block this further.

To be clear the only one I think that would be good is file, but the filesystem is pretty intertwined with system.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 2943436-nr-bot_lh2z8sfh.txt91 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

nicxvan’s picture

nicxvan commented
Status: Needs work » Reviewed & tested by the community

Rebased in the ui I'll keep an eye on tests.

Credit updated too.

mstrelan’s picture

mstrelan commented

I don't really understand the Remaining Tasks. Surely there are some tests that do need the system module? I think we could potentially have a follow up to find tests that have system plus some other modules and see which ones can have system removed.

mstrelan’s picture

mstrelan commented

Opened #3547124: Remove system module from kernel tests that don't need it for #35

  • catch committed e8a8eef9 on 11.x 
    Issue #2943436 by almaudoh, nicxvan, luismagr, quietone, mstrelan:...

catch closed merge request !13161

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Nice to remove all that cruft.

Committed/pushed to 11.x, thanks!

Now that this issue is closed, please review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, please credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.