Fatal error when there's a views page with admin/content/xxx path

So you're saying that your view on admin/content/xxx is NOT a view that uses the system display handler?

    // Grep all router items below the target path.
    $num_parent_parts = count(explode('/', $path));
    $children = preg_grep('@^' . preg_quote($path, '@') . '/@', array_keys($callbacks));

    // Ensure correct inheritance of properties on the original parent path
    // (being replaced) to child items.
    foreach ($children as $child_path) {
      // Only apply property inheritance to direct children of the parent path.  <-----------------
      $num_child_parts = count(explode('/', $child_path));
      if ($num_parent_parts == $num_child_parts - 1) {

So, hrm, possibly yes... that code does not seem to check whether the $child[_path] actually 1) is a view and 2) is a view using the system display handler.

OTOH, we always need to inherit, since e.g. the case of the admin/content view, any router item properties of the admin/content view would be inherited to any + all child router items.

In any case, we should be able to reproduce this bug in a simple test. Let's do that first. :)

How about something like this to get the ball rolling? Just creating a default view at admin/content/test and checking for the title text in the response? It should currently fail as nothing is returned.

Sorry, had the default view in the wrong place. Created a setUp method for in on the plugin test class.

Did I put the stuff in the setUp the wrong way round?! One of those days ....

That's what I was after, 1 failing test!

I was about to say dude, they whole point of this patch is for the test to fail! :P

@sun, we have the failing test now :)

@alexweber, yes, the first 2 patches were truly broken though! Hence elation with the last failing test, which is correct :)

Here is test with a proposed fix, to check whether the child page callback is a 'views_page'. Should work?!

Hm, that passes locally..

#11: 1673820-11.patch queued for re-testing.

@damiankloip, this may sound silly but is this against dev? I'd love to help out if I get a chance later on today...

@alexweber, yep, this is against the 7.x-1.x repo, same as my local.

@damiankloip, I've reviewed the patch and have 2 comments at the moment:

1 - It throws a notice in strict mode:

Notice: Undefined index: page callback in views_plugin_display_system->execute_hook_menu() (line 161 of /path/to/admin_views/plugins/views_plugin_display_system.inc).

2 - Some tests still don't pass:

72 passes, 2 fails, 1 exception, and 17 debug messages

I'm gonna take a few to look into it and I'll post back if I find anything!

Update: The patch from #11 fixes this specific issue but breaks links under "admin/people", specifically the "admin/people/create" link. That's what was causing the failed test. Moreover, the test added by the patch was thrown into another existing test, which isn't ideal.

Attached is a patch that refactors that approach and creates a new test which, as expected, fails because page views created in the admin/content/xxx path are most certainly broken.

Ok, this should now work perfectly!

1. Test slightly refactored (the one from #19 was never going to pass)
2. Fixed the original bug
3. All tests pass!

silly testbot, don't make me slap you

#20: admin_views-1673820-20-final-fix-bug-and-pass-test.patch queued for re-testing.

@alexweber, good work!

I have changed a few comments, removed the cache and menu rebuild stuff too, as the view wont be previously loaded so there is no real reason to clear the static cache (I don't think). Also, menu_rebuild() will be invoked when the view is saved anyway.

So... Hopefully, this version should be good!

Hmm, that's annoying. Maybe the menu_needs_rebuild being set isn't enough....

oh dear.....

Just trying something else ....

Oh, ok. So we do need both? I'm not entirely sure why, but give testbot what he wants :)

Sorry for doubting the menu stuff to start with! :)

@damiankloip, no problem. The test bot was on crack or something cause now the patch from #20 passed too :)

Either way, a maintainer should probably take a look at these and hopefully commit the fix!

@alexweber, that would be me then ;) I would like it if @sun could take a look too though, before I commit this one. So maybe we will leave it for a bit...

@damiankloip oops :)

It never ocurred to me to look at the project page I just assumed you were some random guy like me trying to get this to work :)

+++ b/plugins/views_plugin_display_system.inc
@@ -177,6 +177,14 @@ class views_plugin_display_system extends views_plugin_display {
+        // Page views created are children of a system view,
+        // 'admin/content/child-view', for example, will fail as the 'file' key
+        // is inherited. The simplest possible fix in this case is just
+        // removing it if the callback is a views page.
+        // @link http://drupal.org/node/1673820
+        if (isset($child['page callback']) && ($child['page callback'] === 'views_page')) {
+          unset($child['file']);
+        }

1) Can we improve and clarify the comment to state more clearly what special condition we're trying to account for? If/however/then/else... Especially the parent/child relationship and types of router items being targeted (a view) is not really clear. The @see (bogus @link) should not be necessary.

