To be honest, I'm a little bit confused how this event does not exist already and has there any attempt which tried to add this Rules event to Feeds already? This one looks related and @OnkelTem mentioned his sandbox project which implements this Rules event, but it seems this event still hasn't been added to Feeds.

What I tried to solve by using this Rules event is very simple, I wanted to send out password reset notifications with Rules every time when a new user imported by Feeds.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mxr576 created an issue. See original summary.

mxr576’s picture

Because there was an unfortunate naming choice before, Feeds' named its one and only "Before saving an item imported via ..." event with "feeds_import_[imported_name]", therefore to add my hook I needed to write an update hook which updates all Rules configs which uses this event to use the new "feeds_import_before_[imported_name]" rule event name.

MegaChriz’s picture

The rules integration isn't optimal yet. I'm not sure though if it is a good idea to rename an existing event. You could miss rules configurations that exist in code, for example. Better not take that risk.

For invoking the after rule event, there is already an issue about that: #1886230: "After importing feed" rules event never seems to fire.

Other rules related issues:
#713122: Rules Integration / Working with the feeds field
#2446307: Trigger Feed Import Using Rules
#1870528: Undefined index: content_type in feeds_rules_event_info() (line 56 feeds/feeds.rules.inc
#2477901: Multiple Feeds Import to create single content type

I would close this issue as "won't fix" if you agree.

mxr576’s picture

Status: Active » Needs review

Sorry, but based on the mentioned issues' descriptions and by checking the latest submitted patches in these issues I would say that they are completely unrelated to this issue.

#1886230: "After importing feed" rules event never seems to fire is about a Rules event which triggered after a feeds importer run, mine is about triggering a Rules event after a feed item imported.

I'm not saying my update hook is bulletproof, but let the community to test this patch and provide feedbacks about it. I see the drawbacks of renaming an event at this point, but without renaming introducing a new event with similar name could be really misleading and confusing for developers.

MegaChriz’s picture

#1886230: "After importing feed" rules event never seems to fire is about a Rules event which triggered after a feeds importer run, mine is about triggering a Rules event after a feed item imported.

Ah, I see now.

Just saying, the last time when we tried to update configuration via an update function, that resulted into a lot of trouble for a lot of people. See #2537290: Call to undefined function feeds_importer_load_all() and #2561357: Fatal error when trying to update from 7.x-2.0-alpha9 to 7.x-2.0-beta1. I would rather avoid such thing again, if possible. In the end, we removed the update that caused the trouble.

We could include a database update here, but then I think it would be wise to have tests for it. I made a start at writing tests for database updates in #2537290-9: Call to undefined function feeds_importer_load_all(), but it's quite a hard thing to write tests for.

mxr576’s picture

Thanks for the notice. Just to be clear, in these issues you've tried to update feeds' configs, but my update hook updates rules configs by using rules entity calls. Of course, using pure DTNG calls in this case would not be a proper solution.

MegaChriz’s picture

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

I've added some tests for Rules integration and fixes the following two issues:
#1886230: "After importing feed" rules event never seems to fire
#1870528: Undefined index: content_type in feeds_rules_event_info() (line 56 feeds/feeds.rules.inc

Unfortunately, this also means that your patch no longer applies.

For this to be committed, this would need two tests:

  1. A test that tests if the event 'feeds_import_after_IMPORTER_ID' gets invoked.
  2. A test that tests if Rules configurations are updated properly. Alternatively, it would be simpler to not rename the event for "Before saving an item".
Ivan Berezhnov’s picture

Please check my rerolled add.
I created from comment #2.

Status: Needs review » Needs work
mxr576’s picture

Properly rerolled #2, because #8 contains missing lines of code. This patch works with the latest dev & stable.

mxr576’s picture

MegaChriz’s picture

@mxr576
Do you want to write a test for the update code?

mxr576’s picture

I'm not sure when I will have time for that. If someone can take that over I would be thankful.

Ivan Berezhnov’s picture

Issue tags: +CSKyiv18
Vector-’s picture

So the way I opted to name things in my patch is obviously not ideal for anyone using the existing patches in this issue (as it would break any configurations based on them, but I'm not sure that could be entirely avoided given my proposal here...), but couldn't this be refactored to not require an update event? Then the update event could be handled separately if it's actually desirable to go down the path of attempting to update existing configurations.

Hiding the file because it breaks configs based on existing patches here.

Vector-’s picture

And without the trouble of handling updates, a basic test for this seems like some pretty straightforward copy-paste from the before importing an item test? Here's my attempt at doing so.

Also probably worth explaining that the naming convention used previously in this issue is incompatible with rerolling this without the update because of the potential for collisions - that's why I opted to change it. If we err on the side of not changing existing, 'feeds_after_import_IMPORTER_ID' seems like a reasonable distinction against the current event 'feeds_import_IMPORTER_ID' given that the feeds hooks that could be used to implement these events in an external module are 'hook_feeds_presave' and 'hook_feeds_after_save'?

Same as before, leaving these off the display for now.

Apparently I messed up the tests somehow though...