Make features rebuild() non-destructive

This is by design. All of the Domain exports work this way, to enable environment sync from dev to prod.

How else would you have it work? If you rebuild domains, we wipe and rebuild those, too.

And, frankly, you had MONTHS to review the initial patch. So I'm going to move this to Feature request, and see if anyone else wants to work on it.

I will also say that the Features API is very unclear about what "rebuild* means.

I take *rebuild* quite literally to mean wipe/rebuild.

See #1038362: Document exporting domain settings for the original discussion, especially comments 50-60 and 53, which defined this policy.

Looking at the code and the API, it would seem that the solution is to split hook_features_revert() from hook_features_rebuild(), which ATM perform the same actions.

hook_features_rebuild() would only wipe records found in the default feature.

NOTE: Snark will not help you here. Your OP made me angry due to its tone.

I disagree about what "revert" means. To me, it is clear that "revert" means "wipe and rebuild the way this is defined in the feature."

As far as workflow goes, your team should be updating Features as they build -- or creating the Features later in the build. Failure to do so is a workflow issue, not a code issue. This can get tricky if you are doing automated builds -- which is something already discussed in the other issue.

The question that I have -- which Features docs do not answer for me -- is "is there a rebuild process that is distinct from revert."

Here's the relevant, and unhelpful documentation:

- `hook_features_revert()` reverts components of a feature back to their default
state.
- `hook_features_rebuild()` updates faux-exportable components back to their
default state. Only applies to faux-exportables.

Further information about hook_features_rebuild():

 * This hook is called at points where Features determines that it is safe
 * (ie. the feature is in state `FEATURES_REBUILDABLE`) for your module to
 * replace objects in the database with defaults that you collect from your
 * own defaults hook. See API.txt for how Features determines whether a
 * rebuild of components is possible.

Well, that's clear as mud. And there is neither UI nor drush command for triggering this hook, AFAICT.

Perhaps we're better off removing that hook entirely?

Looking at the code, it does appear that Features default behavior is the reverse of our behavior -- in their paradigm hook_features_revert() always call hook_features_rebuild().

However, I think that is wrong, too, because it leads to orphaned data because Features never deletes anything. Revert should be a hard reset. If rebuild should be a soft reset, that's fine.

So, I'm open to other solutions here, but working code is more valuable than debate here.

I take that back, domain_conf_features_rebuild/revert follows the core pattern.

Perhaps this patch is what we're after.

Revised patch which applies the approach to all modules in the package.

In testing, I can't find any instance where Features actually calls the hook_features_rebuild() call. And since the hook_features_revert() is working as intended, I'm not sure this patch actually does anything.

By expected behavior, consider this message:

[ bash] : drush fd domains_test
Legend: 
Code:       drush features-revert will remove the overrides.
Overrides:  drush features-update will update the exported feature with the displayed overrides

If removing overrides includes deleting non-matching records, I think that is proper.

Sorry haven't used domain conf or domain in a long while.

Features revert/rebuild should only be reverting/rebuilding items that are exported for the $module being reverted -- i.e. check what items have been exported for that module and revert(delete) those from the database for both rebuild/revert. Unless domain is doing something different where it considers the entire configuration exported for every feature, then deleting everything would be an incorrect implementation of the hook.

I cannot think of a case where hook_features_rebuild would be different from hook_features_revert (though there probably is).

Rebuild is called when features decides that a feature's code has been changed instead overridden (via comparison of hashes of previous code compared to database code compared to current code, if I recall correct). It's called by features in flush cache hook and while accessing specific pages (modules form I believe). "This feature code has changed and the feature is not overridden, better update the database for faux exportables".

@hefox

Right, that's a better explanation, which needs to be in the Features documentation.

However, I think Features get this very wrong. Rebuild and Revert should be two entirely separate operations.

- Rebuild: Check all my exported data and reset the database version to match.
- Revert: Replace all database data with exported data.

The fact that Features never "clean up" after themselves, when dealing with things like Content fields, is a huge hole in the API.

I'm willing to make this change in the behavior, because I can see the following scenario:

* I want to export settings for the default domain so that no one can ever tamper with them. All other domains are fair game.

