Problem

Going to admin/structure/features/<invalid-feature> shows errors instead of returning a standard 404.

Steps to reproduce:

  • Install a fresh d7 site
  • Download and enable features-7.x-dev
  • Go to admin/structure/features/foobar

Expected result: 404 Not Found (since foobar is not a valid feature)
Actual result: 200 Found with a broken page + warning message below

Warning: array_flip(): Can only flip STRING and INTEGER values! in features_get_component_states() (line 958 of /var/www/d7/modules/contrib/features/features.export.inc).
Warning: Invalid argument supplied for foreach() in features_admin_components() (line 1167 of /var/www/d7/modules/contrib/features/features.admin.inc).
Warning: array_flip(): Can only flip STRING and INTEGER values! in features_get_component_states() (line 958 of /var/www/d7/modules/contrib/features/features.export.inc).
Warning: array_diff(): Argument #1 is not an array in features_get_storage() (line 646 of /var/www/d7/modules/contrib/features/features.export.inc).

Proposed resolution

Patch attached.
Menu item admin/structure/features/%feature uses an auto-loader wildcard that calls feature_load() which ends up calling features_get_info(). However, for the auto-loader wildcard to return a 404 when %feature is not a valid feature, feature_load() should return FALSE instead of array().

CommentFileSizeAuthor
features-errors_invalid_feature.patch613 bytesfengtan

Comments

fengtan created an issue. See original summary.

fengtan’s picture

fengtan
Location Montreal, Canada
commented
Issue summary: View changes
skwashd’s picture

skwashd commented
Status: Needs review » Reviewed & tested by the community

Patch looks good to me.

  • mpotter committed 9031b4a on 7.x-2.x authored by fengtan 
    Issue #2562871 by fengtan: Errors when going to admin/structure/features...
mpotter’s picture

mpotter commented
Status: Reviewed & tested by the community » Fixed

Committed to 9031b4a.

Status: Fixed » Closed (fixed)

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

donquixote’s picture

donquixote commented

Not happy.
Semantically this is a regression from #1635662: Undefined index: feature in features_get_info()
In practice it does not really matter, but still..

Firstly, it is the responsibility of the wildcard loader to interpret the result from features_get_info() and return FALSE for "feature not found".
features_get_info() is an API function, its return value / signature should not be dictated by the needs of the wildcard loader.

features_get_info() has two "modes":
- Fetch info about a specific module -> return value \stdClass
- Fetch info about all (feature) modules -> return value \stdClass[]

The "none found" return value used to always empty array, which made sense for the second mode, but not for the first mode.
The new return value for "none found" with this patch is FALSE for both cases. This makes somewhat sense for the first case (although NULL would be better), but not for the second case.

The ideal return value would be:
- When fetching a specific module: \stdClass if found, NULL if not found.
- When fetching all (feature) modules: \stdClass[], or empty array if none found.

For BC reasons, we have to continue using FALSE instead of NULL in the first case.

Technically the $type parameter could be something other than 'feature' or 'module', in which case I would still expect an empty array and not FALSE.

In practice this never occurs. The list of feature modules is never empty because we always have 'features_test' module.
Unless some exotic module uses hook_system_info_alter() to remove features_test.

Still, this is not the way it should be.