Support from Acquia helps fund testing for Drupal Acquia logo

Comments

adrien.gibrat’s picture

Here is the patch... my first contribution to drupal! cool, I'm proud, but exhausted (haven't sleep this night)...

Just replace the filterbynodetype_form_alter function by this one:

/**
 * Implementation of hook_form_alter
 */
function filterbynodetype_form_alter($form_id, &$form) {
  if (isset($form['type']) && 'node-form' == $form['#id']) {
    $type = $form['type']['#value'];
    $formats =& $form['body_filter']['format'];
    foreach (element_children($formats) as $element) {
      if (! variable_get('filterbynodetype_' . $formats[$element]['#return_value'] . '_' . $type, 1)) {
        unset($formats[$element]);
      }
    }
    if (2 == count(element_children($formats))) { // 1 format and 1 extra item for the link
      // We don't know what the IDs are of the two fields, so we have to iterate to get them.
      foreach (element_children($formats) as $element) {
        if (isset($formats[$element]['#title'])) {
          // This is a filter, so we assign it to the filter set as the only option.
          $formats[$element] = array(
            '#type' => 'value',
            '#value' => $element,
            '#parents' => $formats[$element]['#parents'],
          );
          $formats['format'] = array( // I have no idea why it uses this structure, but this is what filter.module does.
            '#type' => 'item',
            '#description' => $formats[$element]['#description'],
          );
          foreach (element_properties($formats) as $element) {
            unset($formats[$element]);
          }
		}
      }
    }
    $fields = db_query("SELECT field_name FROM {node_field_instance} WHERE type_name = '$type' AND widget_type = 'text';");
    while($field = db_fetch_object($fields)) {
      foreach (element_children($form[$field->field_name]) as $element) {
        if(isset($form[$field->field_name][$element]['format'])) {
          $form[$field->field_name][$element]['format'] = $formats;
        }
      }
    }
  }
}
Crell’s picture

Interesting addition. Can you roll a proper patch so that I can look at it? Code pasted into the text box here is not useful for reviewing. Thanks.

http://drupal.org/diffandpatch

I did note one problem right off, though. Never put values directly into the query string. Use the %d/%s-based escaping in db_query().

adrien.gibrat’s picture

The final version with module check and syntax highlight + a attachment version


function filterbynodetype_form_alter($form_id, &$form) {
  if (isset($form['type']) && 'node-form' == $form['#id']) {
    $type = $form['type']['#value'];
    $formats =& $form['body_filter']['format'];
    foreach (element_children($formats) as $element) {
      if (! variable_get('filterbynodetype_' . $formats[$element]['#return_value'] . '_' . $type, 1)) {
        unset($formats[$element]);
      }
    }
    if (count(element_children($formats)) <= 2) { // 1 format and 1 extra item for the link
      // We don't know what the IDs are of the two fields, so we have to iterate to get them.
      foreach (element_children($formats) as $element) {
        if (isset($formats[$element]['#title'])) {
          // This is a filter, so we assign it to the filter set as the only option.
          $formats[$element] = array(
            '#type' => 'value',
            '#value' => $element,
            '#parents' => $formats[$element]['#parents'],
          );
          $formats['format'] = array( // I have no idea why it uses this structure, but this is what filter.module does.
            '#type' => 'item',
            '#description' => $formats[$element]['#description'],
          );
          foreach (element_properties($formats) as $element) {
            unset($formats[$element]);
          }
        }
      }
    }
    if (module_exists('content') && module_exists('text')) {
      $fields = db_query("SELECT field_name FROM {node_field_instance} WHERE type_name = '$type' AND widget_type = 'text';");
      while($field = db_fetch_object($fields)) {
        foreach (element_children($form[$field->field_name]) as $element) {
          if (isset($form[$field->field_name][$element]['format'])) {
            $form[$field->field_name][$element]['format'] = $formats;
          }
        }
      }
    }
  }
}

adrien.gibrat’s picture

Thanks for the advice about escaping in db_query()...