The original intent of exportables in DA was simply: I want to verify configuration is identical across X number of installations. That's what the current code does. And just getting that out the door was a huge win.

If we make this change, my questions are:

* We remove the delete queries, that much is clear -- we can replace them with merge.
* Do we have to do anything about how diffs are calculated? I don't think so, but it isn't clear from the API.

@Deciphered

Thanks for that. The OP was very much a problem for me, and it put me on the defensive, which is counter-productive.

Here's a revised patch that changes that behavior.

Aside from the two questions above, the third question is:

* Look at the @todo lines in the case of "empty" features exports -- e.g. a Domain with no values in domain_conf(). Should we delete those records from the target table. I would assume so.

See also #1487756: Documentation: Clarify the (non) difference between rebuild and revert

Here's my problem with the above approach -- and with Features in general.

* Create a DA site.
* Add 4 domains.
* Export them all to a Feature.
* Enable the Feature.
* Feature shows state == Default
* Create a new domain
* Feature shows state == Overridden
* Revert the Feature
* New domain is not removed
* Feature shows state == Overridden

Now, I just told Features explicitly that I want to restore to the default state. But it won't.

Perhaps this is better addressed in the Features queue, since it seems this is the expected behavior.

OK, I see why I'm getting the above behavior -- because I forced it to act that way because it's the behavior I expect. This patch removes that check; but this, too, creates confusion for me. Now the above feature is not marked "overridden" even though the records really don't match.

Is the "Features approved" solution to this to create a new type of Feature export for these modules that is "EXPORT ALL THE SETTINGS and tell me if there is deviation from any of them"?

That is, we would have a features hook to export some domains, and a features hook to export ALL domains, and treat any deviation as "Overwritten."

Here's a version that allows all domains to be exported as a single, revertable feature but retains the ability to export individual domains.

@hefox -- is this acceptable in the Features world? It looks like it satisfies both needs here.

True, but the 'all domains' checkbox right now is really a helper function for sites with > 10 domains, because checking each box causes an AJAX callback that takes time.

The problem with the patch in 15, is that it won't track changes: e.g., if you export all your domains to code and then add one, it won't report as 'overridden'.

We could, in theory, add another option to the export form:

[ ] -- Wipe records on rebuild.

And we could set a flag for that "wipe == TRUE". That would actually be pretty easy. Then if "wipe" is present, we check the data integrity against the entire table.

Here's a patch that allows for that. It will need some good documentation.

Here's a screen shot of how the interface changes when exporting.

Haven't seen any other features intergration that works like that, but it makes sense.

The more standard way for features would probably be to use the population method, ie. add all domains to export when all domains option is checked
e.g. in hook_features_export

if (all domains) {
 foreach over each domain, adding to features export;
}