2) I wonder whether 'file' is really the only property that produces problems? Is it possible that we might have to change the primary condition for adjusting the children in this loop, instead of trying to fix up a particular unintended consequence of performing the adjustments in the first place?

+++ b/tests/admin_views.test
@@ -145,3 +145,94 @@ class AdminViewsDefaultViewsTestCase extends AdminViewsWebTestCase {
+  /**
+   * Tests creation of a view with page display and a child of "admin/content".
+   */
+  function testAddPageViewAdminContent() {
+    $this->drupalLogin($this->admin_user);
+
+    // Test the child view exists by checking for the page title.
+    $this->drupalGet('admin/content/test');
+    $this->assertText('admin_views_test_normal');
+  }

Can we expand and clarify the comments for this test a bit? It's not really clear right now what exact scenario is being tested (unless you know of this issue).

Thanks for the feedback sun.

As far as your comment regarding "file", I did test this extensively and it's pretty much the only key that breaks things. I do agree, however, that it's more elegant to avoid the problem happening than fixing it later.

I'll get on this later on today when I have some time. That is, if damiankloip doesn't beat me to it first :)

@alexweber, I am having a quick look at this now. I don't have too much time though. So if you see nothing on here later, jump in! The original patch attempted to move this logic before the child item is modified I think?

Worth a go.

unfortunate about the whitespace :)

Heh, that still breaks. Ran out of time now...

This is proving to be a herculean effort for such a small issue... :)

Props for persistence, lol, I'm not giving up either, I just won't be able to check this out for another 8 hours, I'm at work atm.

The last effort admittedly were quick and slightly lazy :), when I have more time. As before, please jump in later if you feel up to it.

The test failure is being caused, because the new condition checks for a page callback being set, but children do not necessarily have a page callback.

I think we want to leave the initial condition alone, and rather add a second inner condition; e.g.:

      if ($num_parent_parts == $num_child_parts - 1) {
        // ...explain edge-case...
        if (isset($callbacks[$child_path]['page callback']) || $callbacks[$child_path]['page callback'] === 'views_page') {
          continue;
        }

@sun ha, that's pretty much what I had in my first patch after the test :) its always nice to go around the houses though...ill fix this up shortly when om home.

How about this?

Schoolboy error! what is wrong with me today....

@sun, happy for me to put this in?

Pretty please :)

And IMHO this alone warrants a new version... I've had to disable this module on quite a few websites because of this bug... I'm sure I'm not the only one!

Looks good, thanks! :)

Awesome, committed. Thanks @alexweber & @sun!

@sun there are a couple of other patches I would like to get in before a release, but I think this was the last bug blocker. What do you think? Can we just straight to an rc? Is that too bold? :) It will get more people using it think. You know how people get scared of alpha/beta releases, plus we have done quite alot of testing and patches since the last release.

Thanks @damiankloip and @sun! :)

big thanks for the work on this - has solved a huge headache for me

I'm not sure that this has been totally fixed. I just downloaded admin_views-7.x-1.0-alpha1+22-dev and the Relation module, which uses the path $base_url/admin/content/relation, and I'm seeing the same error described by OP.

This was originally posted in the Relation queue: #1739332: Contrib module admin_views breaks relation tab

Got this issue too with admin_views alpha 1, this wasn't a views page but a custom module menu item under admin/content/bulk-upload. Have upgraded to 7.x-1.x but the issue still remains, bumping issue priority

@acbramley thanks for clearing this up. The committed patch was specific to views pages so I guess we need to account for custom paths too...

