The commit from this issue introduced a regression, which is fixed in #3183346: [regression] Overridden features are reverting upon update.
(although technically it was already broken in #1325288: Use regular cache table for features_codecache).

Background

Features contains functionality to determine a hash reflecting the state of a features component in code vs in the database.
By comparing this hash, it can determine if a features component is overridden or not.

A third hash reflects the "last known state" of the features component some time in the past, e.g. when it was last reverted, updated or reviewed.
By comparing this hash to the other hashes, features can determine if a component needs review.
A feature needs review if both the code and the database have diverged since the last known state.
E.g. new code was deployed, but the feature was also manually changed.

Originally this "last known hash" was stored in a variable 'features_codecache'.
This caused problems because the variable can be quite big. Variables are loaded into memory in every request in Drupal 7.

In #1325288: Use regular cache table for features_codecache the storage was moved into a newly created cache table 'cache_featurestate'.
This helped to un-pollute the variables memory.

API:

  • features_get_signature('normal', ..) gets an md5 signature reflecting the component state in the database.
  • features_get_signature('default', ..) gets an md5 signature reflecting the component state in code.
  • features_get_signature('cache', ..) gets the stored md5 signature for 'default' (= code) from some time in the past, e.g. when the feature was reverted, updated or reviewed.
  • features_set_signature(..) sets the md5 signature stored in 'features_codecache' or 'cache_featurestate'. The way it is used, it always stores the value found in code ('default').

Problem

The value does not semantically belong into the cache system.
It should never be "cleared".
It is not a cache, it is a "state".

While clearing of the cache can be mostly prevented, there is still a risk, e.g. if data from cache tables is omitted in a database dump.
And as said it is semantically wrong to use the cache system for this.

Another problem is that in the current implementation the entire data can only be loaded all at once.
There is no way to only load the data for a specific feature or component.
Also when saving the entire data needs to be read and written, even if it only change for a specific component.

Solution

Provide a dedicated table which is not part of the cache system or the variable system.
This can have one record per features component.

Columns: ['module', 'component', 'signature']
Primary key: ['module', 'component']

Optionally we could also store a timestamp. But this can be done in a follow-up.

Follow-up

A remaining problem is that stored signatures never get "cleaned up" even after a feature is disabled or a component is removed in code.
This problem existed since the beginning, so it is not a regression, and does not need to be fixed as part of release 7.x-2.12.

Perhaps there are reasons to keep these values around.
But we should perhaps have an operation to clean them up.

Testing scenarios

We don't have sufficient automatic tests, we need to do some manual testing.
xdebug debugging can help to verify what exactly is happening.

Scenario: Upgrade

Basic scenario:

  • Start with a code + database from an earlier version of features.
    The site should have some features, where some are "needs review", some "overridden".
  • Update the code.
  • Run the db update.

Variations:

  • Start with a database from the -dev version which already includes #1325288: Use regular cache table for features_codecache.
  • Run other operations after you update the code, but before you run the db update:
    - Clear cache.
    - Revert a feature (UI or drush). (this could be as part of a deployment)
    - Update a feature (UI or drush). (this should not normally be avoided)
  • Run the operations from drush OR the UI.

Expected state after db update:

  • Table 'features_signature' was created.
  • Table 'features_signature' contains all the hashes that were previously stored elsewhere.
  • Features are in the same state (overridden, needs review) as before the update.
  • Table 'cache_featurestate' was removed.
  • Variable 'features_codecache' was removed.

Scenario: Fresh installation

Install the new version of features on a website where features was not previously installed.

Scenario: Uninstall

Uninstall features.
Check that the db table is gone.

Scenario: Overall functionality

All the regular features functionality still works as before.

Comments

donquixote created an issue. See original summary.

donquixote’s picture

Issue summary: View changes
joelpittet’s picture

+1 this is likely a better way to go.

donquixote’s picture

Issue summary: View changes
donquixote’s picture

Issue summary: View changes

I am working on a patch.

Meanwhile I am adding a "testing scenarios" section to the issue summary.

donquixote’s picture

Issue summary: View changes
donquixote’s picture

Issue summary: View changes

Update testing scenarios.

donquixote’s picture

Issue summary: View changes
donquixote’s picture

Status: Active » Needs review
StatusFileSize
new14.42 KB

Here we go.

klausi’s picture

Status: Needs review » Reviewed & tested by the community

Nice work, looks good to me!

+++ b/features.install
@@ -156,9 +194,113 @@ function features_update_7200() {
+    $schema = array(

you could call features_schema() here to avoid duplicating the definitions.

  • donquixote committed f0b1d39 on 7.x-2.x
    Issue #3162854 by donquixote, joelpittet, klausi: Move '...
donquixote’s picture

Status: Reviewed & tested by the community » Fixed

  • donquixote committed 4ed1fc0 on 7.x-2.x authored by jstoller
    Issue #3176326 by jstoller: Unknown column 'udated' in 'field list'....

Status: Fixed » Closed (fixed)

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

donquixote’s picture

Issue summary: View changes
izmeez’s picture

Issue summary: View changes

Fixed minor omission of square brackets.