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.
Comment | File | Size | Author |
---|---|---|---|
#20 | interdiff-2224643-18-20.txt | 18.06 KB | MegaChriz |
#20 | feeds-format-per-field-2224643-20.patch | 15.34 KB | MegaChriz |
#18 | input-format-per-field-2224643-18-D7.patch | 9.39 KB | hanoii |
#13 | input-format-per-field-2224643-13-D7.patch | 7.77 KB | hanoii |
Comments
Comment #1
hanoiiAttached is the patch.
Comment #2
hanoiiComment #4
hanoiiNew patch with the failing test modified, removing the input_format setting.
Comment #5
hanoiiForgot the patch.
Comment #6
hanoiiComment #7
hanoiiComment #8
MegaChriz CreditAttribution: MegaChriz commentedThis looks like a duplicate of #1588938: Allow selection of filter for each text field imported. Cross referencing for now.
Comment #9
hanoiiIt 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.
Comment #10
rcodina CreditAttribution: rcodina commentedPatch 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.
Comment #11
twistor CreditAttribution: twistor commentedI'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.
!empty()?
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().
Can we move this to another issue? I'm not convinced that this is useful, and it broadens the scope of the issue considerably.
I don't think we need all this.
Hmmm... Same thing here about falling back to the importer setting.
Period.
Ditch this as well.
This is all we need for these callbacks.
We need to runs these through filter_access().
Leave this.
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!
Comment #12
MegaChriz CreditAttribution: MegaChriz commented@hanoii
Now that #962912: Mapping to node summary is in, do you like to work on a new patch yourself?
Comment #13
hanoii@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.
Comment #14
hanoiiComment #15
hanoiiI actually tested this more deeply and it seems to work as expected.
Comment #16
hanoiiComment #17
MegaChriz CreditAttribution: MegaChriz commentedThanks for working on this. A quick review (I haven't looked at everything):
I think these lines should not be removed as the "global" input format still exists (at least for now).
Comments should always end with a period, a exclamation mark or a question mark.
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?
Comment #18
hanoiiOk, 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.
Comment #19
MegaChriz CreditAttribution: MegaChriz commentedThanks 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.
Comment #20
MegaChriz CreditAttribution: MegaChriz commentedI'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:
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.
Comment #21
twistor CreditAttribution: twistor commentedCommitted with some additions and cleanups.
Comment #23
MegaChriz CreditAttribution: MegaChriz commentedGreat! Nice cleanups. I only wondered if the following line that was added to the test is right:
The gamma field in the test shouldn't have a text format, because text processing is turned off:
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.
Comment #24
wonder95 CreditAttribution: wonder95 commentedI'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.
Comment #25
twistor CreditAttribution: twistor commentedThe 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.
Comment #26
twistor CreditAttribution: twistor commentedOy, didn't mean to unassign MegaChriz.
Comment #27
MegaChriz CreditAttribution: MegaChriz commentedI 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.