Problem/Motivation

I am creating a master/slave approach with a master site that populate content and taxonomy with various fields.

I needed a way to both accept formatted text in different formats as well as obtain the input format from the feed itself.

Proposed resolution

I added support for this module to handle input filter configurations from the text field instead, as opposed to a global setting currently in views.

Remaining tasks

I have not done any tests so that might probably be missing if needed.

User interface changes

I removed the global setting for input filter in the node processor and add a configuration form for each text field on a per field basis.

API changes

No API change.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

hanoii’s picture

Status: Active » Needs review
FileSize
8.71 KB

Attached is the patch.

hanoii’s picture

Issue summary: View changes

Status: Needs review » Needs work

The last submitted patch, 1: 2224643-1-input-format-per-field.patch, failed testing.

hanoii’s picture

Issue summary: View changes

New patch with the failing test modified, removing the input_format setting.

hanoii’s picture

Issue summary: View changes
FileSize
9.42 KB

Forgot the patch.

hanoii’s picture

Issue summary: View changes
hanoii’s picture

Status: Needs work » Needs review
MegaChriz’s picture

hanoii’s picture

It seems very similar, yes, quite unfortunate, I looked at the patch it the other issue, it looks similar but not the same, not sure which one to give priority to.

rcodina’s picture

Patch on #5 works for me on 7.x-2.x-dev from 2014-Feb-10. Thank you!

I would give priority to your patch because the other issue is too old: feeds module has changed a lot since then. I wish your patch don't get lost and some day gets commited because I think this feature is very useful.

twistor’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

I've close #1588938: Allow selection of filter for each text field imported as a dupe, since this is more up-to-date.

