I have some features consisting solely of panels (including minipanels and both 'pages' and 'handlers' components). Each feature is for a single component.

With the current dev release of panels, all of these features are listed as "Overridden" by drush features-list, but viewing the status of each in the Features web interface shows them as being in their default form.

Similarly, drush features-diff reports that there is no override.

I have established that features_get_normal($component, $module_name, $reset); is returning different values for $reset = FALSE vs TRUE.

The difference between the returned arrays seems to be that the 'pid' values for the panes are different. Depending on the value of $reset, the pids in the instance I am looking at now are either numbered from 'new-1', or from 'new-52'.

There are exactly 51 panes in that feature's panel.

edit: And in a different example with 59 panes, the pid numbering starts at 'new-1' (through to 'new-59') when $reset = FALSE, and 'new-60' (through to 'new-118') when $reset = TRUE.

edit2: More specifically, after further testing, with $reset = FALSE a cached value is returned if possible, and with $reset = TRUE the numbering starts from the previous maximum + 1. So calling features_get_normal() three times in a row with $reset = TRUE resulted in pid ranges of 1-51, 52-102, and 103-153. If we then call it again with $reset FALSE, we get a copy of that last 103-153 version.

n.b. Issue moved to Features issue queue as of comment #8 below. Start there for the summary version.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jweowu’s picture

Project: Panels » Features
Issue summary: View changes

Added more data

jweowu’s picture

Issue summary: View changes

More info

jweowu’s picture

Issue summary: View changes

Clarification

Letharion’s picture

Project: Features » Panels
Assigned: Unassigned » merlinofchaos

Sounds like something that needs merlins attention.
@jweowu I appreciate that you took the time to debug this yourself. :)

merlinofchaos’s picture

I don't know how features determines this status.

When you visit the admin page for the panel itself, does it show the correct status?

Letharion’s picture

Assigned: merlinofchaos » Unassigned

My bad, based on the fact that @jweowu had done some debugging already, I assumed he'd already figured out which module was responsible for this.

jweowu’s picture

merlinofchaos: Yes, the summary says "Storage: In code" when I visit the admin page for the panel.

Letharion: Sorry, not fully debugged. I just posted what I knew so far and hoped that it was enough for someone to figure out what was happening. Switching to the dev version of Panels (from 7.x-3.0-alpha3) triggered the behaviour, so this seemed the likely culprit.

jweowu’s picture

The $reset argument to features_get_normal() might be a red herring; regardless of whether it's drush or the web interface, that always seems to be FALSE.

I'm not overly familiar with any of this, but as best as I can tell (and the following is the same for both drush features-list and the web interfaces):

Features determines the status by comparing (for database vs code) the md5 hash value returned by features_get_signature() for each component of the feature. This happens in features_get_component_states() when called by features_get_storage() (there is also features_detect_overrides() but that does not appear to be called in these instances).

Here's the relevant code from features_get_component_states() in features.export.inc:

          $normal = features_get_signature('normal', $feature, $component, $reset);
          $default = features_get_signature('default', $feature, $component, $reset);
          #[...]
          // DB and code states match, there is nothing more to check.
          if ($normal == $default) {
            #[...] for drush features-list, $normal != $default
          }
          // Component properly implements exportables.
          else if (!features_hook($component, 'features_rebuild')) {
            $cache[$feature][$component] = FEATURES_OVERRIDDEN;
          }
          // Component does not implement exportables.
          else {
            #[...] this case is not executed in these instances
          }

When I use drush features-list, the $normal hash value for the "page_manager_pages" component (for my example feature using that component) is different to the $default value. If I use the web interface, the signatures are the same (and identical to the $default value when using drush).

features_get_signature() calls features_get_default() and features_get_normal(), which is how I originally ended up looking at that latter function.

merlinofchaos’s picture

Sounds like this is a features issue so far?

jweowu’s picture

Digging a bit more, I've had a look at what $normal actually is for this example when I run drush features-list, and it looks like a variant of what I saw when calling features_get_normal() with $reset = TRUE -- the pid values don't get reset back to 1 when they (presumably) should.

drush features-list generates the code for all features, and this time I see a starting value of 'new-409' for the particular feature+component I'm testing. Presumably 408 panes were generated by all of the features prior to this one.

(You can skip to the end now, if you like.)

features_get_normal() obtains the code like so:
$code = features_invoke($component, 'features_export_render', $module_name, $module->info['features'][$component], NULL);

