Problem/Motivation

Running \Drupal\KernelTests\Config\DefaultConfigTest creates a directory called : in DRUPAL_ROOT. This artefact shouldn't exist.

It gets created by image_install. It happens because file_default_scheme() depends on system config BUT file.inc is part of core.

Steps to reproduce:

  1. Run Drupal\KernelTests\Config\DefaultConfigTest locally
  2. Check for the existence of a directory called : in the root of the checkout. It will contain another directory called styles

Proposed resolution

The simplest fix here is to make it return 'public' if the configuration does not exist. Once it does then the directory is created as expected in virtual file system set up by KernelTestBase.

Remaining tasks

User interface changes

None

API changes

None.

Data model changes

None

CommentFileSizeAuthor
#10 2861622-10.patch2.18 KBalexpott
#10 5-10-interdiff.txt770 bytesalexpott
#5 2861622-5.patch1.43 KBalexpott
#5 2861622-5.test-only.patch649 bytesalexpott
#4 2861622-4.patch817 bytesalexpott
#4 2-4-interdiff.txt1.29 KBalexpott
#2 2861622-2.patch506 bytesalexpott

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Priority: Normal » Major
Status: Active » Needs review
StatusFileSize
new 2861622-2.patch506 bytes

Marking this major since any file operation in kernel tests might cause files to be created in places you don't expect.

dawehner’s picture

dawehner
Primary language German
commented

OOH, I always saw that and thought I became crazy / used some bad command in the command line. Can we explain in a comment why we need some fallback? (pre installation could be a different place where this function returns the wrong output).

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Issue summary: View changes
StatusFileSize
new 2-4-interdiff.txt1.29 KB
new 2861622-4.patch817 bytes

Here's a better fix that is isolated to KTB only.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
StatusFileSize
new 2861622-5.test-only.patch649 bytes
new 2861622-5.patch1.43 KB

Here's a test.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented

If we're writing the test in #5 then file_default_scheme() should be deprecated in a follow-up.

The last submitted patch, 4: 2861622-4.patch, failed testing.

The last submitted patch, 5: 2861622-5.test-only.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 5: 2861622-5.patch, failed testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs work » Needs review
StatusFileSize
new 5-10-interdiff.txt770 bytes
new 2861622-10.patch2.18 KB
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

@Mile23 wrt to deprecation of file_default_scheme() see #2244513: Move the unmanaged file APIs to the file_system service (file.inc) - I don't think that it is in scope here.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

Given that this is a problem in the testing infrastructure (kernel tests don't boot up a real site) I think fixing it just inside tests is totally cool. The referenced issue will fix other parts.

The only small thing I was wondering, do we need a change notice as this might break some contrib tests? I guess this is such an edge case, that this is not worth it?

  • catch committed 9cf2b35 on 8.4.x 
    Issue #2861622 by alexpott: Running \Drupal\KernelTests\Config\...

  • catch committed 629ae3d on 8.3.x 
    Issue #2861622 by alexpott: Running \Drupal\KernelTests\Config\...
catch’s picture

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

Committed/pushed to 8.4.x and cherry-picked to 8.3.x. Thanks!

Status: Fixed » Closed (fixed)

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