Comments

twistor’s picture

Status: Active » Needs review
StatusFileSize
new6.77 KB

Status: Needs review » Needs work

The last submitted patch, 1: feeds-generic-entity-language-2529538-1.patch, failed testing.

twistor’s picture

Status: Needs work » Needs review
StatusFileSize
new6.76 KB
megachriz’s picture

@twistor
For my understanding, so I know where to focus on in this issue, does this issue deals with all the issues that I had found with the language setting on the processor in #1183440-234: Multilingual Feeds - Make field import language-aware or only some of them? Additionally, I think I see in the code that if the processor's language is set to a language that is not enabled, Feeds will import the items in LANGUAGE_NONE instead of the configured language. If so, this would answer my question if Feeds should import items in languages that are not available (answer being "no").

My assumption now is that this issue is aimed to fix the following:

  • Provide an option on the processor to set language when the processor's entity has language support.
  • Import items in the language that is set on the processor (if that language is available).
  • Import items in LANGUAGE_NONE if the processor's language is not available.
  • Support for at least the following entity types:
    • Node (via "Locale" module)
    • Taxonomy term (via either the module "Entity translation" or "Taxonomy translation")

Let me know, then I know which automated tests I can write for this issue.

twistor’s picture

You've got it, except for one point:
Entity translation doesn't enable entity language support for entities that don't have it. Confusing, I know. It is more of a field translation module.

As to whether we should import disabled/removed languages. That's debatable, so if you have any opinion, let me know. My stance is to avoid invalid configuration if possible.

megachriz’s picture

Here are some tests using the language setting on the node processor. I hope to write tests for the taxonomy processor later.
The tested scenarios are based on the manual tests I did earlier in #1183440-234: Multilingual Feeds - Make field import language-aware. The following scenarios are tested:

  • Availibility of the language setting on the node processor with the locale module turned on and off.
  • Import in a language.
  • Import in one language, change the language setting and import again.
  • Use language setting in combination with language mapping target.
  • Import with locale module turned off.
  • Import with locale module uninstalled.
  • Import with disabled language.
  • Import with removed language.

The tests are currently failing locally. The only thing that failed was that Feeds still imports in a language when the locale module is disabled. All the other scenarios are passing! I do have to verify though, if the tests are making the right assertions.

As to whether we should import disabled/removed languages. That's debatable, so if you have any opinion, let me know. My stance is to avoid invalid configuration if possible.

I imagine me two possible "bug" reports:

  • When Feeds imports into disabled/removed languages

    I used an importer created on an other website not realising that I had set that one to import in language X. On the website in question that language is not installed. However, in the database/UI is see the nodes are imported in language X and this is causing PHP notices when using the UI of language module Y.

  • When Feeds imports in LANGUAGE_NONE in case of disabled/removed languages

    I created an importer on my local install and had set it to import into language X. After testing the import locally, I pushed the importer live to do the real import, not realising that I hadn't yet installed language X on the live site. Now all my items are imported in LANGUAGE_NONE! Couldn't Feeds at least have warned me before doing the import?

In conclusion, I think it's better that Feeds doesn't import entities in non-existent language. It would be good though to validate the importer configuration when the importer is installed (either as default importer [in a Feature module], or when using the "Import importer" feature). I think importer validation would be out of scope for this issue. This should probably be handled in #2320781: Validate feed importer configuration: check for invalid bundle and invalid language instead.

Status: Needs review » Needs work

The last submitted patch, 6: feeds-generic-entity-language-2529538-6.patch, failed testing.

megachriz’s picture

Status: Needs work » Needs review
StatusFileSize
new21.48 KB
new18.51 KB

And here are also tests for importing multilingual terms. I figured that for node and term some performed tests are the same, so I created a base class called Feedsi18nTestCase for testing multilingual entities and two classes that extends that class: one for node and one for term.

The test for multilingual terms depends on i18n_taxonomy (thanks twistor for pointing out that the Entity translation module doesn't make vocabularies translatable. That's one complexity less). It's possible that the testbot doesn't execute that one cause I believe the testbot doesn't check the patch for additional dependencies. If this is the case, I will do a separate commit just for adding that extra dependency (since I can do commits now, yay!).

Let's see how this goes. Will there still be just two failed assertions, like in #6? We'll see.

Status: Needs review » Needs work

The last submitted patch, 8: feeds-generic-entity-language-2529538-8.patch, failed testing.

  • MegaChriz committed 21ff590 on 7.x-2.x
    by MegaChriz: add i18n_taxonomy as a test dependency for issue #2529538.
    
megachriz’s picture

Status: Needs work » Needs review
StatusFileSize
new21.52 KB
new840 bytes

I've looked at the reason why Feeds is still asigning a language when the locale module is disabled and it appears to be that language_list() returns a list of languages even when the locale module is disabled. Therefore I added a check for the availibility of the locale module to FeedsProcessor::entityLanguage(). The reason why language_list() returns a list of languages when the locale module is disabled is that it must be able to return these early in the bootstrap process at a point that this module isn't yet loaded. See #173227: Optional languages not available.
I've added the test dependency to i18n_taxonomy now in a commit in order for the testbot to execute also the test "Feedsi18nTaxonomyTestCase".

Status: Needs review » Needs work

The last submitted patch, 11: feeds-generic-entity-language-2529538-11.patch, failed testing.

megachriz’s picture

Status: Needs work » Needs review
StatusFileSize
new21.56 KB
new1.64 KB

The reason that the test failed was that Feedsi18nTestCase::setUp() tried to set the processor settings at a point were no taxonomy vocabularies yet were available, thus resulting in a test failure. The attached patch moves setting the processor settings to Feedsi18nNodeTestCase and Feedsi18nTaxonomyTestCase. Let's see if this fixes all the test failures.

stefan.r’s picture

This seems like a nice patch, at first sight this looks good. I don't see any big problems and tests make sense.

  • MegaChriz committed 7912fb5 on 7.x-2.x
    Issue #2529538 by twistor, MegaChriz, stefan.r, drclaw, olofjohansson,...
megachriz’s picture

Status: Needs review » Fixed

Committed! Thanks all. In the commit I have also credited contributors from #1183440: Multilingual Feeds - Make field import language-aware from which I think had a part on this side of the language support issue (language support on the processor).

On to the second part of the language support issue!
#1183440: Multilingual Feeds - Make field import language-aware

Status: Fixed » Closed (fixed)

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