I think the following issue is a bug, but I am not totally sure. I have observed the following behavior:

1. If I have a config file under /core/modules/x and a config file with the same name under /profiles/myprofile then the file under profile will be used as source config in Reports/Diff.

2. If I have a config file under /modules/contrib/x and a config file with the same name under /profiles/myprofile then the file under contrib module will be used as source config in Reports/Diff. (same with modules/custom)

I think profile should be the source config in both cases.

CommentFileSizeAuthor
#17 3003469-17-with-test.patch5.74 KBjhodgdon
#14 3003469-14-TEST-ONLY-FAIL.patch4.42 KBjhodgdon
#14 3003469-14-with-test.patch5.74 KBjhodgdon
#14 interdiff.txt589 bytesjhodgdon
#11 3003469-TEST-ONLY-FAIL.patch4.39 KBjhodgdon
#11 3003469-with-test.patch5.71 KBjhodgdon
#10 updates_report.png30.1 KBjhodgdon
#10 updates_report.png30.1 KBjhodgdon
#4 3003469.patch1.32 KBjhodgdon
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Pasqualle created an issue. See original summary.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

Yes, I think you are correct that the file under profiles should be used as the config in both cases. Sounds like a bug!

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Title: Source config » Source of config is inconsistent between modules and profiles
Category: Support request » Bug report
Issue tags: -Source config
jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Active » Needs review
Issue tags: +badcamp 2018, +Needs tests
FileSize
3003469.patch1.32 KB

I looked into this, and it appears that the problem is just that the code in this module that is trying to figure out where config is coming from is wrong, not that the config system's logic is wrong.

Here's a patch. We'll probably need to figure out how to test it. What I'm not sure about is why it was sometimes working for profiles at all? I would think it would have just been wrong.

Anyway, the only thing this patch would affect is the Provider column in reports that shows you who provided the config, because if you for instance did a report on View type config, the report (or class or Drush or whatever) would just ask the config system "get me this view config" and the config system would pick the right one. Or at least I would hope so... if it isn't, that would be a Core bug. So I'm assuming this bug report is just saying that the "provider" column in reports from this module is wrong.

Pasqualle’s picture

Pasqualle
Location 🇭🇺 Budapest
CreditAttribution: Pasqualle as a volunteer commented

Hmm, I just now realized that the Provider column is telling me where the source config is. I was always looking at it as that module created the config schema, not the actual config. I was wrong, the Provider column is really useful, it almost solves my feature request from #2923415: Show the full path to the config file.

I can see that config is picked from the profile also, without this patch, but the Provider column shows "profile_name Module". Maybe the profile is part of module list, and that is why it was searched for config files. I am just guessing here..

I am not sure if I follow your notes about core config system picking up the right config. The config system is always using the config from the database. Afaik core does not need to know who added the given config.
If you install a module and it contains my-config.yml, then you can not install a second module which contains a config with the same name. So there is no order as you can have only one config.
The special case is the profile, which can overwrite any config (at install). Therefore if there is a matching config under the profile, then most probably profile provided the source for given config.

Ok, instead of talking I should test the patch now..

Pasqualle’s picture

Pasqualle
Location 🇭🇺 Budapest
CreditAttribution: Pasqualle as a volunteer commented

OK, I have applied the patch, but I am using sub-profiles also. See #2945742: Support for sub-profiles.
So for me the config is mixed up for sure. It works like this:
If the config is in base profile, then provider shows "base_profile_name module". The diff seems to pick up the source config correctly.
If the config is in sub-profile, then provider shows "sub_profile_name profile", but the diff shows the source config not from sub-profile but from the source module. Like diff on system.date shows the config from the system module, and not from my sub-profile where it is overridden.

I guess this was not an useful review..

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs review » Needs work

OK, I'll need to look into this more, it looks like! Let's mark it Needs Work. Thanks for the additional information.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs work » Needs review

Returning to this issue...