That third argument looks like an array of the machine names of the panels.

features_invoke calls page_manager_pages_features_export_render() for the page_manager_pages component, so that's getting called with arguments $module and $data, where $data is the array of panel machine names.

/**
 * Implements hook_features_export_render() for page_manager.
 */
function page_manager_pages_features_export_render($module, $data) {
  // Ensure that handlers have their code included before exporting.
  page_manager_get_tasks();
  return ctools_component_features_export_render('page_manager_pages', $module, $data);
}

That does this:

  $schema = ctools_export_get_schema($component);
  if (function_exists($schema['export']['to hook code callback'])) {
    $export = $schema['export']['to hook code callback']($data, $module);

which is: page_manager_page_manager_pages_to_hook_code($data, $module)

Okay, so we're in ctools now. Within that function we see $objects = ctools_export_load_object('page_manager_pages', 'names', array_values($names));

The return value of that looks good -- pid values start at 'new-1'.

It then loops over $objects calling the 'export callback' function on each one, which is page_manager_page_export(). The $page argument is the $object with the correct pids.

That then calls page_manager_export_task_handler(). Its $handler argument has correct pids.

That function passes $handler to panels_panel_context_export() to generate the bulk of the export (bringing us back to the Panels module). That processes $handler into $display, but retains correct pids, and then calls panels_export_display($display)

Annnnnd hello:

$pid_counter = &drupal_static(__FUNCTION__, 0);

http://drupalcode.org/project/panels.git/blobdiff/19d9f34e9500538da8d772...

#1277908: Introduce UUIDs onto panes & displays for better exportability & features compatibility

Letharion’s picture

Title: drush features-list incorrectly reporting panels as overridden » Features incorrectly reporting panels as overridden. Pid counting changed.
Project: Panels » Features
Version: 7.x-3.x-dev » 7.x-1.x-dev
Category: bug » feature

Summary for those starting to read here:
The number of panes recently changed, to solve #1277908: Introduce UUIDs onto panes & displays for better exportability & features compatibility. This in turn causes Features do consider features overridden, when they are not.

Because of https://drupal.org/node/1277908#comment-5361822
merlinofchaos:

Since CTools exportables has a flag that expressly says what the status of the exportable is, features is in error for not using that.

I'm passing this over to Features, hoping they can accommodate the new behaviour and/or check the ctools flag.

Letharion’s picture

Issue summary: View changes

Clarification

Letharion’s picture

Nice, I got this on one of my own sites now.
In response to #3, I can report the same thing as #4. The page manager UI itself reports the pages as "In code".

bobodrone’s picture

Priority: Normal » Major

I also have encountered this problem with latest dev-version of ctools and features 7.1-beta4.

The ctools page manager says the panel page is "in code" but features says it is overridden.
Example of diff:

Line 55	
'locks' => array(),
 	
),
- 'new-47' => array(
- 'pid' => 'new-47',
+ 'new-2' => array(
+ 'pid' => 'new-2',
   'panel' => 'main',  	
   'type' => 'entity_field',

But now to a more serious thing!
I have updated my feature in my local environment and are deploying them to the live server. Then when I am trying to revert the feature on the live server it refuses to revert, even if it seems to work and it return a status message that "Reverted all page_manager_pages components for..."
But then the override, with my new configuration code overridden, is still there and my feature will never be reverted and therefore my new configuration never get deployed this way.

I can however do "the workaround way" by reverting the panel page inside page manager instead (even if it says it is in code) and it will be deleted from the db, and then save it again to make it work.

/ bobodrone

acrollet’s picture

Status: Active » Needs review
FileSize
1.55 KB

I'm also experiencing this difficulty. I'm attaching a patch that additionally checks the status as reported by ctools when a component is deemed to be overridden.

jweowu’s picture

Thanks acrollet; I've tested that briefly, and it seems to do the trick!

DamienMcKenna’s picture

I have not tested it yet, but relying upon CTools to discern whether functionality is overridden sounds like a good way of handling this.

HnLn’s picture

Great, this has been bugging me :-) Path works for me.

samhassell’s picture

Patch in #11 may need to check if ctools is installed as its not a dependency for Features. Otherwise it appears to work correctly.

das-peter’s picture

I've the issue too and from my experience with it I would say it has something to do with how ctools enumerates the "components" of exportables.
The whole issue looks as if there's a static cache for the enumeration: If you process all exportables in one request you'll get another enumeration as if you process only one exportable.

A basic question is if this is really the intended behaviour. For me it sounds scary :)

