Problem/Motivation

As part of #2788253: Plan for DateTime Range experimental module, we need to verify that removing the datetime_range module without can be done without lasting disruption.

Essentially, we need to

- install the module
- add some fields, etc
- add some views
- try to remove the module, make sure all of the necessary checks happen to prevent removal with data
- remove the data
- uninstall the module
- make sure there is no remaining cruft

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Comments

mpdonadio created an issue. See original summary.

mpdonadio’s picture

Manual test 8.4.x/HEAD (0ce4401143ad8c3331cb63d0820e03a76871afa2)

- Can't uninstall module when daterange fields exist on nodes.
- Field settings are disabled when content exists.
- Delete fields w/ data, get the confirmation screen; field tables gone from DB after I confirm.
- Config export before after deleting fields is identical w/ module enabled

mpdonadio’s picture

Manual test 8.4.x/HEAD (0ce4401143ad8c3331cb63d0820e03a76871afa2)

Repeated #2, but did config export before I enabled the module, and then after I uninstalled it. Same results.

Visual inspection of MySQL didn't see any cruft tables.

mpdonadio’s picture

Manual test 8.4.x/HEAD (0ce4401143ad8c3331cb63d0820e03a76871afa2)

- make view, export conf, add dateraneg fied, save, delete the field from the view, export conf, only diff was

160a161
>       display_extenders: {  }

which is essentially a no-op change.

- delete field that is used in a view, get warned that Views config for that view will be deleted, delete it, View is done => this is consistent with other modules that define fields and hook_field_views_data them (datetime.module in core and address contrib module), so I think this is expected (though odd) behavior

mpdonadio’s picture

Status: Active » Needs review

Not sure what else to test out here? This module is essentially just interfacing with Field API, so if something doesn't work it is probably a bug in Field API.

?

gambry’s picture

@mpdonadio have you tested (and/or are we supposed to test) including changes from #2786577: The Views integration Datetime Range fields should extend the views integration for regular Datetime fields?
May be that's not part of this issue as this one is listed as must-have and the Views integration is could-have, but if it's the case I'm happy to run your same tests with #2786577 applied.

mpdonadio’s picture

Issue tags: +DrupalCampNJ2017

#6, that is not needed as part of this. The purpose of this issue is to ensure that that module does't leave any database table around, and config around, bypass data protections, etc, when you try to remove it. Essentially, if you install the module, do stuff, and then uninstall it, there are no side-effects or orphaned data. Conversely, that the proper protections are in place when you do try to remove things that the module "owns".

I talked about this with @xjm at DrupalCampNJ last weekend, and made the issue/list right after we got done. In the end, I think we will need her guidance on what to do next. If we don't get next steps in a few days time, I will tag this so it gets attention.

mpdonadio’s picture

#4.2, thanks to @larowlan in IRC, since the module is listed as a dependency in the view (in view.dependencies.module), this is expected behavior. And thanks to @andypost for pointing me to #2468045: When deleting a content type field, users do not realize the related View also is deleted as a reference for this quirk.

jhedstrom’s picture

I'm not sure I understand this.

verify that removing the datetime_range module without can be done without lasting disruption.

Meaning we need to verify that removing the module from core, if it isn't out of experimental status, can be done? If it passes experimental status, there's no removal, correct?

mpdonadio’s picture

#9, the requirement is listed as

No disruption or regression to stable functionality (including being able to remove the experimental module without lasting disruption).

The way @xjm explained it to me at DCNJ, is that uninstalling the module should leave no trace. No cruft tables, no orphaned config, no orphaned data, no orphaned files, no hidden/implied dependencies on the module, that sort of thing. And, that the normal protections are in place when you try to remove something and there is a dependency (eg, get warned about deleting a field, can't change field settings once data is there). Also, that having it installed doesn't have side effects.

That is what I tested in #2, #3, and #4.

I think we covered everything in those manual tests. And, we went the full 8.2.x cycle w/o any bugs caused by the module that show up in other places in core (eg, having daterange enabled doesn't break datetime or timestamp formatters).

W/o further guidance, I don't know what else to test here.

gambry’s picture

Title: Verify removing the datetime_range module without can be done without lasting disruption » Verify removing the datetime_range module can be done without lasting disruption

I've run all the checks listed on IS against 3 fields (using all the datetime_range available types) and I confirm tests results from #2, #3 and #4.

I just want to flag Creating a view -> exporting the config -> adding a datetime_range field to the view -> saving -> removing it and saving -> exporting the config. I don't see any change in the exported configuration (as diff on #4 shows a no-op change setting).

As this is a check-only task, does it need RTBC?

mpdonadio’s picture

Title: Verify removing the datetime_range module can be done without lasting disruption » [no patch] Verify removing the datetime_range module can be done without lasting disruption
Version: 8.3.x-dev » 8.4.x-dev
Status: Needs review » Reviewed & tested by the community

Yeah, this needs RTBC for approval for sign-off on #2788253: Plan for DateTime Range experimental module. I am not 100% sure who approves it, though, so not sure which "needs XXX manager" tag to add.

xjm’s picture

Status: Reviewed & tested by the community » Fixed

I think all the testing above covers it. Nice work! I'd consider this resolved. (Also saving issue credit since testing deserves credit too.)

One thing I don't see explicit mention of is disabling and re-enabling the module (which is a good way to test that nothing is slipping past the intended APIs for managing uninstall). In general, that should be part of what we check. However, I'm confident that that is also okay in this case.

For those who are wondering where this issue came from, the Content Moderation module could not be uninstalled safely when it was first added. ;) So, we explicitly added it as a testing/review step for experimental modules then. It's especially important to get right even before the experimental module is first added, since it affects data integrity and could permanently change the site even after the experimental module is not in use. (The expected workflow is that the module might be enabled, tested, uninstalled, and maybe later re-enabled later once stable.)

The reason we ended up with a separate issue for this for DateTime Range is that we had that massively long issue for the initial implementation and were coming up on a much-postponed beta deadline and granted the module many exceptions to try to come up with a compromise that would let us get the module in so contrib wouldn't be so stuck. Normally we would do this testing in the original issue, but in this one case we allowed it to be deferred to the followup.

Thanks everyone!

Status: Fixed » Closed (fixed)

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