As seen in #2537290: Call to undefined function feeds_importer_load_all() trying to run an upgrade for change source and target definitions is crazy. Let's not do it.

We can maintain compatibility by allowing targets and sources to be hidden, that way things can change, but existing installs still work.

CommentFileSizeAuthor
#9 feeds-allow-hidden-targets-2542416-9.patch7.36 KBtwistor
#9 interdiff.txt3.79 KBtwistor
#8 interdiff-2542416-5-8.txt2.18 KBmegachriz
#8 feeds-allow-hidden-targets-2542416-8.patch7.28 KBmegachriz
#5 feeds-allow-hidden-targets-2542416-5.patch6.19 KBtwistor
#4 feeds-allow-hidden-targets-2542416-4.patch3.91 KBtwistor
#2 feeds-allow-hidden-targets-2542416-2.patch3.9 KBtwistor

Comments

twistor’s picture

twistor commented

Also, even force enabling and loading modules to try and run the upgrade won't fix importers that are in disabled features and such.

twistor’s picture

twistor commented
Status: Active » Needs review
StatusFileSize
new feeds-allow-hidden-targets-2542416-2.patch3.9 KB

Status: Needs review » Needs work

The last submitted patch, 2: feeds-allow-hidden-targets-2542416-2.patch, failed testing.

twistor’s picture

twistor commented
Status: Needs work » Needs review
StatusFileSize
new feeds-allow-hidden-targets-2542416-4.patch3.91 KB
twistor’s picture

twistor commented
StatusFileSize
new feeds-allow-hidden-targets-2542416-5.patch6.19 KB
twistor’s picture

twistor commented
Title: All sources and targets to be defined but hidden. » Allow sources and targets to be defined but hidden.
twistor’s picture

twistor commented

This should delete the whole update hook, just make it a no-op.

megachriz’s picture

megachriz
Primary language Dutch
commented
StatusFileSize
new feeds-allow-hidden-targets-2542416-8.patch7.28 KB
new interdiff-2542416-5-8.txt2.18 KB

Nice work. I tested this patch by defining a mapper to "field_image" and by setting one source from FeedsSyndicationParser to be hidden. Both show up as "DEPRECATED" when the importer uses them, so I guess this is good.

My review:

  1. +++ b/feeds_ui/feeds_ui.admin.inc
@@ -830,11 +836,14 @@ function feeds_ui_mapping_form_submit($form, &$form_state) {
+      $result[$k] = $v['name'] . ' (' . (!empty($v['hidden']) ? t('DEPRECATED') : $k) . ')';

    It would be nice if the key of the target is shown even if the target is "hidden". 

  2. +++ b/feeds_ui/feeds_ui.admin.inc
@@ -1028,9 +1044,8 @@ function theme_feeds_ui_mapping_form($variables) {
+      $source = isset($sources[$mapping['source']]) ? check_plain($sources[$mapping['source']]) : check_plain($mapping['source']);

    Further in the code $source gets a second time through check_plain(). This causes text to be double escaped.

  3. If a hidden target is used, in the UI it will show up as "DEPRECATED". Wouldn't it be better then to call this property "deprecated" instead of "hidden"?
  4. The property should be documented in feeds.api.php.

In the attached patch I addressed points 1, 2 and 4 from above. If a hidden target is used, it will show up like this:

Image: URI (field_image) - DEPRECATED

We could debate about what to call the property ("hidden" or "deprecated"). Now it seems not to be consistent to have a target that is marked as hidden and when it used it is marked as deprecated.

twistor’s picture

twistor commented
StatusFileSize
new interdiff.txt3.79 KB
new feeds-allow-hidden-targets-2542416-9.patch7.36 KB

My comment in #7 was supposed to say, "This should not delete the whole update hook, just make it a no-op."

I agree, deprecated makes more sense than hidden. I was just going off of other keys in other parts of the API.

  • MegaChriz committed 12bd52b on 7.x-2.x authored by twistor 
    Issue #2542416 by twistor, MegaChriz: Allow sources and targets to be...
megachriz’s picture

megachriz
Primary language Dutch
commented
Status: Needs review » Fixed

All looks good. Committed #9.

Status: Fixed » Closed (fixed)

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