The current implementation breaks with Commerce Product UI as well as various other modules which expect certain array keys in their entity_info_alter() implementation without explicitly checking for their existance.

The attached patch solves the problem by doing a more verbose and exact check using a full (but fake) entity info array for the checks.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

  • das-peter committed 90d617d on 7.x-1.x authored by fubhy
    Issue #2342803 by fubhy: Fixed dependency discovery for Fieldable Panels...
das-peter’s picture

Interesting and complex approach.
Committed to latest dev.
@fubhy Is this something we might need to extend to other entity types as well?

fubhy’s picture

I was actually wondering why there wasn't a generic approach for dependency detection for all supported UUID Features Entity Types. At the same time I didn't want to overdo it in this issue as I was not trying to extend the functionality but merely to fix the error that appeared in conjuction with Commerce Product UI.

Dependency detection could actually also be extended to automatically support other modules that fiddle with the entity property info. Like entity cache, various i18n related stuff, etc.

MatthewHager’s picture

This doesn't support multiple entities in multiple FPP bundles. On line 138, there is a specific check for an array that is part of fieldable_panels_panes that causes a white screen.

PHP Fatal error: Unsupported operand types in /var/aegir/platforms/test/sites/all/modules/contrib/fieldable_panels_panes/fieldable_panels_panes.module on line 138

MatthewHager’s picture

I've fixed the issue. When you have multiple FPP bundles the array merge on line 186 causes all sorts of duplicates that don't need to be there and cause issues in later lines where strings are expected instead of arrays that get created by the merge. I've replaced that function with array_replace_recursive which won't create duplicates where the keys are the same. This has resolved all the issues and the module appears to work properly.

This is my first patch attempt ever :D, if accepted will also be my first Drupal contribution.

das-peter’s picture

Status: Active » Needs work

@MatthewHager There's one slight problem: array_replace_recursive was introduced in php 5.3, the official requirement for Drupal is 5.2.5 (although 5.3 is recommended).
But what about drupal_array_merge_deep, could do this the trick? Would be great if you could give it a test.

JonMcL’s picture

This patch, which is currently in DEV, is triggering a rash of notices for me:'

  • Notice: Undefined index: image in _uuid_fpp_features_dependencies() (line 240 of /Volumes/Behemoth HD/Users/jonmcl/Documents/site work areas/nathanre__work/dev.nathanre/sites/all/modules/uuid_features/includes/uuid_fpp.features.inc).
  • Notice: Undefined index: image in _uuid_fpp_features_dependencies() (line 240 of /Volumes/Behemoth HD/Users/jonmcl/Documents/site work areas/nathanre__work/dev.nathanre/sites/all/modules/uuid_features/includes/uuid_fpp.features.inc).
  • Notice: Undefined index: text in _uuid_fpp_features_dependencies() (line 240 of /Volumes/Behemoth HD/Users/jonmcl/Documents/site work areas/nathanre__work/dev.nathanre/sites/all/modules/uuid_features/includes/uuid_fpp.features.inc).
  • Notice: Undefined index: text in _uuid_fpp_features_dependencies() (line 240 of /Volumes/Behemoth HD/Users/jonmcl/Documents/site work areas/nathanre__work/dev.nathanre/sites/all/modules/uuid_features/includes/uuid_fpp.features.inc).

It might just be a matter of adding a isset($backup[$bundle]) to line 240, but I don't know enough about this module (or of Features internals') to suggest that is the right solution.

Peacog’s picture

Status: Needs work » Needs review
FileSize
2.35 KB

Thanks for the array_replace_recursive() fix. I've included a fix that defines an alternative version of the function for PHP versions lower than 5.3 (with code taken from the Language Dropdown module). Is this an acceptable solution?

I also fixed another bug that was writing a weird line: __drupal_alter_by_ref[][fieldable_panels_pane][fieldable_panels_pane] = fieldable_panels_pane into my feature's .info file.

I've haven't seen the errors mentioned in #7.

  • das-peter committed 0c9b8a0 on 7.x-1.x
    Issue #2342803 by fubhy, MatthewHager, Peacog: Fix dependency discovery...
das-peter’s picture

Status: Needs review » Fixed

@Peacog Thanks for the updated patch. I've replaced the implementation of array_replace_recursive with one actually handles all arguments and not just two. If we mock a php function we have to ensure it works as expected (I hope this one does).

Status: Fixed » Closed (fixed)

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