Problem/Motivation

Having an alpha experimental module and trying to manage 8.9.x and 9.0.x makes things unnecessarily complex.

Proposed resolution

Remove the module - we only need it to add a UI in - the patch to do that does not exist yet and can add the module.

As an alpha experimental module we don;t need an upgrade path.

Remaining tasks

User interface changes

None

API changes

The config_environment module is removed.

Data model changes

None

Release notes snippet

The alpha experimental module config_environment module is removed.

CommentFileSizeAuthor
#23 interdiff_21-23.txt1.22 KBNeslee Canil Pinto
#23 3121229-23.patch2.85 KBNeslee Canil Pinto
#21 3121229-21.patch1.32 KBNeslee Canil Pinto
#6 3121229-9.0.x-6.patch1.32 KBalexpott
#6 3121229-8.9.x-6.patch1.32 KBalexpott
#2 3121229-9.0.x-2.patch2.85 KBalexpott
#2 3121229-8.9.x-2.patch2.87 KBalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Active » Needs review
FileSize
3121229-8.9.x-2.patch2.87 KB
3121229-9.0.x-2.patch2.85 KB
bircher’s picture

bircher
Location 🇨🇿
CreditAttribution: bircher as a volunteer and at Nuvole commented
Status: Needs review » Reviewed & tested by the community

We can leave it in 9.1 but it will unlikely get to beta before then, so I am +1 for RTBC for removing it in 8.9 and 9.1.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 2: 3121229-9.0.x-2.patch, failed testing. View results

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Needs work » Reviewed & tested by the community

There was a random fail on the 9.0.x patch.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
FileSize
3121229-8.9.x-6.patch1.32 KB
3121229-9.0.x-6.patch1.32 KB

So the module has now been removed from 8.9.x and 9.0.x but we need to fix core/composer.json.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented

Huh, I had thought the Composer Initiative changes in 8.8 made this obsolete? The need to do #6 I mean.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented

Also why didn't HEAD fail if it is referencing a non-existent module?

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Version: 8.9.x-dev » 9.1.x-dev

#2 is still valid for 9.1.x; the issue in 9.0/8.9 just needs to be hotfixed. I'd like to understand why QA passed first though, and/or file an issue for test coverage if appropriate, because IIRC this happened when 8.6.x was branched too.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented 
+++ b/composer.lock
@@ -525,7 +525,6 @@
-                "drupal/config_environment": "self.version",

I don't think that having this in composer.json causes anything to happen at runtime. It does impact what would happen if you do composer require drupal/config_environment but given we normally don't use a contrib namespace for experimental modules this doesn't have much impact. I think this task is mostly house cleaning. And yeah we can removing this from 8.1.x too as I think we shoudl only add this back in if we get the UI up and ready.

  • xjm committed b71ce37 on 9.0.x 
    Issue #3121229 by alexpott: Hotfix cruft from alpha experimental module...

  • xjm committed 540431c on 8.9.x 
    Issue #3121229 by alexpott: Hotfix cruft from alpha experimental module...
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented

Committed #6 to 9.0.x and 8.9.x (or rather, regenerated the same thing by removing the line from core/composer.json and running composer update drupal/core*, because I wasn't sure if the lock file hash had been fixed yet).

I realized that I had remembered the impact of 8.8's changes the wrong way around: One more step is now required to remove a module (of updating the lockfile), rather than one less (of not having to change core/composer.json).

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 6: 3121229-9.0.x-6.patch, failed testing. View results

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Status: Needs work » Reviewed & tested by the community

Fail doesn't have anything to do with #2.

Still wondering though if there's any test that could be added (in a followup obviously) so that QA would fail if the core/composer.json contains non-existent (or non-core) Drupal modules.

alexpott’s picture

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

Yep we can add a test to ensure that core/composer.json only has replace statements for core modules that exist - and also is not missing any.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented

If this has been committed already, can we mark it fixed?

alexpott’s picture

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

It's still open against 9.1.x - which I'm +1 on removing there too as I think it'll result in less release manager work. If we manage to get an experimental config environment ui in then great but adding this back can be part of that patch.

Neslee Canil Pinto’s picture

Neslee Canil Pinto
he/him
Primary language English
CreditAttribution: Neslee Canil Pinto commented
FileSize
3121229-21.patch1.32 KB

@alexpott, patch for 9.1.x.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 21: 3121229-21.patch, failed testing. View results

Neslee Canil Pinto’s picture

Neslee Canil Pinto
he/him
Primary language English
CreditAttribution: Neslee Canil Pinto commented
Status: Needs work » Needs review
FileSize
3121229-23.patch2.85 KB
interdiff_21-23.txt1.22 KB

  • catch committed 9979c5f on 9.1.x 
    Issue #3121229 by alexpott, Neslee Canil Pinto, xjm: Remove...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Needs review » Fixed

Ah OK that makes sense. Yeah we can commit the revert of this once there's a viable patch for something to keep the diff smaller if we want, but makes sense to not have it when there isn't active work on a UI in progress.

Committed 9979c5f and pushed to 9.1.x. Thanks!

Missed that this was needs review instead of RTBC, but patch is identical to previous ones except the composer.lock hash.

Status: Fixed » Closed (fixed)

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