Would need to check, but I believe that would be detected as overridden when new domain is added but not complely sure (don't remember if it redoes features_populate when testing for overridden status).

One thing to consider is what would happen if someone doesn't know what's in other features, has a feature with all/whipe option checked, and adds a domain then exports that domain to a new feature so they end up with two features with domains, one with wipe option check and one with a single domain. It looks like it'd currently delete the new domain when first feature is exported.

Personally, features is in a need of generalized option "export all of this type to this component" and "wipe everything not in code for this component". there's been people mentioning that (not sure if issue queue yet, it's been mentioned in person and in unrelated issue queue like #649298: Features install/revert does not remove additional fields from content type already stored in the db). Wouldn't be that hard to generalized I suspect.

BTW, working on a patch that would remove the ahah on every checkbox click and instead do it on demand with a "preview" type button (not direct purpose of patch, after effect of fixing other issues with that horrid ui), #1400298: Removing auto-detected components from a feature (UI update, removing Ajax)

Right. It's a tough problem to solve. We're already populating "all domains" as an option now.

But there are cases where you might have 8 domains on dev and only 4 on live. My opinion at this point is that we move forward with the patch, with the following todo items:

* Sanity-check the language we're using. Is "Wipe domain tables on revert/rebuild" clear enough?
* Document the usage of this feature properly.
* Document the gotcha that deleting/reverting domains will, in fact, wipe any sub-components, too.

There may be an additional todo for sanity checking what happens if, say, Domain Conf has data for domains that don't exist, but I think we handle that now.

Was just bit by this when I put the default domain of our client's into a feature and then woke up the next morning to find all of the other domains (which can be created arbitrarily by our client's clients and therefore aren't in features) had been deleted. Thank goodness I made an SQL backup the night before… I'll take the domain out of the feature to fix it, but then doing a fresh install with our codebase isn't going to work correctly. Looking forward to this, well, not happening.

I was just bit by this as well... I was trying to export two domains to two different features and ended up with only one of them. I tested patch #16 and it solves the issue for me.

I am not sure about the "wipe" option; I haven't seen anything like that with other modules using features... neither the wipe option, nor the "export all" option, actually.

Personally I'm not sure about the "export (and track) all domains" use case; I see domains as something similar to content types or views, i.e. I export selected ones and don't care about other ones. If I create a new one, I add it manually; if I want to remove a temporary one, I delete it manually. (There could be a "Wipe all non-code domains" on the domains list page to ease this step). If the goal is to prevent/revert any "not-code domains" in a production environment, shouldn't this be solved by preventing creation via permissions? Just some thoughts after skimming through the comments above; disregard if this has already been discussed and decided somewhere else.

Either way, I'd love to see something committed that fixes "single-domain exporting"... let me know if there's something I can do to help.

I just got back from vacation. Personally, I think Features gets it wrong by not allowing wipe/rebuild.

The "Add all domains" bit is a UX helper for when you have 10+ domains.

I think we just need consensus on what behavior should be supported. I like the patch, because it gives site builders more power over how features behaves.

My intent has been to commit this and document the behavior.

I think we just need consensus on what behavior should be supported.

IMO we should focus on fixing the "single domain export" use case, which I think is in line with what features was built for and works well with.

I understand the need for the "wipe/rebuild" use case and I agree with you that that is something which the Features module gets wrong... And as such, I think that it's something that the "domain" module should not have to fix, but something that should be added to the features module; I.e. solve it in a generic way, such that it can be used by other modules as well. From what I can see #649298: Features install/revert does not remove additional fields from content type already stored in the db is moving into that direction...

That said, if one of the existing patches is preferred, let me know which one to test and I will do so :-)

Patch 19 is the current "needs review".

I think the only sticking point in that patch is the "wipe/rebuild" option, which I like ATM. So I think it's actually ready to go in.