Core support for sub-profiles is not finished:
#1356276: Allow profiles to define a base/parent profile
and this module will definitely require some changes for sub-profiles in any case -- it has all over it the assumption of only one profile. We have a separate issue for that, currently postponed on the above Core issue:
#2945742: Support for sub-profiles

So. What we need to figure out is whether the current patch is correct for the case of only 1 profile (which is all that this module supports right now). Then we can worry about sub-modules when/if Core supports them on that other issue.

The most recent review in #6 (aside from the sub-module part) said that there is still an issue with this patch: in the Provider column, if the source is a profile, it says "xyz module" not "xyz profile" as the source name.

But I just tested it on simplytest.me as follows:
a) Installed with Standard profile with Config Update with the patch in #4, and turned on the Config Update Reports (config_update_ui) module.
b) Went to the Appearance page and changed the default theme to Stark, to make sure that there would be something in the report that had been changed from the values in the Standard Profile.
c) Went to the Reports page and ran the report on Everything. It looks correct to me. Here is a screenshot, showing that items from Standard profile are shown as "Standard profile" not "Standard module" in the Provider column:

d) Also noted that for system.theme configuration, it says the provider is "Standard profile".
e) Also I did a diff on the system.theme configuration and it says my site has "stark" as the default theme, while the source value is "bartik", which comes from the profile (in system module, the default is stark, so there would have been no differences to my current site config).

So... I think the current patch is working correctly, aside from the idea of base profiles, which I think we should address on the other issue once Core has support.

Thoughts?

Pasqualle’s picture

Pasqualle
Location 🇭🇺 Budapest
CreditAttribution: Pasqualle as a volunteer commented

Yes, I agree.

The provider column issue is coming from the use of sub-profiles.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs review » Needs work
FileSize
updates_report.png30.1 KB
updates_report.png30.1 KB

Hah, I guess I forgot my screenshot from #8, uploading for completeness.

So, looks like you agree this should be RTBC? Oh, except it probably needs a test. I think I should be able to figure something out between the Standard profile and either Node or System or User module (Standard overrides a couple of configs there). Well, actually similar to what I did to test it manually.

So setting to Needs Work for the test.

And this time I will try to actually remember the screenshot after finishing my comment. :)

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs work » Needs review
FileSize
3003469-with-test.patch5.71 KB
3003469-TEST-ONLY-FAIL.patch4.39 KB

OK, here we go! I've made a test that verifies if a module and profile both provide the same config, the profile wins. Locally anyway, it fails without the patch, and passes with the patch. Here's a test-only patch, and a patch with the test and code changes combined.

The last submitted patch, 11: 3003469-with-test.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 11: 3003469-TEST-ONLY-FAIL.patch, failed testing. View results

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs work » Needs review
FileSize
interdiff.txt589 bytes
3003469-14-with-test.patch5.74 KB
3003469-14-TEST-ONLY-FAIL.patch4.42 KB

OK, the FAIL test did the right thing, but the with-test patch failed due to coding standards checks in the test file. Fixing that...

The last submitted patch, 14: 3003469-14-with-test.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Status: Needs review » Needs work

The last submitted patch, 14: 3003469-14-TEST-ONLY-FAIL.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs work » Needs review
FileSize
3003469-17-with-test.patch5.74 KB

OK, I forgot a comma at the end of the line in

    'foo_module' => [
      'module',
      ['foo.barbaz.one', 'baz.bar.one'],
      ['foo.barbaz.two'],
    ],

Not making a test-only and interdiff this time...

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Issue tags: -Needs tests

  • jhodgdon committed a33c46d on 8.x-1.x 
    Issue #3003469 by jhodgdon: Source of config is inconsistent between...
jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs review » Fixed

Thanks again for reporting this issue and discussing! Committed that patch.

Status: Fixed » Closed (fixed)

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

Chris Matthews’s picture

Chris Matthews CreditAttribution: Chris Matthews commented
Parent issue: » #3081273: Plan for Configuration Update Manager 8.x-1.7 release