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.
| Comment | File | Size | Author |
|---|---|---|---|
| #9 | features-7.x-2.x-3162854-9-new-table-features_signature.patch | 14.42 KB | donquixote |
Comments
Comment #2
donquixote commentedComment #3
joelpittet+1 this is likely a better way to go.
Comment #4
donquixote commentedComment #5
donquixote commentedI am working on a patch.
Meanwhile I am adding a "testing scenarios" section to the issue summary.
Comment #6
donquixote commentedComment #7
donquixote commentedUpdate testing scenarios.
Comment #8
donquixote commentedComment #9
donquixote commentedHere we go.
Comment #10
klausiNice work, looks good to me!
you could call features_schema() here to avoid duplicating the definitions.
Comment #12
donquixote commentedComment #16
donquixote commentedComment #17
izmeez commentedFixed minor omission of square brackets.