Tried to find corresponding issue - but could only find closed 6.x issues.

My question is: What should "admin/content/webform" show?

Right now it shows all nodes, that potentially could have a webform. I instinctively feel that it should show alle nodes with a specific webform, with at least one component. I guess the record_exist value should be TRUE.

But maybe it's just me :-)

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

quicksketch’s picture

Right now it shows all nodes, that potentially could have a webform. I instinctively feel that it should show alle nodes with a specific webform, with at least one component. I guess the record_exist value should be TRUE.

This page shows:
- ALL "webform" nodes.
- Any other webform-enabled nodes that have ANY CHANGES to the webform default configuration (components, e-mails, settings, etc.)

The second condition is enforced indirectly. Since Webform will not allow rows in the "webform" database table unless the webform settings differ from the defaults. Note that other modules that extend Webform may break this rule if they don't properly implement hook_webform_node_defaults_alter().

quicksketch’s picture

Category: feature » support
Status: Active » Fixed

Unless there are further questions.

mazaza’s picture

Status: Fixed » Active

Actually there is - sorry for the delay - but I tried to make certain that I'm not missing something completely.

If I enable webforms (admin/config/content/webform) on a content type called "Page", and I have 1000 pages - but only 5 of them have webforms - the admin/content/webform still shows 1000 pages.

I tried to look at the code, and could find anything that tried to "hide" the 995 pages without a form.

webform.admin.inc

function webform_admin_content() {
  $webform_types = webform_variable_get('webform_node_types');

  $nodes = array();
  if ($webform_types) {
    $nodes = db_select('node', 'n')
      ->fields('n')
      ->condition('n.type', $webform_types, 'IN')
      ->execute()
      ->fetchAllAssoc('nid');
  }

  return theme('webform_admin_content', array('nodes' => $nodes));
}

This returns all "1000 Pages". Looking further into the node-object - loading it - I found that the $node->webform['record_exists'] was set to TRUE for my 5 pages with webforms.

So writing myself a new theme function (theme_webform_admin_content), I could make a little test, like this:

...
$node_test = node_load($node->nid);
$has_webform = $node_test->webform['record_exists'];
if ($has_webform) {
.. build row
}

Actually the Record Exist - set by the Hook you mention - seems very, so I wondered why it wasn't used.

I think that would be the "normal" behaviour. - only showing pages with webform! But I'm sorry if I've missed something completely.

quicksketch’s picture

Title: What should "admin/content/webform" show? » Webform "admin/content/webform" page should only show nodes with webform configuration, not all webform-enabled types
Category: support » bug
Priority: Normal » Minor

Hm, you're right. That's quite dumb. We go through all this work to make sure that the "webform" database table is nice and clean, but then the SELECT statement is just directly against the "node" table! We should be able to fix this easily enough by adding an INNER JOIN to the webform table.

mazaza’s picture

Thanks for the reply.

INNER JOIN seems right to me too.

alison’s picture

Hi all -- did you end up implementing what was described in quicksketch's comment? (#4)

We should be able to fix this easily enough by adding an INNER JOIN to the webform table.

It doesn't look like it to me, but I haven't dug too deep yet, wanted to check in with this issue queue first.

quicksketch’s picture

No this hasn't been fixed yet. I'm the maintainer too, so I just need to implement my own suggestion. ;)

If you're interested in making the change yourself and filing a patch that could help cut down on testing time for me.

alison’s picture

Okay :) I will try!

weseze’s picture

Attached is a patch that changes the query with a join on the webform table to only show nodes that actually have a webform.

weseze’s picture

Version: 7.x-3.15 » 7.x-3.x-dev
Status: Active » Needs review
quicksketch’s picture

Status: Needs review » Reviewed & tested by the community

Thanks @weseze! I'll try to make sure this gets in the next release.

ParisLiakos’s picture

quicksketch’s picture

Status: Reviewed & tested by the community » Fixed

Upon applying this patch, I get this error on the admin/content/webform page:

Notice: Undefined property: stdClass::$n.nid in DatabaseStatementBase->fetchAllAssoc() (line 2198 of /Users/nate/Sites/drupal7/includes/database/database.inc).

Looks like fetchAllAssoc('n.nid') just needs to be fetchAllAssoc('nid') (like ParisLiakos's version in the other issue). Corrected and committed.

Status: Fixed » Closed (fixed)

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

caspervoogt’s picture

just wanted to chime in to say I too have been struggling with this one. I just updated to 7.x-3.19 and this patch apparently is not included there. So I switched to the dev branch and that works great. But I thought you should know it's not in 7.x-3.19, and I think it ought to be.