"_feedapi_store_settings($args, $settings) should be _feedapi_store_settings($settings, $node_type, $nid = NULL), because in this way we can compare if nid settings == node_type settings and only store settings if they are different. Which will allow a much more organic presets behaviour: settings will be "overridable" on a per node basis. If you don't override, you will always be able to control all existing nodes on a per node
type basis (== goodness :)." - alex_b
I extracted this from the code because it should live here as a task.

CommentFileSizeAuthor
#14 201056-14_not_store_per_feed_settings_by_default.patch8.34 KBaron novak
#9 201056_not_store_per_feed_settings_by_default_3.patch9.86 KBaron novak
#7 201056_not_store_per_feed_settings_by_default_2.patch9.87 KBaron novak
#6 201056_not_store_per_feed_settings_by_default_1.patch5.3 KBaron novak
#5 201056_not_store_per_feed_settings_by_default.patch2.13 KBaron novak

Comments

alex_b’s picture

alex_b commented

Thank you aron.

alex_b’s picture

alex_b commented
Title: refactoring _feedapi_store_settings($args, $settings) » Only save per-node settings if different from per-content-type settings
Version: 5.x-1.x-dev » 6.x-1.x-dev
Component: Code » Code feedapi (core module)

I still think we should do this: Only save per-node settings if they're different from per-content-type settings.

alex_b’s picture

alex_b commented
Priority: Minor » Normal

When default settings are used, we should only show a status "Default options used (override)" for users with "adminster advanced feedapi options" permissions. When override is clicked, the FeedAPI node settings form is revealed and can be configured. Likewise, if settings are overridden there should be a "Revert to default options" link.

alex_b’s picture

alex_b commented

Additional requests:

- Introduce global switch that forces FeedAPI to use per-content-type settings.
- Verify that newly added processors fall back cleanly to their defaults on pre-existing feed nodes.

aron novak’s picture

aron novak
Primary language Hungarian
Location Hungary, Budapest
commented
Status: Active » Needs work
StatusFileSize
new 201056_not_store_per_feed_settings_by_default.patch2.13 KB

This is the first iteration of achieving nice support of the requested feature.
Now it does the basics, by default, the feed uses the content-type settings, but you can istruct feedapi to behave differently and specify the settings.
Things to do:
Nice UI support for this if JS is available (hide fields what have no effect)
Global switch that forces FeedAPI to use per-content-type settings.
Verify that newly added processors fall back cleanly to their defaults on pre-existing feed nodes

aron novak’s picture

aron novak
Primary language Hungarian
Location Hungary, Budapest
commented
StatusFileSize
new 201056_not_store_per_feed_settings_by_default_1.patch5.3 KB

Second iteration:
UI support to hide those settings what have no effect
Updated tests what fail otherwise after the patch.
Things to do:
Global switch that forces FeedAPI to use per-content-type settings.
Verify that newly added processors fall back cleanly to their defaults on pre-existing feed nodes

aron novak’s picture

aron novak
Primary language Hungarian
Location Hungary, Budapest
commented
Status: Needs work » Needs review
StatusFileSize
new 201056_not_store_per_feed_settings_by_default_2.patch9.87 KB

Third iteration:
Added:
Global switch that forces FeedAPI to use per-content-type settings.
Verify that newly added processors fall back cleanly to their defaults on pre-existing feed nodes
Fixed:
The jquery UI was bogus, it messed up per-content-type settings page

alex_b’s picture

alex_b commented
Status: Needs review » Needs work

Nice work so far.

- 'overwrite' should be 'override'
- Label of check box does "Use per content-type settings" does not work - should be rather "Override default settings"
- There is a bunch of changes in feedapi_get_settings() - what is their purpose? I had the impression that feedapi_get_settings() could stay untouched.

I'm thinking we should get 1.7 stable ASAP and then roll a 1.8 with this feature.

aron novak’s picture

aron novak
Primary language Hungarian
Location Hungary, Budapest
commented
Status: Needs work » Needs review
StatusFileSize
new 201056_not_store_per_feed_settings_by_default_3.patch9.86 KB

Text and naming issues were fixed.
"bunch of changes in feedapi_get_settings()".
Let me explain. For the basics of the feature (only save per-node settings), these changes are not mandatory.
However you requested, which was a good point, this:

Verify that newly added processors fall back cleanly to their defaults on pre-existing feed nodes.

Correct me if I'm wrong, but i cannot see a way to achieve this without altering that function. In a nutshell: in each cases, at the beginning the node_type settings should be fetched to make possible to fall-back.

The patch conflicts with: #499680: Restructure tests . So if one of these patches are committed, the other needs to be updated.

1.7 stable ASAP - agree. We should not add such a big change after two beta releases.

aron novak’s picture

aron novak
Primary language Hungarian
Location Hungary, Budapest
commented
Priority: Normal » Critical

This is such a patch what I would like to see in FeedAPI 1.9.

aron novak’s picture

aron novak
Primary language Hungarian
Location Hungary, Budapest
commented

#375649: Drop per-feed configurations or this

amir simantov’s picture

amir simantov commented

This is a followup to a post I have posted - #531538: Why Feed URL is not a plural field? which has been stated as duplicated.

I am using a web-based rss aggregator for the task.

alex_b’s picture

alex_b commented
Status: Needs review » Needs work

feedapi_hideadditional -> feedapi_hide_additional
needs to be rerolled to reflect recent changes to HEAD.

aron novak’s picture

aron novak
Primary language Hungarian
Location Hungary, Budapest
commented
Status: Needs work » Needs review
StatusFileSize
new 201056-14_not_store_per_feed_settings_by_default.patch8.34 KB

Rerolled.
feedapi_hideadditional -> feedapi_hide_additional
And I also renamed a similarly ugly CSS class name.

There is a hidden killswitch variable:

variable_get('feedapi_no_per_feed', FALSE)

So with one variable, it's possible to disable site-wide per-feed settings.