And sorry for the patch but i can't do it now: the http://unxutils.sourceforge.net/UnxUtils.zip link is broken... and i'm not very confortable with patch to do it :(

I join the corrected version, you should be able to test it.

adrien.gibrat’s picture

Tanks to this video, putty and a shared host, I finally build a patch with diff!

ricflomag’s picture

Thanks adrian, works great. I had wrote a patch myself, but it was not as smart as yours!

Hope to see it released soon...

ricflomag’s picture

Assigned: Unassigned » ricflomag
Status: Active » Needs review
FileSize
4.11 KB

Well, finally it did not work so well... The issues were:

  1. if the body of the node type is disabled, it would do nothing
  2. the $formats form must be different from one field to another, so it must be rebuilt for each field
  3. if the default input format is removed, all the checkboxes would be left unchecked, so the default input format would be used if the user does not select one.
  4. in particular if only one input format is left available (hence hidden because it is the only choice), but it is not the default one, it would not be selected, so the default input format would be used instead.

Here is a patch against 1.x-dev (18/06/07) to address these issues.

There is another unsolved issue: using firefox (and i guess any gecko based browser), the #default_value may not work, because the browser auto-fills the form with previously cached values, ignoring the checked='checked' attribute. The only workaround i can think of would be to set the default values using javascript when the page is loaded. but this is a drupal-wide matter...

ricflomag’s picture

Posting the whole filterbynodetype.module file too.

Crell’s picture

Status: Needs review » Needs work

Right, so, I finally got a chance to look over the patch in #7. *sigh* Unfortunately, there are a couple of problems:

- You're still putting variable text directly into the query string. That is a security hole. You need to use proper placeholders instead of putting $type directly into the query, and to be clean you should do the same for 'text'. The semi-colon is also not needed.

- You're not restoring the help text for filter text, at least not if there's only one filter remaining. It should still show the help text, just as it does on a normal node.

- Please give _filterbynodetype_form_alter_helper() a proper docblock header, not an inline comment. While you're at it, a more descriptive name than "helper" would be good, too. :-)

- Inline comments should be in sentence format, per coding standards. (Capitalize first word, end with a period, use a complete sentence.)

ricflomag’s picture

Thanks for your review Crell,

Six months ago, it's a long time, i didn't even remember what was that all about ;) I tried to address your comments, the only thing i've left undone is to restore the help text for the filters, as i am not sure how to do this well.

Here is the new patch. I have not tested it though. Please feel free to finish the job :-)

Regards,
Ric.

Crell’s picture

The original module did so; you can probably just copy its code (which I admit is nasty, since this whole module is a nasty hack). Sorry, I don't have time today to forward-port that snippet myself. :-(

ricflomag’s picture

Here it is, but once again, NOT TESTED. Please anyone interested in using it, report your experience :-)

Crell’s picture

Please test a patch before posting it. Also, there is no need to repost the entire file. That is useless for review purposes and just wastes disk space. Also, files ending in .test are reserved for unit test scripts. :-)

ricflomag’s picture

Sorry Crell, i'm doing my best to help and address your comments about the patch, but i don't have much time, like you. I could have say "Sorry, i won't do it" instead of making and posting the patch. I hope you understand.

I've posted the whole file for two reasons: people often have problems or don't know how to apply a patch, so i anticipate their request to be able to download the whole file; and the patch is so important compared to the size of the original file, that it's easier to review the whole file.

Regards.

okeedoak’s picture

Using the "filterbynodetype.module.patched.test (whole file)" seems to work. Good job and thank you!

A couple of minor issues:

  1. In the Site Configuration menu, the menu placement for "Input filter node types" appears on the bottom of the menu and not alphabetically.
  2. Even though certain filters are turned off for a particular CCK type, they still appear in the Default value >> Input Format area when configuring specific CCK fields.

Again these are minor issues.

I'm running Drupal 5.7 and CCK 5.x-1.7

okeedoak’s picture

Found a conflict with Site Notes: http://drupal.org/project/sitenotes Trying to create new Site Note (5.x-1.2) results in the White Screen of Death (WSOD). How can I debug this?