I tested 1483150-domain-all-features-rebuild_1_0.patch (#19), on a clean D7 (default install profile), with modules: Features, Domain, Domain Alias, Domain Configuration, Domain Theme. Exported features mentioned below contain settings from domain module including all submodules.

Test case 1

Feature with 2 domains, manually selected for export.

1. [OK] Export via UI works
2. [OK] Changes in live configuration are detected properly as being different to exported code
3. [OK] New domains are completely ignored in comparison (as expected)
4. Reverting components:
   1. [BAD] domains:
      1. [OK] If a new domain is added, it is not touched by the reversion process (as exected).
      2. [BAD] If a domain was deleted, the change is detected but reversion does not restore the deleted domain (If the deleted domain was the default domain, this can lead to "no default domain", i.e. a lot of error messages)
      3. [OK] If a property was changed, it gets reverted as expected (e.g. sitename property gets reverted to exported value).
   2. [BAD] domain aliases: reverts without error message, but behaves strangely...
      1. [OK] If an existing alias was deleted, reversion correctly restores the deleted alias.
      2. [BAD] If a new alias was added, then the component remains "overridden", i.e. "new aliases" are not deleted automatically.
      3. [BAD] If the "redirect" checkbox changed, then a new alias with identical pattern is created, and the old one remains in the list. Expected would be just one alias (per pattern) with original "redirect value".
   3. [BAD] domain variables: trying to revert leads to a PDOException, and leaves the feature stuck in the "Rebuilding" state: SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry '1' for key 'PRIMARY': INSERT INTO {domain_conf} (domain_id, settings) VALUES [...]
   4. [BAD] domain theme: trying to revert leads to a PDOException, and leaves the feature stuck in the "Rebuilding" state: SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry '3-garland' for key 'PRIMARY': INSERT INTO {domain_theme} (domain_id, theme, settings, status, filepath) VALUES [...]

Test case 2:

Feature with "all domains", without wipe

Same as Test case 1... "all domains" is simply a (javascript?) helper which adds all existing domains to the feature. I.e. without the "wipe" option, handling is the same as above.

Test case 3:

Feature with "all domains", with wipe.

5. [OK] Export via UI works
6. [OK] Changes in live configuration are detected properly as being different to exported code
7. [OK?] New domains are also listed as "overriding" the feature. I think this is expected?
8. Reverting components:
   
   1. [BAD] domains:
      1. [OK] If a new domain is added, it is detected, and reverting deletes it. (expected with "wipe" option).
      2. [BAD] If a domain was deleted, the change is detected but reversion does not restore the deleted domain (same as above)
      3. [OK] If a property was changed, it gets reverted as expected (same as above).
   2. [OK?] domain aliases: reverts properly and without error message; the wipe causes all existing aliases to be deleted first; then all aliases specified in the feature are created. This causes all Alias IDs (integer) to change... (potential problem for other modules depending on alias id? Alternative would be to use alias pattern as machine name + update existing entries)
   3. [OK] domain variables: reverts values as expected.
   4. [OK] domain theme: reverts values as expected.

Summary

• Deleted domains are not restored.
• Domain sub-modules (aliases, configuration, theme) should behave like "with wipe" all the time. The "wipe" option can then be removed from the features UI for domain sub-modules.

Yes, "all domains" is simply a JavaScript helper. Try exporting 50 domains and you'll see why this is there. Features--.

Let's build, export, and post some common Features for testing.

Here are four simple Features exports. In each case, I exported the default domain and a customized secondary domain. In my test install, there are 13 total domains right now.

None of these have "wipe" enabled.

Attached are the two I used for testing (ft1 -> no wipe, ft3 -> wipe).

URLs used: drupal7qd3.dev, sub0.drupal7qd3.dev, sub1.drupal7qd3.dev

Updated patch that should fix the first [BAD] fail (1.4.1.2) above.

On this report:

[BAD] If a new alias was added, then the component remains "overridden", i.e. "new aliases" are not deleted automatically.

I would consider that "non-destructive" and therefore desirable. This issue is about not deleting records not defined in the feature, isn't it? The fact that it is "overridden" is a Features issue we can't really do anything about. This is why the original implementation was wipe/rebuild. Or are you arguing that Aliases are a different class of data than the domains themselves?

As for alias ids, I don't think any module should be relying on them.

Updated patch that should fix the first [BAD] fail (1/4.1.2) above.

Tested and confirmed. Also fixes it for the "with wipe" case (3/8.1.2).

This issue is about not deleting records not defined in the feature, isn't it? [...] Or are you arguing that Aliases are a different class of data than the domains themselves?

Yes, I am; For one, "domain aliases" are not being displayed to the end user anywhere (different to "domains"), and having "more aliases" in a feature should actually not have any unwanted effects... Secondly: at the moment when you export domain aliases, you have to select "domains" instead of individual aliases, which suggests that aliases are handled "as a collection", i.e. export all and wipe+rebuild.

If we want to export "individual domain aliases", then they should show up individually on the features UI. Personally I'm fine with wipe+rebuild in this case.

Makes sense to me. Just checking.

Any news on this front? :-)

Nope. Busy busy busy.

Okay, I'll have a look at changing aliases+conf+theme settings back to "export all" as per comments #34 and #35, and hopefully upload a patch later...

Here we go. Attached patch fixes all remaining issues marked as [BAD] in #28.

The patch adds conditional db_delete() statements for deleting submodule records for a specific domain being rebuilt, e.g. all domain aliases for a specific domain.

Looks pretty solid to me.

Has anyone else tested it?

Refactored against head. Cleaned up comments a bit.

I think we're ready to fire here. Will need documentation for the wipe/rebuild piece.

Will commit tomorrow if no one derails it.

Committed. 3975188..96fa47d

Awesome :)

The docs page could still use some work. See https://drupal.org/node/1331868