- Issue created by @ptmkenny
- 🇳🇱Netherlands megachriz
I think that this is a tough one. When an import completes, but some items failed to import, then technically the import successfully finished: there were no exceptions that aborted the import process.
Which items failed to import is recorded in a \Drupal\feeds\State object. In
\Drupal\feeds\Entity\Feed::finishImport()
the state object is asked to display and log messages. After that, in\Drupal\feeds\Entity\Feed::unlock()
, the data about this import is cleared and the data from the State object is destroyed.In
By the way, I see you make a lot of valuable contributions to the Feeds project, @ptmkenny. Do you know that on Drupal Slack there is a weekly Feeds meeting to discuss the progress of the project? It is in the #feeds channel and on every Thursday (18:00 to 19:00 UTC April-October, 19:00 to 20:00 UTC November-March).\Drupal\feeds\Entity\Feed::finishImport()
there is the eventFeedsEvents::IMPORT_FINISHED
being dispatched, so I think there probably lies a key in the solution. Perhaps the Drush command can subscribe to this event? And then determine from that if there were failed items in it? - 🇯🇵Japan ptmkenny
Yeah, I agree this is a tough one. The import behavior definitely makes sense; normally, you would not want to halt processing just because some items fail to import. However, I would like to add a "strict mode" that would throw an exception if any failure occurs.
Unfortunately, I don't think Drush can subscribe to events (I tried asking on Drupal Answers). So it seems quite difficult to access
finishImport
from drush.I tried throwing an exception in
ProcessorBase.php
:public function postProcess(FeedInterface $feed, StateInterface $state) { $tokens = [ '@feed' => $feed->label(), '@item' => $this->getItemLabel(), '@items' => $this->getItemLabelPlural(), ]; if ($state->created) { $state->setMessage($this->formatPlural($state->created, '@feed: Created @count @item.', '@feed: Created @count @items.', $tokens)); } if ($state->updated) { $state->setMessage($this->formatPlural($state->updated, '@feed: Updated @count @item.', '@feed: Updated @count @items.', $tokens)); } if ($state->failed) { $message = $this->formatPlural($state->failed, '@feed: Failed importing @count @item.', '@feed: Failed importing @count @items.', $tokens); throw new FeedsDrushException($message); $state->setMessage($this->formatPlural($state->failed, '@feed: Failed importing @count @item.', '@feed: Failed importing @count @items.', $tokens), 'error'); }
This works but maybe there's a better place?
I thought about adding a
bool $strict = FALSE
parameter to import, but it seems that would be difficult to pass around everywhere.Another way to do it would be to add a config checkbox like "Throw an exception on import failures" and then just check the config value. The advantage of a config value is that it's very easy to set with drush.
If you like this approach, I will make an MR.
Thanks for letting me know about the Feeds meeting; I did not know about that. I've joined the Slack channel. I live in Japan so the meeting is in the middle of the night for me, but I'll be in the channel now from time to time.
- 🇳🇱Netherlands megachriz
Hm, I just thought about that I subscribed to event subscriber in tests. So I would tend to believe that it should be possible.
See for example
\Drupal\Tests\feeds\Kernel\Feeds\Target\FieldTest::testUniqueWithArrayValue()
:/** * Tests handling of array values for a unique target. */ public function testUniqueWithArrayValue() { // Respond to after parse event. $this->container->get('event_dispatcher') ->addListener(FeedsEvents::PARSE, [$this, 'afterParseUniqueWithArrayValue'], FeedsEvents::AFTER); $this->testTargetUnique('field_alpha', 'value', 2); }
And the callback is called during the test. I would think that the same idea can be applied in a drush command.
Though perhaps only if the whole import is processed in a single run. But I think that is the case right now: if I'm correct, the import process when triggered by drush is not batched.
- 🇳🇱Netherlands megachriz
Thanks for letting me know about the Feeds meeting; I did not know about that. I've joined the Slack channel. I live in Japan so the meeting is in the middle of the night for me, but I'll be in the channel now from time to time.
Welcome to the channel! 😀
You can always reply in the meeting afterwards, only then it will take some time to get a response from me.
- 🇯🇵Japan ptmkenny
Ok, here's a proof of concept for this.
I managed to create an event subscriber, but as suggested by 4uk4 on Drupal Answers, the event subscriber has to be a module event subscriber that checks whether it is being executed with drush.
The problem is that the event subscriber as written causes all drush imports to fail with an exception, but the default behavior (no exception) should be kept and this should be an option.
I'm marking this "Needs review" only to hear your thoughts on whether this would be a useful feature to add to the module; if you don't think so, I'll close the MR; sometimes adding extra complexity isn't worth it. In any case, this is a good solution for my case and there is sample code if anyone else needs it in the future.
- 🇳🇱Netherlands megachriz
Thanks, but I wonder if something like the following would work too:
public function __construct(EntityTypeManagerInterface $entity_type_manager, EventDispatcherInterface $event_dispatcher) { $this->entityTypeManager = $entity_type_manager; $this->eventDispatcher = $event_dispatcher; parent::__construct(); } public function importFeed($fid = NULL, array $options = ['import-disabled' => FALSE, 'strict-mode' => FALSE]) { // (...) if ($options['strict-mode])) { $this->eventDispatcher->addListener(FeedsEvents::IMPORT_FINISHED, [$this, 'onImportFinished']); } // User has confirmed importing, start import! $feed->import(); // (...) } public function onImportFinished(ImportFinishedEvent $event, string $event_name) { // (...) }
- 🇯🇵Japan ptmkenny
I added the drush_throw_exception_alternate MR which implemented your suggestion in #8, but when I actually run a command, I get this error:
Ouput: ["Error: Call to a member function addListener() on null in Drupal\\feeds\\Commands\\FeedsDrushCommands->importAllFeeds() (line 295 of \/var\/www\/html\/web\/modules\/contrib\/feeds\/src\/Commands\/FeedsDrushCommands.php)."]
It seems that drush can't inject the event dispatcher.