Even more so, I've lost access to admin/content and admin/people after upgrading to --dev. I was really looking forward to using this module but can't now :(

You can always try to help resolve the issue....

I was going to start looking into it today, what's the reason behind the code that changes all children menu items to use the same properties as the views menu item with the system display?

This seems to be working ok for me, do you have your hook_menu implementation?

(Needs work is a patch needing work)

Ok, a bit more detail:

* After enabling admin_views, I saving a view with a system path of admin/content and dumped out $child directly under the line $child = &$callbacks[$child_path]; (line 166 of views_plugin_display_system.inc), please find the child dumps in the attached file below.

* I now get access denied on admin/content, and errors like this on sub pages (this example is on the custom page defined in a custom module tpow_bulk_upload:

PHP Fatal error:  require_once(): Failed opening required '/var/www/drupal/transpower/sites/all/modules/custom/tpow_bulk_upload/node.admin.inc'

Child menu items

Can you provide some sample code so I can re create your implementation of hook_menu etc.. here locally? If/when I can re create this I will try and get a fix done today.

Are you just using the unchanged/stock default views too?

Ok, so my custom module is called tpow_bulk_upload. Here's the hook_menu:

/**
 * Implements hook_menu().
 */
function tpow_bulk_upload_menu() {
  $items = array();

  // Upload page
  $items['admin/content/bulk-upload'] = array(
    'access arguments' => array('access administration pages'),
    'page callback' => 'drupal_get_form',
    'page arguments' => array('tpow_bulk_upload_form'),
    'title' => t('Bulk upload tool'),
    'description' => t('Bulk upload multiple files and create content from them (based on file name and form settings).'),
    'type' => MENU_LOCAL_TASK | MENU_NORMAL_ITEM,
  );
  $items['admin/content/bulk-upload/upload'] = array(
    'title' => t('Bulk upload tool'),
    'type' => MENU_DEFAULT_LOCAL_TASK,
    'weight' => -10,
  );
  return $items;
}

For the page callback you could just use:

function tpow_bulk_upload_form($form, &$form_state) {
  $form['heading'] = array(
    '#markup' => '<h2>' . t('Bulk upload tool') . '</h2>',
  );

  return $form;
}

The views I am using are cloned versions of admin_content_node and admin_user_user from the alpha 1 version of admin views. I will try to replicate this on a fresh drupal install

Replicated the error:
Fatal error: require_once(): Failed opening required '.../sites/all/modules/custom/tpow_bulk_upload/node.admin.inc'

Installed my custom module and 7.x-1.x admin_views on a fresh drupal install (obviously along with views, entity, vbo, and ctools), went to admin/content/bulk-upload and got the error.

That'll happen, we always blindly inherit the "file" array key from the parent menu item; in this case "admin/content" (which lists nodes and requires node.inc). If im not mistaken that patch above was specfic to views implementations but im guessing this can also happen with custom hook_menu()s and possibly with page manager too.

Updating to the latest dev fixed my problem. tx

@alexweber, yes you are right. I'm not entirely sure of the best way to approach this reliably for all cases. I would guess if you added a 'file' key to your hook_menu item (just specifying even the .module file of your module) it would work ok?

@damiankloip Yeah I think that would work!

I'm not sure we can fix this tbh, we don't really have a way of knowing if the child is meant to inherit the file property from it's parent or not. So I think the solution would have to be that it needs to be explicitly set if you have any child menu items?

Closing as 'won't fix' for this. @sun, please reopen if you disagree.

I ran into this error as well, but it was dealing with admin/structure/blocks wasn't showing (500 Error). First thing I did was try and clear the cache, and this was unsuccessful. I then went into the command line, and tried clearing the cache using Drush. Drush returned this error to me:

PHP Fatal error: view::destroy(): The script tried to execute a method or access a property of an incomplete object. Please ensure that the class definition "views_plugin_display_system" of the object you are trying to operate on was loaded _before_ unserialize() gets called or provide a __autoload() function to load the class definition in /var/www/html/xxxxxx/sites/all/modules/views/includes/view.inc on line 1991
Drush command terminated abnormally due to an unrecoverable error. [error]
Error: view::destroy(): The script tried to execute a method or access a property of an incomplete object. Please ensure that the class definition
"views_plugin_display_system" of the object you are trying to operate on was loaded _before_ unserialize() gets called or provide a __autoload() function to load the class
definition in /var/www/html/xxxx/sites/all/modules/views/includes/view.inc, line 1991

I searched based on this error and came to this post you are reading now. When I saw that the error had something to do with Views working on Content I remembered that i had recently installed and then uninstalled the Administrator Views module.

I reinstalled the module, enabled, and tested to see if admin/structure/blocks would render, and it did! I then went back and disabled the module and everything was fixed. I figured that somewhere along the way there was a DB corruption that happened during the uninstall of the module. By simple reinstalling and then uninstalling the module again it seemed to have cleaned up whatever mess that was causing Drupal to throw the 500.

It was admin_views causing this for me as well, thank you jdrefahl.

I got this after uninstalling the module, after the security team marked this project unsupported tonight in SA-CONTRIB-2019-076. I got a white page after clearing the cache and after drush cc all:

PHP Fatal error:  view::destroy(): The script tried to execute a method or access a property of an incomplete object. Please ensure that the class definition "views_plugin_display_system" of the object you are trying to operate on was loaded _before_ unserialize() gets called or provide an autoloader to load the class definition in /var/www/html/[WEBSITEADDRESS]/public_html/sites/all/modules/views/includes/view.inc on line 2047

Reinstalling and uninstalling the module fixed it.