Convert to a storage transformer

Created on 28 August 2020, about 4 years ago
Updated 21 October 2023, 11 months ago

Problem/Motivation

We should allow Config Filter 2.x to be installed instead of 1.x.

Currently the tests fail with 2.x, however. (See #9 in #3140046-9: Drupal 9 compatibility β†’ .)

Proposed resolution

Allow 2.x of Config Filter to be installed in composer.json and fix the tests.

πŸ“Œ Task
Status

Fixed

Version

1.0

Component

Code

Created by

πŸ‡©πŸ‡ͺGermany tstoeckler Essen, Germany

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • First commit to issue fork.
  • @jonmcl opened merge request.
  • Open in Jenkins β†’ Open on Drupal.org β†’
    Core: 9.5.5 + Environment: PHP 7 & MySQL 5.5
    last update about 1 year ago
    Composer error. Unable to continue.
  • πŸ‡ΊπŸ‡ΈUnited States JonMcL Brooklyn, NY

    I hope you all don't mind me creating merge request !5. It is for Drupal 10 compatibility and config_filter ^2.4 (necessary to Drupal 10 compatibility). !5 was made off of !4.

  • πŸ‡©πŸ‡ͺGermany tstoeckler Essen, Germany

    Absolutely don't mind, no problem. Since the tests are still failing, I can't merge it as is. As far as I can tell, #15 is still accurate in terms of what could happen to push this forward.

  • @tstoeckler opened merge request.
  • πŸ‡©πŸ‡ͺGermany tstoeckler Essen, Germany

    Okay, took another stab at this and created a new MR. This time around I went ahead and converted the filter to a core storage transformer, so we don't need a dependency on Config Filter at all anymore. This would have been needed at some point anyway and in the case of this module it will actually be required to properly fix Config Split integration, see #3154531: Some issue when working together with config split β†’ (although I have yet to comment there as of writing this).

    I looked at the issue with installing from existing config again as described in #15 and I now found that this is in fact broken in core: Installing from existing config is not compatible with storage transformers. Attaching a tiny core patch that will fix that and make those tests pass. I will open a core issue for that, but will probably just mark those tests as skipped for now, as I don't think it makes sense to wait on a core bugfix.

    With that I can actually make the tests green locally and the move to a storage transformer is also a really nice code simplification. It's good that I had split out most of the logic into the separate storage classes, as we can just use them in the same way now, so the amount of changes is not that big in fact.

    Having two separate event subscribers would not strictly be necessary, but it will become necessary for the Config Split integration, so just went ahead and did that already. In fact it may be the case that the branch already works together with Config Split but I didn't test.

    Note that I won't merge this as is yet, as there is a few things to figure out, as can maybe be seen from the code. Also this will be the starting point of a 2.x branch, and I will tag a stable 1.0 release before this.

    But it's really not far away. Thanks everyone, in particular @bircher for getting this started!

  • πŸ‡©πŸ‡ͺGermany tstoeckler Essen, Germany

    Oops, forgot the core path, here we go.

  • Status changed to Fixed 11 months ago
  • πŸ‡©πŸ‡ͺGermany tstoeckler Essen, Germany

    Went ahead and merged now, thanks again everyone!

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

Production build 0.71.5 2024