Problem/Motivation

  • Currently, ModuleTestBase::assertModuleConfig() assumes that all a module's default configuration should be available as configuration objects whenever the module is installed.
  • If a module provides configuration that belongs to a different module's API (e.g., the node listing view provided by the node module in #1806334: Replace the node listing at /node with a view), this configuration is not available unless the other module's API is available.

Proposed resolution

Remaining tasks

Related issues

Files: 
CommentFileSizeAuthor
#25 drupal8.module-test-config.25.patch1.37 KBsun
PASSED: [[SimpleTest]]: [MySQL] 50,485 pass(es). View
#24 drupal8.module-test-config.24.patch1023 bytessun
PASSED: [[SimpleTest]]: [MySQL] 50,511 pass(es). View
#20 module-config-1850158-19.patch789 bytestim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 49,352 pass(es). View
#17 enable-disable-1850158-17.patch677 bytestim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 49,296 pass(es). View
#7 config-1850158-7.patch938 bytesxjm
PASSED: [[SimpleTest]]: [MySQL] 48,757 pass(es). View
#1 drupal8.config-module-test.1.patch677 bytessun
FAILED: [[SimpleTest]]: [MySQL] 48,721 pass(es), 4 fail(s), and 0 exception(s). View
assert-module-config.patch941 bytesxjm
PASSED: [[SimpleTest]]: [MySQL] 48,755 pass(es). View

Comments

sun’s picture

Issue tags: +Configuration system
FileSize
677 bytes
FAILED: [[SimpleTest]]: [MySQL] 48,721 pass(es), 4 fail(s), and 0 exception(s). View

I'd suggest to do this instead.

xjm’s picture

Ah, that's much cleaner.

I confirmed locally that this also fixes #1806334: Replace the node listing at /node with a view; however, I'm seeing a couple other failures locally.

xjm’s picture

Ah, the failures with #2 #1 are caused by the fact that Breakpoint only provides breakpoint.yml, which I guess is not matched by listAll('breakpoint.').

Status: Needs review » Needs work

The last submitted patch, drupal8.config-module-test.1.patch, failed testing.

sun’s picture

Oh my. That's essentially why we badly need #1701014: Validate config object names

This means we have to rename breakpoint into breakpoint.settings as part of this patch.

xjm’s picture

The problem is:

  $extension = '.' . self::getFileExtension();
  $files = glob($this->directory . '/' . $prefix . '*' . $extension);

If the prefix is breakpoint., then this pattern will only match breakpoint.*.yml. Weird that this hasn't caused other problems.

xjm’s picture

Status: Needs work » Needs review
FileSize
938 bytes
PASSED: [[SimpleTest]]: [MySQL] 48,757 pass(es). View

Ah yes, #1701014: Validate config object names is the problem exactly. Edit: well, a complement to it, maybe we need to re-scope that issue a little. So we consider breakpoint.yml an invalid config filename, and the config filename must always have two(-plus) parts?

Aside, finding the string breakpoint to replace it with breakpoint.settings within the module directory is entertaining. Here's just the file rename as a start.

sun’s picture

So we consider breakpoint.yml an invalid config filename, and the config filename must always have two parts?

Exactly. Breakpoint's config object essentially is correctly namespaced, but it doesn't actually have a name. ;)

xjm’s picture

Uh. The fact that #7 passes makes me wonder about breakpoint's test coverage. We just renamed breakpoint.multipliers to breakpoint.settings.multipliers, so it seems like something should fail? I guess there's no coverage for whether defaults are installed.

