Closed (fixed)
Project:
Drupal core
Version:
8.6.x-dev
Component:
content_moderation.module
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
12 Mar 2018 at 16:20 UTC
Updated:
3 Jul 2018 at 11:04 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
timmillwoodThis is a good point.
I've queued a test because there are a number of tests which depend on the editorial workflow. I guess we could add this config to the testing profile too? or change the tests to have their own config?
Comment #3
chr.fritschI don't think we are adding things like this to the testing profile.
Maybe introducing a ContentModerationTestTrait that provides a method to create the workflow?
Comment #4
timmillwoodRight, I think a trait might be the best solution here.
Comment #5
chr.fritschLet's see how far we get with this...
Comment #7
timmillwoodThanks @chr.fritsch, looking good so far, 25 is better than 57.
Comment #8
chr.fritschNext try. Hopefully, we come even closer...
Comment #10
chr.fritschAll the tests are fixed now
Comment #11
timmillwoodTwo nit picks:
Functionalnamespace, but also used in Kernel tests, maybe we should put it in\Drupal\Tests\content_moderation\ContentModerationTestTraitor\Drupal\Tests\content_moderation\Traits\ContentModerationTestTrait?Otherwise, looks good!
Comment #12
chr.fritschThanks for reviewing @timmillwood!
I moved the trait into a "Trait" folder and added a check to
Drupal\Tests\standard\Functional\StandardTestComment #13
timmillwoodAwesome, thanks, looks great!
Comment #14
larowlanThis is going to break people custom tests and contrib tests.
Are we sure it is the right approach?
Profiles can duplicate configuration provided by modules and their configuration will take precedence over that defined in the module.
E.g. we have in core
core/profiles/demo_umami/config/install/automated_cron.settings.yml
core/profiles/standard/config/install/automated_cron.settings.yml
core/modules/automated_cron/config/install/automated_cron.settings.yml
Is there a reason why your profile can't have its own version in config/optional?
Comment #15
berdirYou can override existing configuration, but you can't prevent it from being installed at all. Another problem with overriding is dependencies, it's a pain to for example to override default views that are provided by modules because they are installed when the module is installed (required configuration is, optional is different) and it is for example impossible to add a configurable field/filter to them as that doesn't exist yet at that point.
I'd prefer to have almost all config entities in standard, but I also see the argument that it breaks existing install profiles and other modules depending on that specific configuration.
I did feel quite strongly about the related media issue as it was quite a lot of config there that also created bundles and fields which then created tables. This is just a single file, no big deal to delete that in hook_install() if you want to have workflow(s) with different name(s) I think.
Comment #16
timmillwoodI don't believe enforcing config on sites is right, which is why I had supported this issue.
I'm not to worried about breaking people's tests, they can just make use of the trait @chr.fritsch added. I've added a "Needs change record" tag to make sure we have a change record which notifies people about this change and the new trait to use.
I'm more concerned about existing install profiles which depend on the editorial workflow config. I think we'd need to study the backwards compatibility documentation to see if this is even a change we can make. If it's ok to make we can just note it in the change record and 8.6.0 release notes.
Comment #17
chr.fritschI agree with @timmillwood. Creating a change record makes total sense.
https://www.drupal.org/core/d8-bc-policy doesn't say anything about configuration objects and we did that change for media at a point where it was marked as "stable" as well. It doesn't matter if it's one config file or 10.
Comment #18
alexpott+1 for a change record and +1 for the patch. We really should have very little config in core modules. Every piece of non-required configuration makes it more difficult to build products on top of Drupal. We should add something into the experimental module docs to say that a requirement to going stable is to move config to standard's optional directory.
Comment #19
alexpottJust tagging with the release notes mention tag in case this lands - so we don't forget.
Comment #20
alexpottAlso this is something release managers need to make a decision on.
Comment #21
chr.fritschCR drafted: https://www.drupal.org/node/2958726
Comment #22
sam152 commentedAdding a related issue created by someone with the same problem.
Comment #23
sam152 commentedHaving read through this, I agree we have a need for this based on not being able to "override but delete". It immediately felt a bit like the "libraries-override" feature that themes have but for the relationship install profiles have to module dependencies, but I have no reason to believe something like that would be any better than simply moving non-essential configuration to standard. So all in all +1.
Comment #24
sam152 commentedRerolling this one. Given this one has been sitting in the queue for a while, my intention was to tentatively RTBC this and hope it landed on @catch's desk for sign off and commit. I did a quick git log of the standard profile "config" folder and couldn't find anything recent which indicated how these kind of issues usually play out.
I also tested this manually by installing minimal and verifying:
Comment #25
sam152 commentedRTBC with the caveat @alexpott wanted a release manager to review it before commit.
Comment #27
chr.fritschTestbot hickup
Comment #28
catchI think the change is fine. It might require modules to update, but the trait is there for tests, and copying one configuration file is not bad.
Committed 34637de and pushed to 8.6.x. Thanks!