Here's the error:
Fatal error: Call to undefined function _filterbynodetype_form_alter_helper() in filterbynodetype.module on line 35

Tested Site Node creation with the standard 5.x-1.x-dev, 2007-Jun-18 version and there is no conflict.

okeedoak’s picture

FileSize
7.13 KB

I've taken the code from the 5.x-1.x-dev, 2007-Jun-18 version that deals with non CCK types and combined it with the changes in the patch to get a working version.

Results:

  • Story node: works.
  • Custom CCK type: works.
  • Site Node content type: no error, no effect on input types available however. The unpatched 5.x-1.x-dev version had the same behavior though.

Also changed the weight of the menu item to 0 so it appears in alphabetical order with the other items in the Site Configuration menu.

I wonder if other modules that provide their own content types are also unaffected?

Crell’s picture

This module assumes the node form structure defined by node.module for admin-created types. Contrib modules that define node types need to follow the same structure or this module will have no (useful) effect. That is easy to do, however, by simply making the following the start of the form:

$form = node_content_form($node);

That gives you the same structure and conditionals as admin-created nodes for free, which is nice.

I'm going out of town for a few days so won't be able to look at #17 until next week. If anyone else wants to give it a whirl and report back, please do! The main thing to remember is that the help filter text needs to be preserved.

okeedoak’s picture

Tested the patched version from #17 with the new 2008-May-07 Dev version of Site Notes (http://drupal.org/project/sitenotes) which incorporates the changes mentioned in #18. Everything seems to work fine: Site Notes now respects the filter settings of Filter by node type and even the help filter text is displayed correctly. I tested the filter help text:

  1. With multiple allowed input formats -> Input Format fieldgroup choices are correct. Help text is available for each choice.
  2. With a single allowed input format -> No Input Format fieldgroup is shown but help text for the specific active filter is still shown.

Thanks for your post in #18 on how to get the integration process between the two modules to work. (I just passed on the information, I didn't code the changes.)

okeedoak’s picture

Regarding the preservation of Input Format help text in CCK content types with the patch from #17:

  1. With multiple allowed input formats -> Input Format fieldgroup choices are correct. Help text is available for each choice.
  2. With a single allowed input format -> No Input Format fieldgroup is shown and **NO** help text for the specific active filter is shown.

Number two is a bug. Maybe someone who is more qualified than myself could look at the code to see where the problem lies.

One suggestion: on pages with many text areas and only one allowed input format the help text would quickly becomes redundant and clutter the page. If there was a choice on the Site configuration page to show or not show the help text that would be great.

ricflomag’s picture

I've had some time to test the patch i made on #12, correct the error reported by okeedoak on #16 and also change the menu weight to 0.

@okeedoak: The difference with your work on #17, as far as i can see, is that the function _filterbynodetype_form_alter_add_filter() is now used twice, preventing code redundancy. (In the patch i posted on #12, the first call to the function was misspelled and led to the error).

Comment #20 is still to address, sorry to let some work undone.

Summit’s picture

Subscribing, greetings, Martijn

Crell’s picture

Project: Filter by node type » Content Construction Kit (CCK)
Version: 5.x-1.x-dev » 6.x-2.x-dev
Component: Code » text.module

After much consideration, I've decided that this functionality does not belong in filterbynodetype. Limiting what input formats are allowed on a given field should be the responsibility of the field and the module that provides it, that is, node.module for the body and text.module for CCK textfields. filterbynodetype exists only because node.module is too dumb to do so, and that won't change in a stable release. CCK, however, moves faster so we should implement that functionality in CCK directly, not via form_alter black magic.

KarenS, yched, I turn this over to you to figure out how best to implement it. :-)

open-keywords’s picture

@Crell

that's all probably good decisions, but this feature is really needed and what is going to happen for users of D5 and CCK 1.x ?? (looks assigned to 6.x-2.x !) ??

markus_petrux’s picture

Status: Needs work » Closed (won't fix)

Please, search for Better Formats module.