[lorentz:breakpoint | Sun 18:17:44] $ grep -r "multipliers" *
breakpoint.module:      $output .= '<dt>' . t('Assigning resolution multipliers to breakpoints') . '</dt>';
breakpoint.module:      $output .= '<dd>' . t('Multipliers are a measure of the viewport\'s device resolution, defined to be the ratio between the physical pixel size of the active device and the <a href="http://en.wikipedia.org/wiki/Device_independent_pixel">device-independent pixel</a> size. The Breakpoint module defines multipliers of 1, 1.5, and 2; when defining breakpoints, modules and themes can define which multipliers apply to each breakpoint.') . '</dd>';
config/breakpoint.yml:multipliers: [1x, 1.5x, 2x]
lib/Drupal/breakpoint/Plugin/Core/Entity/Breakpoint.php:   * The breakpoint multipliers.
lib/Drupal/breakpoint/Plugin/Core/Entity/Breakpoint.php:   * @var multipliers
lib/Drupal/breakpoint/Plugin/Core/Entity/Breakpoint.php:  public $multipliers = array();
lib/Drupal/breakpoint/Plugin/Core/Entity/Breakpoint.php:    // Remove unused multipliers.
lib/Drupal/breakpoint/Plugin/Core/Entity/Breakpoint.php:    $this->multipliers = array_filter($this->multipliers);
lib/Drupal/breakpoint/Plugin/Core/Entity/Breakpoint.php:    if (!array_key_exists('1x', $this->multipliers)) {
lib/Drupal/breakpoint/Plugin/Core/Entity/Breakpoint.php:      $this->multipliers = array('1x' => '1x') + $this->multipliers;
lib/Drupal/breakpoint/Tests/BreakpointCRUDTest.php:    $breakpoint->multipliers['2x'] = '2x';
lib/Drupal/breakpoint/Tests/BreakpointTestBase.php:      'multipliers',
sun’s picture

Status: Needs review » Reviewed & tested by the community

You know. That's even better — at least as far as this issue is concerned (and the feature it is blocking).

Let's create a follow-up issue to unbreak Breakpoint module (LOL, pun intended) and add test coverage.

xjm’s picture

Yeah, as far as I can tell Breakpoint doesn't even use what's in that yaml file at all, so we're not actually even breaking anything that isn't already broken. Maybe it was intended for use once a UI was added?

sun’s picture

LOL! How crazy is that? :-D

I'd say that makes an even better explanation than the previous one of no test coverage. ;)

So even better, we don't even break anything with that rename. :)

xjm’s picture

Per webchick's request I tested existing breakpoints in standard with this patch applied and confirmed none of the behavior changes. The toolbar still flips its orientation from horizontal to vertical, and the primary navigation still switches from tabs to touch buttons. The text labels next to the icons also still get hidden.

xjm’s picture

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Hm. Let's definitely get a follow-up (probably a "major task") to figure out what's happening w/ Breakpoint module. I remember that going in with some weird workaround stuff for CMI that are probably still there, and I definitely didn't expect Toolbar to continue to work, since AFAIK it's piggy-backing off the Breakpoint module definitions.

In the meantime though, this seems simple enough, so committed and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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

tim.plunkett’s picture

Priority: Normal » Major
Status: Closed (fixed) » Needs review
FileSize
677 bytes
PASSED: [[SimpleTest]]: [MySQL] 49,296 pass(es). View

#1871774: ModuleTestBase::assertModuleConfig() assumes all of module's default config is owned by that module essentially reverts the assertion part of this fix, because all that was actually needed was the breakpoint file rename.

Essentially, it means that if a module provides a config file from another module's namespace, but doesn't have any in it's own, this will fail.

Full coverage for this will be added in #1776830: [META-1] Installation and uninstallation of configuration provided by a module that belongs to another module's API, but in the meantime this blocks #1779026: Convert Text Formats to Configuration System.

I'm marking my issue as a duplicate.

xjm’s picture

I'm not sure #17 is the correct fix for this; it's unblocking one issue to block another.

tim.plunkett’s picture

Status: Needs review » Needs work

If foo.module has foo/config/bar.baz.yml, that file should not be copied if bar.module is not enabled.

However, just because foo.module has a foo/config directory, that does not mean it requires a foo.*.yml file.

The first part is why this change was made. The second part is what this change broke.

We'll need to fix the logic within the assertion to support both.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
789 bytes
PASSED: [[SimpleTest]]: [MySQL] 49,352 pass(es). View

Well. Git does not allow empty directories. So if $module/config exists, as it already checks, then we don't need to prove that its a non-empty directory.

This line is not related to why this issue was originally fixed, and it is the offending line for my bug.

sun’s picture

In that case, we should keep the existing test coverage assertion (since it was introduced for a reason), but call ->listAll() two times, once without and once with module prefix, and assert that $all_names is non-empty.

tim.plunkett’s picture

Status: Needs review » Needs work

Fair compromise :)

sun’s picture

#1779026: Convert Text Formats to Configuration System is blocked by this issue — any chance to move forward with whatever we agreed on? :)

sun’s picture

Status: Needs work » Needs review
FileSize
1023 bytes
PASSED: [[SimpleTest]]: [MySQL] 50,511 pass(es). View

Unless I'm mistaken, what we want is this?

sun’s picture

FileSize
1.37 KB
PASSED: [[SimpleTest]]: [MySQL] 50,485 pass(es). View

Clarified comments + intentions for posterity.

Looks RTBC to me in case bot comes back green.

xjm’s picture

Status: Needs review » Reviewed & tested by the community

Yeah, this looks correct to me assuming it passes. The comments in #25 are excellent.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks.

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.