You should hold off on this until #962912: Mapping to node summary goes in, since they will conflict, and that one is pretty much ready. There is one complaint in that issue, if anyone wants to test, hint, hint.

  1. +++ b/mappers/text.inc
    @@ -26,6 +26,18 @@ function text_feeds_processor_targets_alter(&$targets, $entity_type, $bundle_nam
    +      if (isset($instance['settings']['text_processing']) && $instance['settings']['text_processing']) {
    

    !empty()?

  2. +++ b/mappers/text.inc
    @@ -33,10 +45,11 @@ function text_feeds_processor_targets_alter(&$targets, $entity_type, $bundle_nam
    -  if (isset($source->importer->processor->config['input_format'])) {
    -    $format = $source->importer->processor->config['input_format'];
    -  }
    

    We need this for backwards compatibility, otherwise we'll break people's installs.

    Just prefer the mapping setting over the importer setting.

    The importer setting is a nice default anyway, We should probably fallback to that instead of filter_fallback_format().

  3. +++ b/mappers/text.inc
    @@ -49,13 +62,90 @@ function text_feeds_set_target($source, $entity, $target, array $values) {
    +/**
    + * Callback for mapping text fields sub-fields, i.e. format.
    + */
    +function text_feeds_set_target_sub($source, $entity, $target, array $values, $mapping = array()) {
    

    Can we move this to another issue? I'm not convinced that this is useful, and it broadens the scope of the issue considerably.

  4. +++ b/mappers/text.inc
    @@ -49,13 +62,90 @@ function text_feeds_set_target($source, $entity, $target, array $values) {
    + * @return
    + *   Returns, as a string that may contain HTML, the summary to display while
    + *   the full form isn't visible.
    + *   If the return value is empty, no summary and no option to view the form
    + *   will be displayed.
    

    I don't think we need all this.

  5. +++ b/mappers/text.inc
    @@ -49,13 +62,90 @@ function text_feeds_set_target($source, $entity, $target, array $values) {
    +    return t('Input format: %format', array('%format' => $formats[filter_fallback_format()]->name));
    

    Hmmm... Same thing here about falling back to the importer setting.

  6. +++ b/mappers/text.inc
    @@ -49,13 +62,90 @@ function text_feeds_set_target($source, $entity, $target, array $values) {
    + * Select the input format to use for the text field
    

    Period.

  7. +++ b/mappers/text.inc
    @@ -49,13 +62,90 @@ function text_feeds_set_target($source, $entity, $target, array $values) {
    + * @return
    + *   The per mapping configuration form. Once the form is saved, $mapping will
    + *   be populated with the form values.
    + *
    

    Ditch this as well.

  8. +++ b/mappers/text.inc
    @@ -49,13 +62,90 @@ function text_feeds_set_target($source, $entity, $target, array $values) {
    + * @see my_module_summary_callback()
    

    This is all we need for these callbacks.

  9. +++ b/mappers/text.inc
    @@ -49,13 +62,90 @@ function text_feeds_set_target($source, $entity, $target, array $values) {
    +  $formats = filter_formats();
    +  foreach ($formats as $id => $format) {
    +    $formats_options[$id] = $format->name;
    +  }
    

    We need to runs these through filter_access().

  10. +++ b/plugins/FeedsEntityProcessor.inc
    @@ -152,8 +152,6 @@ class FeedsEntityProcessor extends FeedsProcessor {
    -    $form['input_format']['#description'] = t('Select the default input format for the %entity to be created.', array('%entity' => $label_plural));
    -
    

    Leave this.

  11. +++ b/plugins/FeedsProcessor.inc
    @@ -660,14 +660,6 @@ abstract class FeedsProcessor extends FeedsPlugin {
    -    $form['input_format'] = array(
    -      '#type' => 'select',
    -      '#title' => t('Text format'),
    -      '#description' => t('Select the input format for the body field of the nodes to be created.'),
    -      '#options' => $format_options,
    -      '#default_value' => isset($this->config['input_format']) ? $this->config['input_format'] : 'plain_text',
    -      '#required' => TRUE,
    -    );
    
    +++ b/plugins/FeedsTermProcessor.inc
    @@ -34,7 +34,6 @@ class FeedsTermProcessor extends FeedsProcessor {
    -    $term->format = isset($this->config['input_format']) ? $this->config['input_format'] : filter_fallback_format();
    

    And leave this.

It looks good! I think the mapping support is interesting, but it decreases the likelihood of this getting committed promptly. I've got some time coming up, and am trying to get some Feeds things done. This has been on my radar, well, I only wrote a similar patch 2 years ago.

It's important that we not break people's existing installs, so we need backwards compatibility. We can create another issue, about removing the form, and making it more internal if someone is interested. Also, removing the setting would be considered an API change. A crappy API, yes, but if you have to change the tests, then it's an API change. But, that's secondary to ruining user's days.

Needs tests.

Awesome!

MegaChriz’s picture

@hanoii
Now that #962912: Mapping to node summary is in, do you like to work on a new patch yourself?

hanoii’s picture

@MegaChriz ok, I tried to work on the patch after #962912: Mapping to node summary and your notes on #11. I think I covered most except some text things that I don't fully understood but I think they are not there anymore.

Although I think leaving the 'global' input format is not the way to go as it will confuse things, I tried to keep it everywhere. I only tested this from the admin point of view and that it doesn't break anything, I have yet to try it out in the production sites I originally did the first patch to see if it keeps doing the same thing, or test it a bit better, but looks really OK from the coding point of view.

hanoii’s picture

Status: Needs work » Needs review
hanoii’s picture

I actually tested this more deeply and it seems to work as expected.

hanoii’s picture

MegaChriz’s picture

Status: Needs review » Needs work

Thanks for working on this. A quick review (I haven't looked at everything):

  1. +++ b/feeds_import/feeds_import.feeds_importer_default.inc
    @@ -57,7 +57,6 @@ function feeds_import_feeds_importer_default() {
    -        'input_format' => 'plain_text',
    
    +++ b/feeds_news/feeds_news.feeds_importer_default.inc
    @@ -62,7 +62,6 @@ function feeds_news_feeds_importer_default() {
    -        'input_format' => 'filtered_html',
    

    I think these lines should not be removed as the "global" input format still exists (at least for now).

  2. +++ b/mappers/text.inc
    @@ -36,19 +36,41 @@ function text_feeds_processor_targets_alter(&$targets, $entity_type, $bundle_nam
    +  // Logic for backward compatibilty
    
    @@ -60,12 +82,12 @@ function text_feeds_set_target($source, $entity, $target, array $values) {
    +      // Only set format on value, other columns, such as summary and format
    +      // will handled indendently
    
    @@ -73,3 +95,61 @@ function text_feeds_set_target($source, $entity, $target, array $values) {
    + * Summary of input formats
    ...
    +  // Processor-wide input format setting
    ...
    + * Select the input format to use for the text field
    ...
    +  // Processor-wide input format setting
    
    +++ b/plugins/FeedsTermProcessor.inc
    @@ -126,6 +125,14 @@ class FeedsTermProcessor extends FeedsProcessor {
    +        // Add format before follow through to the default
    

    Comments should always end with a period, a exclamation mark or a question mark.

  3. +++ b/mappers/text.inc
    @@ -36,19 +36,41 @@ function text_feeds_processor_targets_alter(&$targets, $entity_type, $bundle_nam
    +      $targets[$name . ':format'] = array(
    +        'name' => check_plain($instance['label']) . ': Format',
    +        'callback' => 'text_feeds_set_target',
    +        'description' => t('The @label field\'s format of the entity. This will overwrite the default or configured format fo the field.', array('@label' => $instance['label'])),
    +        'real_target' => $name
    +      );
    
  4. This is a nice add-on :), but as twistor said in #11 it decreases the likelihood of this getting committed promptly. Have you thought about the case that one with limited privileges could use this mapper to use a format he/she hasn't access to? If the PHP module on the website is enabled, one could set the format to PHP in the source and import PHP code!

Do you want to provide automated tests for this feature as well?

hanoii’s picture

Status: Needs work » Needs review
FileSize
9.39 KB

Ok, 1 and 2 done.

As for 3, I haven't given it a proper thought, and I can see certain security issues. To work more on that, and I will push for this to be accepted as I really need it :) and after all, quite a bit of work put into this patch, but right now I added a mapping setting for the format field, in which you select allowed input formats. Any input format present not specifically allowed, will be ignored.

As for tests, although I'd really like to, I am not with that much time and it's a new thing for me to work on, so the actual time I need for it I guess it will even more.

MegaChriz’s picture

Assigned: Unassigned » MegaChriz

Thanks for your work. :)

I agree mapping to text format is a great feature and I understand you really need it, though still I think it's better to leave it out for now and put the focus solely on the mapping configuration for text fields, so the chances of getting this feature committed soon increase.

I'll plan to write the automated tests.

MegaChriz’s picture

Assigned: MegaChriz » Unassigned
FileSize
15.34 KB
18.06 KB

I've completed the tests and I removed the code for mapping to text format (at least for now). Furthermore, I changed the mapping configuration variable name from "input_format" to "format". Although the processor uses the variable name "input_format", I think "format" is more correct. "input format" was the name that text formats had in Drupal 6.

Three tests were added:

  1. A basic test for setting the "format" mapping configuration, using csv parser and node processor.
  2. A test that tests users with limited privileges can only choose the text format they have access to. Import is not tested during this test.
  3. A test for setting the taxomony term's format, using csv parser and taxonomy term processor.

I'm not yet sure about the implementation for taxonomy term. Now only the text format will be set when mapping to term description. Previously the text format would always be set for new terms (even when not mapping to term description), but never when updating terms.

@hanoii
I hope to work on tests for mapping to text format after the feature in this issue gets in.

twistor’s picture

Status: Needs review » Fixed
Issue tags: -Needs tests

Committed with some additions and cleanups.

  • twistor committed bf4e135 on 7.x-2.x authored by hanoii
    Issue #2224643 by hanoii, MegaChriz: Added Support for input format...
MegaChriz’s picture

Assigned: Unassigned » MegaChriz

Great! Nice cleanups. I only wondered if the following line that was added to the test is right:

+    $this->assertEqual(filter_fallback_format(), $node->field_gamma[LANGUAGE_NONE][0]['format'], 'The gama field got the expected format.');

The gamma field in the test shouldn't have a text format, because text processing is turned off:

+      'gamma' => array(
+        'type' => 'text',
+        'instance_settings' => array(
+          'instance[settings][text_processing]' => 0,
+        ),
+      ),

The test still passes, however. So it's probably good.

Now it's time for the followup: mapping to text format. Last patch where this feature was included was in #18. I'm assigning myself to this issue as a reminder to open the followup issue. I'll post back here once the issue is created.

wonder95’s picture

I've run into a case where I need a standard text field to be imported as plain text and not use the default format (Full HTML), and this looks like what I need, but after installing it, it doesn't seem to give the option to specify a text format for my target text field. I see the option for the Body field, but that's it. I've cleared cache, and I see some changes in the UI for mapping fields, but again, nothing for my text field target. I've also tried removing the mapping to the text field and re-adding it, but that doesn't make a difference. Is it something I need to implement in code? Everything I'm seeing indicates that I should be seeing something different in the UI.

Thanks.

twistor’s picture

Assigned: MegaChriz » Unassigned

The field needs to have Text processing -> "Filtered text (user selects text format)" selected in order for the input format to be selectable.

Edit: Changed previously wrong answer.

twistor’s picture

Oy, didn't mean to unassign MegaChriz.

MegaChriz’s picture

I have opened a follow-up for the other feature that was requested in this issue: #2309273: Mapping to text format
That issue doesn't contain a patch yet, only a description of the feature. The patch from #18 was the last patch from this issue that included the mapping to text format feature. We should use that as a base in #2309273: Mapping to text format.

Status: Fixed » Closed (fixed)

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