acrollet’s picture

Patch attached adds a check to see if ctools is enabled as mentioned in #15.

jweowu’s picture

das-peter: As already detailed in the issue (see the end of comment #7), that is indeed what is happening, and it is intended behaviour.

I've noticed that this change to Panels also makes it a bad idea to run drush features-update for more than one panels-related feature at a time.

fabsor’s picture

This also makes working collaboratively on panels in features basically hopeless, since the pids are changing every time you export, so conflicts occur every time. This has nothing to do with the actual features issue though.

merlinofchaos’s picture

Panels could probably be convinced not to scrub the pid if it's new-* -- that might not be too difficult of a patch if someone wants to pop over to the panels queue and give it a shot.

das-peter’s picture

Since this issue here has quite some attention I'd like to get feedback here first.
Attached is a panels patch which hopefully does what merlinofchaos mentioned in #20.

jweowu’s picture

I've just found that the patches in #11 and #17 prevent Features from seeing changes to Views.

jweowu’s picture

Status: Needs review » Needs work

edit: That seems like it might actually be a Views bug, but I see I'm also not up-to-date with Views (I'm running 7.x-3.0-rc3 and there's a 7.x-3.1). I'll update and re-test.

edit 2: This issue persists with Views 7.x-3.1.

Leaving on "Needs work" regardless of where the bug lies, because I think Features not noticing real changes is a bigger problem than it reporting too many changes.

HnLn’s picture

yep, same problem here with the patch, changes to views are not pciked up.

das-peter’s picture

@jweowu, @HnLn: Did you give #21 without #11 / #17 a test? #21 doesn't change features but panels to get around the initial problem.

fabsor’s picture

Both patches are relevant, since Features probably shouldn't make it's own determination if CTools reports that components are in code, which means the code is used as source for the configuration.

There is one case where this is useful however, and that's when you upgrade panels or any other CTools module and the definition changes in some way. The configuration is still fetched from code, but the actual configuration would be altered if you export it. It is usually a good idea to export the configuration when that happens to avoid strange issues further on. A discussion around this is worth having!

@das-peter, I think it would be a good idea to post your patch (which I support) in the Panels issue queue, since that issue needs to be resolved regardless of what we do with this issue. That way we can approach both problems separately, and it would avoid confusion.

das-peter’s picture

Spin-of issue in panels for #21 created #1417434: Only modifiy numeric pid's on export

jweowu’s picture

I've updated the original issue #1277908: Introduce UUIDs onto panes & displays for better exportability & features compatibility with a new patch to use UUIDs instead of sequential numbers for the exported pane IDs. That approach resolves the problem in this issue, as the pid for any given pane becomes invariable.

See comment #12. It's working well for me. All features / drush feature functionality works the way I want (and once a feature has been updated, features-diff is able to give clean output for future changes).

Review & testing needed.

jweowu’s picture

Actually, I shouldn't have said that the uuid patch resolves this issue, because the patches in #11/#17 are still entirely relevant to this comment:

Since CTools exportables has a flag that expressly says what the status of the exportable is, features is in error for not using that.

But so long those patches result in Views changes not being visible to Features, they aren't really safe to apply.

jweowu’s picture

Issue summary: View changes

issue moved to features

hefox’s picture

Status: Needs work » Postponed (maintainer needs more info)

Anyone know if this is still an issue? Been a while since this was updated and I believe ctools/panels has had many changes since then.

jweowu’s picture

I imagine that the comment I quoted in #29 is still a going concern in theory, however I'm inclined to suggest that we ignore that, based on (a) the observation that the previous attempts to implement that change caused more problems than they solved, and (b) the fact that, since the UUID patch to panels (more than two years ago), I've had no recurrence of the original issue.

The UUID changes recently made it into the stable release of ctools & panels.

If anyone is keen to revisit the ctools exportables status flag, perhaps that should be done as a separate issue?

jweowu’s picture

Status: Postponed (maintainer needs more info) » Closed (duplicate)

As I opened this issue, I'm going to go ahead and close it.

#1277908: Introduce UUIDs onto panes & displays for better exportability & features compatibility fixed the original problem, and if the ctools exportables status flag is ever to be followed up, it should be done in a new issue.